[dpdk-dev] [PATCH 0/3] net/sfc: fix power of 2 alignment macros

[dpdk-dev] [PATCH 0/3] net/sfc: fix power of 2 alignment macros

[Andrew Rybchenko]

P2ROUNDUP() and P2ALIGN() macros are buggy when alignment type is smaller
than type of the value to be aligned.

IS_P2ALIGNED() has no the problem since it cast its arugments
to uintptr_t inside, but fixed anyway to follow the same approach as
new EFX_P2ROUNDUP() and EFX_P2ALIGN().

Patches have checkpatches.sh warnings in base driver since space
is required after sizeof.

Andrew Rybchenko (3):
  net/sfc: fix power of 2 round up when align has smaller type
  net/sfc: fix align to power of 2 when align has smaller type
  net/sfc: unify power of 2 alignment check macro

 drivers/net/sfc/base/ef10_impl.h  |  9 +++---
 drivers/net/sfc/base/ef10_nvram.c |  3 +-
 drivers/net/sfc/base/ef10_rx.c    | 11 ++++---
 drivers/net/sfc/base/efx.h        | 21 ++++++++++---
 drivers/net/sfc/base/efx_mcdi.h   |  9 ++++--
 drivers/net/sfc/base/efx_tx.c     |  4 +--
 drivers/net/sfc/efsys.h           | 51 +++++++++++++++----------------
 drivers/net/sfc/sfc_ethdev.c      |  2 +-
 drivers/net/sfc/sfc_rx.c          |  2 +-
 9 files changed, 64 insertions(+), 48 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH 1/3] net/sfc: fix power of 2 round up when align has smaller type

[Andrew Rybchenko]

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(-)

diff --git a/drivers/net/sfc/base/ef10_impl.h b/drivers/net/sfc/base/ef10_impl.h
index 396180111..7a0004782 100644
--- a/drivers/net/sfc/base/ef10_impl.h
+++ b/drivers/net/sfc/base/ef10_impl.h
@@ -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 */
diff --git a/drivers/net/sfc/base/ef10_nvram.c b/drivers/net/sfc/base/ef10_nvram.c
index 8fc8fd909..0d5378ddf 100644
--- a/drivers/net/sfc/base/ef10_nvram.c
+++ b/drivers/net/sfc/base/ef10_nvram.c
@@ -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);
diff --git a/drivers/net/sfc/base/ef10_rx.c b/drivers/net/sfc/base/ef10_rx.c
index 10eace46c..b087a5d42 100644
--- a/drivers/net/sfc/base/ef10_rx.c
+++ b/drivers/net/sfc/base/ef10_rx.c
@@ -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;
 
diff --git a/drivers/net/sfc/base/efx.h b/drivers/net/sfc/base/efx.h
index 53c7b4212..42195cc9d 100644
--- a/drivers/net/sfc/base/efx.h
+++ b/drivers/net/sfc/base/efx.h
@@ -29,6 +29,10 @@ extern "C" {
 /* The macro expands divider twice */
 #define	EFX_DIV_ROUND_UP(_n, _d)		(((_n) + (_d) - 1) / (_d))
 
+/* Round size up to 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)
 
 /*
diff --git a/drivers/net/sfc/base/efx_mcdi.h b/drivers/net/sfc/base/efx_mcdi.h
index 74cde5075..0941cbdb8 100644
--- a/drivers/net/sfc/base/efx_mcdi.h
+++ b/drivers/net/sfc/base/efx_mcdi.h
@@ -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,
diff --git a/drivers/net/sfc/base/efx_tx.c b/drivers/net/sfc/base/efx_tx.c
index 5cf3dcd3a..e7c5e8089 100644
--- a/drivers/net/sfc/base/efx_tx.c
+++ b/drivers/net/sfc/base/efx_tx.c
@@ -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,
diff --git a/drivers/net/sfc/efsys.h b/drivers/net/sfc/efsys.h
index 762c6eea4..4c122d040 100644
--- a/drivers/net/sfc/efsys.h
+++ b/drivers/net/sfc/efsys.h
@@ -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
diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index 279b58641..1f78a3d8a 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -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;
 	}
 
-- 
2.17.1

[dpdk-dev] [PATCH 2/3] net/sfc: fix align to power of 2 when align has smaller type

[Andrew Rybchenko]

Substitute driver-defined P2ALIGN() with EFX_P2ALIGN() 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_rx.c | 2 +-
 drivers/net/sfc/base/efx.h     | 6 +++++-
 drivers/net/sfc/efsys.h        | 4 ----
 drivers/net/sfc/sfc_rx.c       | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/sfc/base/ef10_rx.c b/drivers/net/sfc/base/ef10_rx.c
index b087a5d42..bb4489bbf 100644
--- a/drivers/net/sfc/base/ef10_rx.c
+++ b/drivers/net/sfc/base/ef10_rx.c
@@ -859,7 +859,7 @@ ef10_rx_qpush(
 	efx_dword_t dword;
 
 	/* Hardware has alignment restriction for WPTR */
-	wptr = P2ALIGN(added, EF10_RX_WPTR_ALIGN);
+	wptr = EFX_P2ALIGN(unsigned int, added, EF10_RX_WPTR_ALIGN);
 	if (pushed == wptr)
 		return;
 
diff --git a/drivers/net/sfc/base/efx.h b/drivers/net/sfc/base/efx.h
index 42195cc9d..6aff68b54 100644
--- a/drivers/net/sfc/base/efx.h
+++ b/drivers/net/sfc/base/efx.h
@@ -29,10 +29,14 @@ extern "C" {
 /* The macro expands divider twice */
 #define	EFX_DIV_ROUND_UP(_n, _d)		(((_n) + (_d) - 1) / (_d))
 
-/* Round size up to nearest power of two. */
+/* Round value up to the nearest power of two. */
 #define	EFX_P2ROUNDUP(_type, _value, _align)	\
 	(-(-(_type)(_value) & -(_type)(_align)))
 
+/* Align value down to the nearest power of two. */
+#define	EFX_P2ALIGN(_type, _value, _align)	\
+	((_type)(_value) & -(_type)(_align))
+
 /* Return codes */
 
 typedef __success(return == 0) int efx_rc_t;
diff --git a/drivers/net/sfc/efsys.h b/drivers/net/sfc/efsys.h
index 4c122d040..79fd3c144 100644
--- a/drivers/net/sfc/efsys.h
+++ b/drivers/net/sfc/efsys.h
@@ -76,10 +76,6 @@ typedef bool boolean_t;
 #define IS_P2ALIGNED(v, a)	((((uintptr_t)(v)) & ((uintptr_t)(a) - 1)) == 0)
 #endif
 
-#ifndef P2ALIGN
-#define P2ALIGN(_x, _a)		((_x) & -(_a))
-#endif
-
 #ifndef ISP2
 #define ISP2(x)			rte_is_power_of_2(x)
 #endif
diff --git a/drivers/net/sfc/sfc_rx.c b/drivers/net/sfc/sfc_rx.c
index 23dff0967..e6809bb64 100644
--- a/drivers/net/sfc/sfc_rx.c
+++ b/drivers/net/sfc/sfc_rx.c
@@ -1019,7 +1019,7 @@ sfc_rx_mb_pool_buf_size(struct sfc_adapter *sa, struct rte_mempool *mb_pool)
 		 * Start is aligned the same or better than end,
 		 * just align length.
 		 */
-		buf_size = P2ALIGN(buf_size, nic_align_end);
+		buf_size = EFX_P2ALIGN(uint32_t, buf_size, nic_align_end);
 	}
 
 	return buf_size;
-- 
2.17.1

[dpdk-dev] [PATCH 3/3] net/sfc: unify power of 2 alignment check macro

[Andrew Rybchenko]

Substitute driver-defined IS_P2ALIGNED() with EFX_IS_P2ALIGNED()
defined in libefx.

Add type argument and cast value and alignment to one specified type.

Fixes: e1b944598579 ("net/sfc: build libefx")
Cc: stable@dpdk.org

Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 drivers/net/sfc/base/ef10_rx.c |  4 ++--
 drivers/net/sfc/base/efx.h     |  4 ++++
 drivers/net/sfc/efsys.h        | 43 +++++++++++++++++++---------------
 3 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/drivers/net/sfc/base/ef10_rx.c b/drivers/net/sfc/base/ef10_rx.c
index bb4489bbf..5f5dd3c62 100644
--- a/drivers/net/sfc/base/ef10_rx.c
+++ b/drivers/net/sfc/base/ef10_rx.c
@@ -1119,12 +1119,12 @@ ef10_rx_qcreate(
 			rc = ENOTSUP;
 			goto fail9;
 		}
-		if (!IS_P2ALIGNED(es_max_dma_len,
+		if (!EFX_IS_P2ALIGNED(uint32_t, es_max_dma_len,
 			    EFX_RX_ES_SUPER_BUFFER_BUF_ALIGNMENT)) {
 			rc = EINVAL;
 			goto fail10;
 		}
-		if (!IS_P2ALIGNED(es_buf_stride,
+		if (!EFX_IS_P2ALIGNED(uint32_t, es_buf_stride,
 			    EFX_RX_ES_SUPER_BUFFER_BUF_ALIGNMENT)) {
 			rc = EINVAL;
 			goto fail11;
diff --git a/drivers/net/sfc/base/efx.h b/drivers/net/sfc/base/efx.h
index 6aff68b54..53ddaa987 100644
--- a/drivers/net/sfc/base/efx.h
+++ b/drivers/net/sfc/base/efx.h
@@ -37,6 +37,10 @@ extern "C" {
 #define	EFX_P2ALIGN(_type, _value, _align)	\
 	((_type)(_value) & -(_type)(_align))
 
+/* Test if value is power of 2 aligned. */
+#define	EFX_IS_P2ALIGNED(_type, _value, _align)	\
+	((((_type)(_value)) & ((_type)(_align) - 1)) == 0)
+
 /* Return codes */
 
 typedef __success(return == 0) int efx_rc_t;
diff --git a/drivers/net/sfc/efsys.h b/drivers/net/sfc/efsys.h
index 79fd3c144..eab5479a4 100644
--- a/drivers/net/sfc/efsys.h
+++ b/drivers/net/sfc/efsys.h
@@ -69,13 +69,6 @@ typedef bool boolean_t;
 #define MIN(v1, v2)	((v1) < (v2) ? (v1) : (v2))
 #endif
 
-/* There are macros for alignment in DPDK, but we need to make a proper
- * correspondence here, if we want to re-use them at all
- */
-#ifndef IS_P2ALIGNED
-#define IS_P2ALIGNED(v, a)	((((uintptr_t)(v)) & ((uintptr_t)(a) - 1)) == 0)
-#endif
-
 #ifndef ISP2
 #define ISP2(x)			rte_is_power_of_2(x)
 #endif
@@ -231,7 +224,8 @@ typedef struct efsys_mem_s {
 		volatile uint32_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_dword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_dword_t)));	\
 									\
 		_addr = (volatile uint32_t *)(_base + (_offset));	\
 		(_edp)->ed_u32[0] = _addr[0];				\
@@ -248,7 +242,8 @@ typedef struct efsys_mem_s {
 		volatile uint64_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_qword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_qword_t)));	\
 									\
 		_addr = (volatile uint64_t *)(_base + (_offset));	\
 		(_eqp)->eq_u64[0] = _addr[0];				\
@@ -266,7 +261,8 @@ typedef struct efsys_mem_s {
 		volatile __m128i *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_oword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_oword_t)));	\
 									\
 		_addr = (volatile __m128i *)(_base + (_offset));	\
 		(_eop)->eo_u128[0] = _addr[0];				\
@@ -287,7 +283,8 @@ typedef struct efsys_mem_s {
 		volatile uint32_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_dword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_dword_t)));	\
 									\
 		EFSYS_PROBE2(mem_writed, unsigned int, (_offset),	\
 					 uint32_t, (_edp)->ed_u32[0]);	\
@@ -304,7 +301,8 @@ typedef struct efsys_mem_s {
 		volatile uint64_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_qword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_qword_t)));	\
 									\
 		EFSYS_PROBE3(mem_writeq, unsigned int, (_offset),	\
 					 uint32_t, (_eqp)->eq_u32[1],	\
@@ -322,7 +320,8 @@ typedef struct efsys_mem_s {
 		volatile __m128i *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_oword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_oword_t)));	\
 									\
 									\
 		EFSYS_PROBE5(mem_writeo, unsigned int, (_offset),	\
@@ -387,7 +386,8 @@ typedef struct efsys_bar_s {
 		volatile uint32_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_dword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_dword_t)));	\
 		_NOTE(CONSTANTCONDITION);				\
 		if (_lock)						\
 			SFC_BAR_LOCK(_esbp);				\
@@ -411,7 +411,8 @@ typedef struct efsys_bar_s {
 		volatile uint64_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_qword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_qword_t)));	\
 									\
 		SFC_BAR_LOCK(_esbp);					\
 									\
@@ -433,7 +434,8 @@ typedef struct efsys_bar_s {
 		volatile __m128i *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_oword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_oword_t)));	\
 									\
 		_NOTE(CONSTANTCONDITION);				\
 		if (_lock)						\
@@ -463,7 +465,8 @@ typedef struct efsys_bar_s {
 		volatile uint32_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_dword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_dword_t)));	\
 									\
 		_NOTE(CONSTANTCONDITION);				\
 		if (_lock)						\
@@ -488,7 +491,8 @@ typedef struct efsys_bar_s {
 		volatile uint64_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_qword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_qword_t)));	\
 									\
 		SFC_BAR_LOCK(_esbp);					\
 									\
@@ -522,7 +526,8 @@ typedef struct efsys_bar_s {
 		volatile __m128i *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_oword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_oword_t)));	\
 									\
 		_NOTE(CONSTANTCONDITION);				\
 		if (_lock)						\
-- 
2.17.1

Re: [dpdk-dev] [PATCH 0/3] net/sfc: fix power of 2 alignment macros

[Andrew Rybchenko]

On 7/24/19 3:59 PM, Andrew Rybchenko wrote:
> P2ROUNDUP() and P2ALIGN() macros are buggy when alignment type is smaller
> than type of the value to be aligned.
>
> IS_P2ALIGNED() has no the problem since it cast its arugments
> to uintptr_t inside, but fixed anyway to follow the same approach as
> new EFX_P2ROUNDUP() and EFX_P2ALIGN().
>
> Patches have checkpatches.sh warnings in base driver since space
> is required after sizeof.
>
> Andrew Rybchenko (3):
>    net/sfc: fix power of 2 round up when align has smaller type
>    net/sfc: fix align to power of 2 when align has smaller type
>    net/sfc: unify power of 2 alignment check macro
>
>   drivers/net/sfc/base/ef10_impl.h  |  9 +++---
>   drivers/net/sfc/base/ef10_nvram.c |  3 +-
>   drivers/net/sfc/base/ef10_rx.c    | 11 ++++---
>   drivers/net/sfc/base/efx.h        | 21 ++++++++++---
>   drivers/net/sfc/base/efx_mcdi.h   |  9 ++++--
>   drivers/net/sfc/base/efx_tx.c     |  4 +--
>   drivers/net/sfc/efsys.h           | 51 +++++++++++++++----------------
>   drivers/net/sfc/sfc_ethdev.c      |  2 +-
>   drivers/net/sfc/sfc_rx.c          |  2 +-
>   9 files changed, 64 insertions(+), 48 deletions(-)
>

Self NACK since the second patch fixes the first one. I'll send v2 shortly.

[dpdk-dev] [PATCH v2 0/3] net/sfc: fix power of 2 alignment macros

[Andrew Rybchenko]

P2ROUNDUP() and P2ALIGN() macros are buggy when alignment type is smaller
than type of the value to be aligned.

IS_P2ALIGNED() has no the problem since it cast its arugments
to uintptr_t inside, but fixed anyway to follow the same approach as
new EFX_P2ROUNDUP() and EFX_P2ALIGN().

Patches have checkpatches.sh warnings in base driver since space
is required after sizeof.

v2:
    - fix [1/3] in accordance with [2/3] changes to be correct from the
      very beginning

Andrew Rybchenko (3):
  net/sfc: fix power of 2 round up when align has smaller type
  net/sfc: fix align to power of 2 when align has smaller type
  net/sfc: unify power of 2 alignment check macro

 drivers/net/sfc/base/ef10_impl.h  |  9 +++---
 drivers/net/sfc/base/ef10_nvram.c |  3 +-
 drivers/net/sfc/base/ef10_rx.c    | 11 ++++---
 drivers/net/sfc/base/efx.h        | 21 ++++++++++---
 drivers/net/sfc/base/efx_mcdi.h   |  9 ++++--
 drivers/net/sfc/base/efx_tx.c     |  4 +--
 drivers/net/sfc/efsys.h           | 51 +++++++++++++++----------------
 drivers/net/sfc/sfc_ethdev.c      |  2 +-
 drivers/net/sfc/sfc_rx.c          |  2 +-
 9 files changed, 64 insertions(+), 48 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH v2 1/3] net/sfc: fix power of 2 round up when align has smaller type

[Andrew Rybchenko]

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(-)

diff --git a/drivers/net/sfc/base/ef10_impl.h b/drivers/net/sfc/base/ef10_impl.h
index 396180111..7a0004782 100644
--- a/drivers/net/sfc/base/ef10_impl.h
+++ b/drivers/net/sfc/base/ef10_impl.h
@@ -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 */
diff --git a/drivers/net/sfc/base/ef10_nvram.c b/drivers/net/sfc/base/ef10_nvram.c
index 8fc8fd909..0d5378ddf 100644
--- a/drivers/net/sfc/base/ef10_nvram.c
+++ b/drivers/net/sfc/base/ef10_nvram.c
@@ -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);
diff --git a/drivers/net/sfc/base/ef10_rx.c b/drivers/net/sfc/base/ef10_rx.c
index 10eace46c..b087a5d42 100644
--- a/drivers/net/sfc/base/ef10_rx.c
+++ b/drivers/net/sfc/base/ef10_rx.c
@@ -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;
 
diff --git a/drivers/net/sfc/base/efx.h b/drivers/net/sfc/base/efx.h
index 53c7b4212..835d057b1 100644
--- a/drivers/net/sfc/base/efx.h
+++ b/drivers/net/sfc/base/efx.h
@@ -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)
 
 /*
diff --git a/drivers/net/sfc/base/efx_mcdi.h b/drivers/net/sfc/base/efx_mcdi.h
index 74cde5075..0941cbdb8 100644
--- a/drivers/net/sfc/base/efx_mcdi.h
+++ b/drivers/net/sfc/base/efx_mcdi.h
@@ -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,
diff --git a/drivers/net/sfc/base/efx_tx.c b/drivers/net/sfc/base/efx_tx.c
index 5cf3dcd3a..e7c5e8089 100644
--- a/drivers/net/sfc/base/efx_tx.c
+++ b/drivers/net/sfc/base/efx_tx.c
@@ -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,
diff --git a/drivers/net/sfc/efsys.h b/drivers/net/sfc/efsys.h
index 762c6eea4..4c122d040 100644
--- a/drivers/net/sfc/efsys.h
+++ b/drivers/net/sfc/efsys.h
@@ -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
diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index 279b58641..1f78a3d8a 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -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;
 	}
 
-- 
2.17.1

[dpdk-dev] [PATCH v2 3/3] net/sfc: unify power of 2 alignment check macro

[Andrew Rybchenko]

Substitute driver-defined IS_P2ALIGNED() with EFX_IS_P2ALIGNED()
defined in libefx.

Add type argument and cast value and alignment to one specified type.

Fixes: e1b944598579 ("net/sfc: build libefx")
Cc: stable@dpdk.org

Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 drivers/net/sfc/base/ef10_rx.c |  4 ++--
 drivers/net/sfc/base/efx.h     |  4 ++++
 drivers/net/sfc/efsys.h        | 43 +++++++++++++++++++---------------
 3 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/drivers/net/sfc/base/ef10_rx.c b/drivers/net/sfc/base/ef10_rx.c
index bb4489bbf..5f5dd3c62 100644
--- a/drivers/net/sfc/base/ef10_rx.c
+++ b/drivers/net/sfc/base/ef10_rx.c
@@ -1119,12 +1119,12 @@ ef10_rx_qcreate(
 			rc = ENOTSUP;
 			goto fail9;
 		}
-		if (!IS_P2ALIGNED(es_max_dma_len,
+		if (!EFX_IS_P2ALIGNED(uint32_t, es_max_dma_len,
 			    EFX_RX_ES_SUPER_BUFFER_BUF_ALIGNMENT)) {
 			rc = EINVAL;
 			goto fail10;
 		}
-		if (!IS_P2ALIGNED(es_buf_stride,
+		if (!EFX_IS_P2ALIGNED(uint32_t, es_buf_stride,
 			    EFX_RX_ES_SUPER_BUFFER_BUF_ALIGNMENT)) {
 			rc = EINVAL;
 			goto fail11;
diff --git a/drivers/net/sfc/base/efx.h b/drivers/net/sfc/base/efx.h
index 6aff68b54..53ddaa987 100644
--- a/drivers/net/sfc/base/efx.h
+++ b/drivers/net/sfc/base/efx.h
@@ -37,6 +37,10 @@ extern "C" {
 #define	EFX_P2ALIGN(_type, _value, _align)	\
 	((_type)(_value) & -(_type)(_align))
 
+/* Test if value is power of 2 aligned. */
+#define	EFX_IS_P2ALIGNED(_type, _value, _align)	\
+	((((_type)(_value)) & ((_type)(_align) - 1)) == 0)
+
 /* Return codes */
 
 typedef __success(return == 0) int efx_rc_t;
diff --git a/drivers/net/sfc/efsys.h b/drivers/net/sfc/efsys.h
index 79fd3c144..eab5479a4 100644
--- a/drivers/net/sfc/efsys.h
+++ b/drivers/net/sfc/efsys.h
@@ -69,13 +69,6 @@ typedef bool boolean_t;
 #define MIN(v1, v2)	((v1) < (v2) ? (v1) : (v2))
 #endif
 
-/* There are macros for alignment in DPDK, but we need to make a proper
- * correspondence here, if we want to re-use them at all
- */
-#ifndef IS_P2ALIGNED
-#define IS_P2ALIGNED(v, a)	((((uintptr_t)(v)) & ((uintptr_t)(a) - 1)) == 0)
-#endif
-
 #ifndef ISP2
 #define ISP2(x)			rte_is_power_of_2(x)
 #endif
@@ -231,7 +224,8 @@ typedef struct efsys_mem_s {
 		volatile uint32_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_dword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_dword_t)));	\
 									\
 		_addr = (volatile uint32_t *)(_base + (_offset));	\
 		(_edp)->ed_u32[0] = _addr[0];				\
@@ -248,7 +242,8 @@ typedef struct efsys_mem_s {
 		volatile uint64_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_qword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_qword_t)));	\
 									\
 		_addr = (volatile uint64_t *)(_base + (_offset));	\
 		(_eqp)->eq_u64[0] = _addr[0];				\
@@ -266,7 +261,8 @@ typedef struct efsys_mem_s {
 		volatile __m128i *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_oword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_oword_t)));	\
 									\
 		_addr = (volatile __m128i *)(_base + (_offset));	\
 		(_eop)->eo_u128[0] = _addr[0];				\
@@ -287,7 +283,8 @@ typedef struct efsys_mem_s {
 		volatile uint32_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_dword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_dword_t)));	\
 									\
 		EFSYS_PROBE2(mem_writed, unsigned int, (_offset),	\
 					 uint32_t, (_edp)->ed_u32[0]);	\
@@ -304,7 +301,8 @@ typedef struct efsys_mem_s {
 		volatile uint64_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_qword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_qword_t)));	\
 									\
 		EFSYS_PROBE3(mem_writeq, unsigned int, (_offset),	\
 					 uint32_t, (_eqp)->eq_u32[1],	\
@@ -322,7 +320,8 @@ typedef struct efsys_mem_s {
 		volatile __m128i *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_oword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_oword_t)));	\
 									\
 									\
 		EFSYS_PROBE5(mem_writeo, unsigned int, (_offset),	\
@@ -387,7 +386,8 @@ typedef struct efsys_bar_s {
 		volatile uint32_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_dword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_dword_t)));	\
 		_NOTE(CONSTANTCONDITION);				\
 		if (_lock)						\
 			SFC_BAR_LOCK(_esbp);				\
@@ -411,7 +411,8 @@ typedef struct efsys_bar_s {
 		volatile uint64_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_qword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_qword_t)));	\
 									\
 		SFC_BAR_LOCK(_esbp);					\
 									\
@@ -433,7 +434,8 @@ typedef struct efsys_bar_s {
 		volatile __m128i *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_oword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_oword_t)));	\
 									\
 		_NOTE(CONSTANTCONDITION);				\
 		if (_lock)						\
@@ -463,7 +465,8 @@ typedef struct efsys_bar_s {
 		volatile uint32_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_dword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_dword_t)));	\
 									\
 		_NOTE(CONSTANTCONDITION);				\
 		if (_lock)						\
@@ -488,7 +491,8 @@ typedef struct efsys_bar_s {
 		volatile uint64_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_qword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_qword_t)));	\
 									\
 		SFC_BAR_LOCK(_esbp);					\
 									\
@@ -522,7 +526,8 @@ typedef struct efsys_bar_s {
 		volatile __m128i *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_oword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_oword_t)));	\
 									\
 		_NOTE(CONSTANTCONDITION);				\
 		if (_lock)						\
-- 
2.17.1

[dpdk-dev] [PATCH v3 0/3] net/sfc: fix power of 2 alignment macros

[Andrew Rybchenko]

P2ROUNDUP() and P2ALIGN() macros are buggy when alignment type is smaller
than type of the value to be aligned.

IS_P2ALIGNED() has no the problem since it cast its arugments
to uintptr_t inside, but fixed anyway to follow the same approach as
new EFX_P2ROUNDUP() and EFX_P2ALIGN().

Patches have checkpatches.sh warnings in base driver since space
is required after sizeof.

v3:
    - add lost [2/3]

v2:
    - fix [1/3] in accordance with [2/3] changes to be correct from the
      very beginning

Andrew Rybchenko (3):
  net/sfc: fix power of 2 round up when align has smaller type
  net/sfc: fix align to power of 2 when align has smaller type
  net/sfc: unify power of 2 alignment check macro

 drivers/net/sfc/base/ef10_impl.h  |  9 +++---
 drivers/net/sfc/base/ef10_nvram.c |  3 +-
 drivers/net/sfc/base/ef10_rx.c    | 11 ++++---
 drivers/net/sfc/base/efx.h        | 21 ++++++++++---
 drivers/net/sfc/base/efx_mcdi.h   |  9 ++++--
 drivers/net/sfc/base/efx_tx.c     |  4 +--
 drivers/net/sfc/efsys.h           | 51 +++++++++++++++----------------
 drivers/net/sfc/sfc_ethdev.c      |  2 +-
 drivers/net/sfc/sfc_rx.c          |  2 +-
 9 files changed, 64 insertions(+), 48 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH v3 1/3] net/sfc: fix power of 2 round up when align has smaller type

[Andrew Rybchenko]

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(-)

diff --git a/drivers/net/sfc/base/ef10_impl.h b/drivers/net/sfc/base/ef10_impl.h
index 396180111..7a0004782 100644
--- a/drivers/net/sfc/base/ef10_impl.h
+++ b/drivers/net/sfc/base/ef10_impl.h
@@ -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 */
diff --git a/drivers/net/sfc/base/ef10_nvram.c b/drivers/net/sfc/base/ef10_nvram.c
index 8fc8fd909..0d5378ddf 100644
--- a/drivers/net/sfc/base/ef10_nvram.c
+++ b/drivers/net/sfc/base/ef10_nvram.c
@@ -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);
diff --git a/drivers/net/sfc/base/ef10_rx.c b/drivers/net/sfc/base/ef10_rx.c
index 10eace46c..b087a5d42 100644
--- a/drivers/net/sfc/base/ef10_rx.c
+++ b/drivers/net/sfc/base/ef10_rx.c
@@ -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;
 
diff --git a/drivers/net/sfc/base/efx.h b/drivers/net/sfc/base/efx.h
index 53c7b4212..835d057b1 100644
--- a/drivers/net/sfc/base/efx.h
+++ b/drivers/net/sfc/base/efx.h
@@ -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)
 
 /*
diff --git a/drivers/net/sfc/base/efx_mcdi.h b/drivers/net/sfc/base/efx_mcdi.h
index 74cde5075..0941cbdb8 100644
--- a/drivers/net/sfc/base/efx_mcdi.h
+++ b/drivers/net/sfc/base/efx_mcdi.h
@@ -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,
diff --git a/drivers/net/sfc/base/efx_tx.c b/drivers/net/sfc/base/efx_tx.c
index 5cf3dcd3a..e7c5e8089 100644
--- a/drivers/net/sfc/base/efx_tx.c
+++ b/drivers/net/sfc/base/efx_tx.c
@@ -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,
diff --git a/drivers/net/sfc/efsys.h b/drivers/net/sfc/efsys.h
index 762c6eea4..4c122d040 100644
--- a/drivers/net/sfc/efsys.h
+++ b/drivers/net/sfc/efsys.h
@@ -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
diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index 279b58641..1f78a3d8a 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -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;
 	}
 
-- 
2.17.1

[dpdk-dev] [PATCH v3 2/3] net/sfc: fix align to power of 2 when align has smaller type

[Andrew Rybchenko]

Substitute driver-defined P2ALIGN() with EFX_P2ALIGN() 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_rx.c | 2 +-
 drivers/net/sfc/base/efx.h     | 4 ++++
 drivers/net/sfc/efsys.h        | 4 ----
 drivers/net/sfc/sfc_rx.c       | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/sfc/base/ef10_rx.c b/drivers/net/sfc/base/ef10_rx.c
index b087a5d42..bb4489bbf 100644
--- a/drivers/net/sfc/base/ef10_rx.c
+++ b/drivers/net/sfc/base/ef10_rx.c
@@ -859,7 +859,7 @@ ef10_rx_qpush(
 	efx_dword_t dword;
 
 	/* Hardware has alignment restriction for WPTR */
-	wptr = P2ALIGN(added, EF10_RX_WPTR_ALIGN);
+	wptr = EFX_P2ALIGN(unsigned int, added, EF10_RX_WPTR_ALIGN);
 	if (pushed == wptr)
 		return;
 
diff --git a/drivers/net/sfc/base/efx.h b/drivers/net/sfc/base/efx.h
index 835d057b1..6aff68b54 100644
--- a/drivers/net/sfc/base/efx.h
+++ b/drivers/net/sfc/base/efx.h
@@ -33,6 +33,10 @@ extern "C" {
 #define	EFX_P2ROUNDUP(_type, _value, _align)	\
 	(-(-(_type)(_value) & -(_type)(_align)))
 
+/* Align value down to the nearest power of two. */
+#define	EFX_P2ALIGN(_type, _value, _align)	\
+	((_type)(_value) & -(_type)(_align))
+
 /* Return codes */
 
 typedef __success(return == 0) int efx_rc_t;
diff --git a/drivers/net/sfc/efsys.h b/drivers/net/sfc/efsys.h
index 4c122d040..79fd3c144 100644
--- a/drivers/net/sfc/efsys.h
+++ b/drivers/net/sfc/efsys.h
@@ -76,10 +76,6 @@ typedef bool boolean_t;
 #define IS_P2ALIGNED(v, a)	((((uintptr_t)(v)) & ((uintptr_t)(a) - 1)) == 0)
 #endif
 
-#ifndef P2ALIGN
-#define P2ALIGN(_x, _a)		((_x) & -(_a))
-#endif
-
 #ifndef ISP2
 #define ISP2(x)			rte_is_power_of_2(x)
 #endif
diff --git a/drivers/net/sfc/sfc_rx.c b/drivers/net/sfc/sfc_rx.c
index 23dff0967..e6809bb64 100644
--- a/drivers/net/sfc/sfc_rx.c
+++ b/drivers/net/sfc/sfc_rx.c
@@ -1019,7 +1019,7 @@ sfc_rx_mb_pool_buf_size(struct sfc_adapter *sa, struct rte_mempool *mb_pool)
 		 * Start is aligned the same or better than end,
 		 * just align length.
 		 */
-		buf_size = P2ALIGN(buf_size, nic_align_end);
+		buf_size = EFX_P2ALIGN(uint32_t, buf_size, nic_align_end);
 	}
 
 	return buf_size;
-- 
2.17.1

[dpdk-dev] [PATCH v3 3/3] net/sfc: unify power of 2 alignment check macro

[Andrew Rybchenko]

Substitute driver-defined IS_P2ALIGNED() with EFX_IS_P2ALIGNED()
defined in libefx.

Add type argument and cast value and alignment to one specified type.

Fixes: e1b944598579 ("net/sfc: build libefx")
Cc: stable@dpdk.org

Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 drivers/net/sfc/base/ef10_rx.c |  4 ++--
 drivers/net/sfc/base/efx.h     |  4 ++++
 drivers/net/sfc/efsys.h        | 43 +++++++++++++++++++---------------
 3 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/drivers/net/sfc/base/ef10_rx.c b/drivers/net/sfc/base/ef10_rx.c
index bb4489bbf..5f5dd3c62 100644
--- a/drivers/net/sfc/base/ef10_rx.c
+++ b/drivers/net/sfc/base/ef10_rx.c
@@ -1119,12 +1119,12 @@ ef10_rx_qcreate(
 			rc = ENOTSUP;
 			goto fail9;
 		}
-		if (!IS_P2ALIGNED(es_max_dma_len,
+		if (!EFX_IS_P2ALIGNED(uint32_t, es_max_dma_len,
 			    EFX_RX_ES_SUPER_BUFFER_BUF_ALIGNMENT)) {
 			rc = EINVAL;
 			goto fail10;
 		}
-		if (!IS_P2ALIGNED(es_buf_stride,
+		if (!EFX_IS_P2ALIGNED(uint32_t, es_buf_stride,
 			    EFX_RX_ES_SUPER_BUFFER_BUF_ALIGNMENT)) {
 			rc = EINVAL;
 			goto fail11;
diff --git a/drivers/net/sfc/base/efx.h b/drivers/net/sfc/base/efx.h
index 6aff68b54..53ddaa987 100644
--- a/drivers/net/sfc/base/efx.h
+++ b/drivers/net/sfc/base/efx.h
@@ -37,6 +37,10 @@ extern "C" {
 #define	EFX_P2ALIGN(_type, _value, _align)	\
 	((_type)(_value) & -(_type)(_align))
 
+/* Test if value is power of 2 aligned. */
+#define	EFX_IS_P2ALIGNED(_type, _value, _align)	\
+	((((_type)(_value)) & ((_type)(_align) - 1)) == 0)
+
 /* Return codes */
 
 typedef __success(return == 0) int efx_rc_t;
diff --git a/drivers/net/sfc/efsys.h b/drivers/net/sfc/efsys.h
index 79fd3c144..eab5479a4 100644
--- a/drivers/net/sfc/efsys.h
+++ b/drivers/net/sfc/efsys.h
@@ -69,13 +69,6 @@ typedef bool boolean_t;
 #define MIN(v1, v2)	((v1) < (v2) ? (v1) : (v2))
 #endif
 
-/* There are macros for alignment in DPDK, but we need to make a proper
- * correspondence here, if we want to re-use them at all
- */
-#ifndef IS_P2ALIGNED
-#define IS_P2ALIGNED(v, a)	((((uintptr_t)(v)) & ((uintptr_t)(a) - 1)) == 0)
-#endif
-
 #ifndef ISP2
 #define ISP2(x)			rte_is_power_of_2(x)
 #endif
@@ -231,7 +224,8 @@ typedef struct efsys_mem_s {
 		volatile uint32_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_dword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_dword_t)));	\
 									\
 		_addr = (volatile uint32_t *)(_base + (_offset));	\
 		(_edp)->ed_u32[0] = _addr[0];				\
@@ -248,7 +242,8 @@ typedef struct efsys_mem_s {
 		volatile uint64_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_qword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_qword_t)));	\
 									\
 		_addr = (volatile uint64_t *)(_base + (_offset));	\
 		(_eqp)->eq_u64[0] = _addr[0];				\
@@ -266,7 +261,8 @@ typedef struct efsys_mem_s {
 		volatile __m128i *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_oword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_oword_t)));	\
 									\
 		_addr = (volatile __m128i *)(_base + (_offset));	\
 		(_eop)->eo_u128[0] = _addr[0];				\
@@ -287,7 +283,8 @@ typedef struct efsys_mem_s {
 		volatile uint32_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_dword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_dword_t)));	\
 									\
 		EFSYS_PROBE2(mem_writed, unsigned int, (_offset),	\
 					 uint32_t, (_edp)->ed_u32[0]);	\
@@ -304,7 +301,8 @@ typedef struct efsys_mem_s {
 		volatile uint64_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_qword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_qword_t)));	\
 									\
 		EFSYS_PROBE3(mem_writeq, unsigned int, (_offset),	\
 					 uint32_t, (_eqp)->eq_u32[1],	\
@@ -322,7 +320,8 @@ typedef struct efsys_mem_s {
 		volatile __m128i *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_oword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_oword_t)));	\
 									\
 									\
 		EFSYS_PROBE5(mem_writeo, unsigned int, (_offset),	\
@@ -387,7 +386,8 @@ typedef struct efsys_bar_s {
 		volatile uint32_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_dword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_dword_t)));	\
 		_NOTE(CONSTANTCONDITION);				\
 		if (_lock)						\
 			SFC_BAR_LOCK(_esbp);				\
@@ -411,7 +411,8 @@ typedef struct efsys_bar_s {
 		volatile uint64_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_qword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_qword_t)));	\
 									\
 		SFC_BAR_LOCK(_esbp);					\
 									\
@@ -433,7 +434,8 @@ typedef struct efsys_bar_s {
 		volatile __m128i *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_oword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_oword_t)));	\
 									\
 		_NOTE(CONSTANTCONDITION);				\
 		if (_lock)						\
@@ -463,7 +465,8 @@ typedef struct efsys_bar_s {
 		volatile uint32_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_dword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_dword_t)));	\
 									\
 		_NOTE(CONSTANTCONDITION);				\
 		if (_lock)						\
@@ -488,7 +491,8 @@ typedef struct efsys_bar_s {
 		volatile uint64_t *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_qword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_qword_t)));	\
 									\
 		SFC_BAR_LOCK(_esbp);					\
 									\
@@ -522,7 +526,8 @@ typedef struct efsys_bar_s {
 		volatile __m128i *_addr;				\
 									\
 		_NOTE(CONSTANTCONDITION);				\
-		SFC_ASSERT(IS_P2ALIGNED(_offset, sizeof(efx_oword_t)));	\
+		SFC_ASSERT(EFX_IS_P2ALIGNED(size_t, _offset,		\
+					    sizeof(efx_oword_t)));	\
 									\
 		_NOTE(CONSTANTCONDITION);				\
 		if (_lock)						\
-- 
2.17.1

Re: [dpdk-dev] [PATCH v3 1/3] net/sfc: fix power of 2 round up when align has smaller type

[Ferruh Yigit]

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?

Re: [dpdk-dev] [PATCH v3 1/3] net/sfc: fix power of 2 round up when align has smaller type

[Andrew Rybchenko]

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.

Re: [dpdk-dev] [PATCH v3 0/3] net/sfc: fix power of 2 alignment macros

[Ferruh Yigit]

On 7/24/2019 2:16 PM, Andrew Rybchenko wrote:
> P2ROUNDUP() and P2ALIGN() macros are buggy when alignment type is smaller
> than type of the value to be aligned.
> 
> IS_P2ALIGNED() has no the problem since it cast its arugments
> to uintptr_t inside, but fixed anyway to follow the same approach as
> new EFX_P2ROUNDUP() and EFX_P2ALIGN().
> 
> Patches have checkpatches.sh warnings in base driver since space
> is required after sizeof.
> 
> v3:
>     - add lost [2/3]
> 
> v2:
>     - fix [1/3] in accordance with [2/3] changes to be correct from the
>       very beginning
> 
> Andrew Rybchenko (3):
>   net/sfc: fix power of 2 round up when align has smaller type
>   net/sfc: fix align to power of 2 when align has smaller type
>   net/sfc: unify power of 2 alignment check macro

Series applied to dpdk-next-net/master, thanks.

Re: [dpdk-dev] [PATCH v3 1/3] net/sfc: fix power of 2 round up when align has smaller type

[Ferruh Yigit]

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.