[v3,1/3] net/sfc: fix power of 2 round up when align has smaller type
Checks
Commit Message
Substitute driver-defined P2ROUNDUP() h with EFX_P2ROUNDUP()
defined in libefx.
Cast value and alignment to one specified type to guarantee result
correctness.
Fixes: e1b944598579 ("net/sfc: build libefx")
Cc: stable@dpdk.org
Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
drivers/net/sfc/base/ef10_impl.h | 9 +++++----
drivers/net/sfc/base/ef10_nvram.c | 3 ++-
drivers/net/sfc/base/ef10_rx.c | 5 +++--
drivers/net/sfc/base/efx.h | 13 +++++++++----
drivers/net/sfc/base/efx_mcdi.h | 9 ++++++---
drivers/net/sfc/base/efx_tx.c | 4 ++--
drivers/net/sfc/efsys.h | 4 ----
drivers/net/sfc/sfc_ethdev.c | 2 +-
8 files changed, 28 insertions(+), 21 deletions(-)
Comments
On 7/24/2019 2:16 PM, Andrew Rybchenko wrote:
> Substitute driver-defined P2ROUNDUP() h with EFX_P2ROUNDUP()
> defined in libefx.
>
> Cast value and alignment to one specified type to guarantee result
> correctness.
>
> Fixes: e1b944598579 ("net/sfc: build libefx")
> Cc: stable@dpdk.org
>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
<...>
> @@ -29,6 +29,10 @@ extern "C" {
> /* The macro expands divider twice */
> #define EFX_DIV_ROUND_UP(_n, _d) (((_n) + (_d) - 1) / (_d))
>
> +/* Round value up to the nearest power of two. */
> +#define EFX_P2ROUNDUP(_type, _value, _align) \
> + (-(-(_type)(_value) & -(_type)(_align)))
> +
I trust you it does what it says J
Just a high level comment, should we have some kind of tools/utilities file in
one of the libraries so everyone can use/share them?
On 7/24/19 7:57 PM, Ferruh Yigit wrote:
> On 7/24/2019 2:16 PM, Andrew Rybchenko wrote:
>> Substitute driver-defined P2ROUNDUP() h with EFX_P2ROUNDUP()
>> defined in libefx.
>>
>> Cast value and alignment to one specified type to guarantee result
>> correctness.
>>
>> Fixes: e1b944598579 ("net/sfc: build libefx")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> <...>
>
>> @@ -29,6 +29,10 @@ extern "C" {
>> /* The macro expands divider twice */
>> #define EFX_DIV_ROUND_UP(_n, _d) (((_n) + (_d) - 1) / (_d))
>>
>> +/* Round value up to the nearest power of two. */
>> +#define EFX_P2ROUNDUP(_type, _value, _align) \
>> + (-(-(_type)(_value) & -(_type)(_align)))
>> +
> I trust you it does what it says J
>
> Just a high level comment, should we have some kind of tools/utilities file in
> one of the libraries so everyone can use/share them?
It is the base driver code and it is used in the base driver, so it can't
rely on any DPDK library. There are RTE_ALIGN_FLOOR/CEIL in
lib/librte_eal/common/include/rte_common.h.
Before the patch the macro was defined in efsys.h (i.e. driver specific
and could use defines available for the driver), but in fact it was
duplicated in too many drivers and we decided to have it in libefx
(base driver in DPDK case) itself.
On 7/24/2019 7:41 PM, Andrew Rybchenko wrote:
> On 7/24/19 7:57 PM, Ferruh Yigit wrote:
>> On 7/24/2019 2:16 PM, Andrew Rybchenko wrote:
>>> Substitute driver-defined P2ROUNDUP() h with EFX_P2ROUNDUP()
>>> defined in libefx.
>>>
>>> Cast value and alignment to one specified type to guarantee result
>>> correctness.
>>>
>>> Fixes: e1b944598579 ("net/sfc: build libefx")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> <...>
>>
>>> @@ -29,6 +29,10 @@ extern "C" {
>>> /* The macro expands divider twice */
>>> #define EFX_DIV_ROUND_UP(_n, _d) (((_n) + (_d) - 1) / (_d))
>>>
>>> +/* Round value up to the nearest power of two. */
>>> +#define EFX_P2ROUNDUP(_type, _value, _align) \
>>> + (-(-(_type)(_value) & -(_type)(_align)))
>>> +
>> I trust you it does what it says J
>>
>> Just a high level comment, should we have some kind of tools/utilities file in
>> one of the libraries so everyone can use/share them?
>
> It is the base driver code and it is used in the base driver, so it can't
> rely on any DPDK library. There are RTE_ALIGN_FLOOR/CEIL in
> lib/librte_eal/common/include/rte_common.h.
>
> Before the patch the macro was defined in efsys.h (i.e. driver specific
> and could use defines available for the driver), but in fact it was
> duplicated in too many drivers and we decided to have it in libefx
> (base driver in DPDK case) itself.
>
Yes there are lots of small functionalities duplicated in various drivers, that
is why I thought perhaps we can unify them.
And the base driver concern is valid for many drivers I think, since those code
are mostly from a shared delivery, using DPDK specific macros in them will cause
more maintenance cost.
@@ -1439,10 +1439,11 @@ ef10_proxy_auth_get_privilege_mask(
#define EFX_RX_PACKED_STREAM_RX_PREFIX_SIZE 8
/* Minimum space for packet in packed stream mode */
-#define EFX_RX_PACKED_STREAM_MIN_PACKET_SPACE \
- P2ROUNDUP(EFX_RX_PACKED_STREAM_RX_PREFIX_SIZE + \
- EFX_MAC_PDU_MIN + \
- EFX_RX_PACKED_STREAM_ALIGNMENT, \
+#define EFX_RX_PACKED_STREAM_MIN_PACKET_SPACE \
+ EFX_P2ROUNDUP(size_t, \
+ EFX_RX_PACKED_STREAM_RX_PREFIX_SIZE + \
+ EFX_MAC_PDU_MIN + \
+ EFX_RX_PACKED_STREAM_ALIGNMENT, \
EFX_RX_PACKED_STREAM_ALIGNMENT)
/* Maximum number of credits */
@@ -367,7 +367,8 @@ tlv_write(
if (len > 0) {
ptr[(len - 1) / sizeof (uint32_t)] = 0;
memcpy(ptr, data, len);
- ptr += P2ROUNDUP(len, sizeof (uint32_t)) / sizeof (*ptr);
+ ptr += EFX_P2ROUNDUP(uint32_t, len,
+ sizeof (uint32_t)) / sizeof (*ptr);
}
return (ptr);
@@ -947,8 +947,9 @@ ef10_rx_qps_packet_info(
*lengthp = EFX_QWORD_FIELD(*qwordp, ES_DZ_PS_RX_PREFIX_ORIG_LEN);
buf_len = EFX_QWORD_FIELD(*qwordp, ES_DZ_PS_RX_PREFIX_CAP_LEN);
- buf_len = P2ROUNDUP(buf_len + EFX_RX_PACKED_STREAM_RX_PREFIX_SIZE,
- EFX_RX_PACKED_STREAM_ALIGNMENT);
+ buf_len = EFX_P2ROUNDUP(uint16_t,
+ buf_len + EFX_RX_PACKED_STREAM_RX_PREFIX_SIZE,
+ EFX_RX_PACKED_STREAM_ALIGNMENT);
*next_offsetp =
current_offset + buf_len + EFX_RX_PACKED_STREAM_ALIGNMENT;
@@ -29,6 +29,10 @@ extern "C" {
/* The macro expands divider twice */
#define EFX_DIV_ROUND_UP(_n, _d) (((_n) + (_d) - 1) / (_d))
+/* Round value up to the nearest power of two. */
+#define EFX_P2ROUNDUP(_type, _value, _align) \
+ (-(-(_type)(_value) & -(_type)(_align)))
+
/* Return codes */
typedef __success(return == 0) int efx_rc_t;
@@ -498,10 +502,10 @@ typedef enum efx_link_mode_e {
+ /* bug16011 */ 16) \
#define EFX_MAC_PDU(_sdu) \
- P2ROUNDUP((_sdu) + EFX_MAC_PDU_ADJUSTMENT, 8)
+ EFX_P2ROUNDUP(size_t, (_sdu) + EFX_MAC_PDU_ADJUSTMENT, 8)
/*
- * Due to the P2ROUNDUP in EFX_MAC_PDU(), EFX_MAC_SDU_FROM_PDU() may give
+ * Due to the EFX_P2ROUNDUP in EFX_MAC_PDU(), EFX_MAC_SDU_FROM_PDU() may give
* the SDU rounded up slightly.
*/
#define EFX_MAC_SDU_FROM_PDU(_pdu) ((_pdu) - EFX_MAC_PDU_ADJUSTMENT)
@@ -587,8 +591,9 @@ efx_mac_stat_name(
#define EFX_MAC_STATS_MASK_BITS_PER_PAGE (8 * sizeof (uint32_t))
-#define EFX_MAC_STATS_MASK_NPAGES \
- (P2ROUNDUP(EFX_MAC_NSTATS, EFX_MAC_STATS_MASK_BITS_PER_PAGE) / \
+#define EFX_MAC_STATS_MASK_NPAGES \
+ (EFX_P2ROUNDUP(uint32_t, EFX_MAC_NSTATS, \
+ EFX_MAC_STATS_MASK_BITS_PER_PAGE) / \
EFX_MAC_STATS_MASK_BITS_PER_PAGE)
/*
@@ -391,6 +391,11 @@ efx_mcdi_phy_module_get_info(
(((mask) & (MC_CMD_PRIVILEGE_MASK_IN_GRP_ ## priv)) == \
(MC_CMD_PRIVILEGE_MASK_IN_GRP_ ## priv))
+#define EFX_MCDI_BUF_SIZE(_in_len, _out_len) \
+ EFX_P2ROUNDUP(size_t, \
+ MAX(MAX(_in_len, _out_len), (2 * sizeof (efx_dword_t))),\
+ sizeof (efx_dword_t))
+
/*
* The buffer size must be a multiple of dword to ensure that MCDI works
* properly with Siena based boards (which use on-chip buffer). Also, it
@@ -398,9 +403,7 @@ efx_mcdi_phy_module_get_info(
* error responses if the request/response buffer sizes are smaller.
*/
#define EFX_MCDI_DECLARE_BUF(_name, _in_len, _out_len) \
- uint8_t _name[P2ROUNDUP(MAX(MAX(_in_len, _out_len), \
- (2 * sizeof (efx_dword_t))), \
- sizeof (efx_dword_t))] = {0}
+ uint8_t _name[EFX_MCDI_BUF_SIZE(_in_len, _out_len)] = {0}
typedef enum efx_mcdi_feature_id_e {
EFX_MCDI_FEATURE_FW_UPDATE = 0,
@@ -799,7 +799,7 @@ siena_tx_qpost(
* Fragments must not span 4k boundaries.
* Here it is a stricter requirement than the maximum length.
*/
- EFSYS_ASSERT(P2ROUNDUP(start + 1,
+ EFSYS_ASSERT(EFX_P2ROUNDUP(efsys_dma_addr_t, start + 1,
etp->et_enp->en_nic_cfg.enc_tx_dma_desc_boundary) >= end);
EFX_TX_DESC(etp, start, size, ebp->eb_eop, added);
@@ -1058,7 +1058,7 @@ siena_tx_qdesc_dma_create(
* Fragments must not span 4k boundaries.
* Here it is a stricter requirement than the maximum length.
*/
- EFSYS_ASSERT(P2ROUNDUP(addr + 1,
+ EFSYS_ASSERT(EFX_P2ROUNDUP(efsys_dma_addr_t, addr + 1,
etp->et_enp->en_nic_cfg.enc_tx_dma_desc_boundary) >= addr + size);
EFSYS_PROBE4(tx_desc_dma_create, unsigned int, etp->et_index,
@@ -76,10 +76,6 @@ typedef bool boolean_t;
#define IS_P2ALIGNED(v, a) ((((uintptr_t)(v)) & ((uintptr_t)(a) - 1)) == 0)
#endif
-#ifndef P2ROUNDUP
-#define P2ROUNDUP(x, align) (-(-(x) & -(align)))
-#endif
-
#ifndef P2ALIGN
#define P2ALIGN(_x, _a) ((_x) & -(_a))
#endif
@@ -937,7 +937,7 @@ sfc_dev_set_mtu(struct rte_eth_dev *dev, uint16_t mtu)
if (pdu > EFX_MAC_PDU_MAX) {
sfc_err(sa, "too big MTU %u (PDU size %u greater than max %u)",
(unsigned int)mtu, (unsigned int)pdu,
- EFX_MAC_PDU_MAX);
+ (unsigned int)EFX_MAC_PDU_MAX);
goto fail_inval;
}