[PATCH 0/5] staging: fsl-dpaa2/eth: Frame buffer work

[PATCH 0/5] staging: fsl-dpaa2/eth: Frame buffer work

[Bogdan Purcareata]

This patchset does some refactoring in the frame buffer area, in order
for it to be in line with firmware (MC) configuration.

Patches 1 - 2 do some label cleanup and move the buffer layout setup
to a dedicated function.

Patch 3 updates tx_data_offset - the offset for Tx frame buffers - to
not account the software annotation area, since it's already accounted
for by the firmware.

Patch 4 updates the required alignment for Rx frame buffers, based on
the accelerator hardware version.

Patch 5 configures a headroom in the Rx frame buffers to prevent
netstack reallocations in forwarding scenarios.

Patchset sent against staging-next.

Bogdan Purcareata (3):
  staging: fsl-dpaa2/eth: Don't account SWA in tx_data_offset
  staging: fsl-dpaa2/eth: Change RX buffer alignment
  staging: fsl-dpaa2/eth: Extra headroom in RX buffers

Ioana Radulescu (2):
  staging: fsl-dpaa2/eth: Label cleanup
  staging: fsl-dpaa2/eth: Split function

 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c | 170 ++++++++++++++-----------
 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h |  44 +++++--
 2 files changed, 129 insertions(+), 85 deletions(-)

-- 
2.7.4

[PATCH 1/5] staging: fsl-dpaa2/eth: Label cleanup

[Bogdan Purcareata]

From: Ioana Radulescu <ruxandra.radulescu@nxp.com>

Clean up goto labels in a couple of functions, by
removing/renaming redundant ones.

Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>
Signed-off-by: Bogdan Purcareata <bogdan.purcareata@nxp.com>
---
 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c | 35 +++++++++++---------------
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
index 9fbc0ee..5d2bd18 100644
--- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
+++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
@@ -1444,34 +1444,32 @@ static struct fsl_mc_device *setup_dpcon(struct dpaa2_eth_priv *priv)
 	err = dpcon_open(priv->mc_io, 0, dpcon->obj_desc.id, &dpcon->mc_handle);
 	if (err) {
 		dev_err(dev, "dpcon_open() failed\n");
-		goto err_open;
+		goto free;
 	}
 
 	err = dpcon_reset(priv->mc_io, 0, dpcon->mc_handle);
 	if (err) {
 		dev_err(dev, "dpcon_reset() failed\n");
-		goto err_reset;
+		goto close;
 	}
 
 	err = dpcon_get_attributes(priv->mc_io, 0, dpcon->mc_handle, &attrs);
 	if (err) {
 		dev_err(dev, "dpcon_get_attributes() failed\n");
-		goto err_get_attr;
+		goto close;
 	}
 
 	err = dpcon_enable(priv->mc_io, 0, dpcon->mc_handle);
 	if (err) {
 		dev_err(dev, "dpcon_enable() failed\n");
-		goto err_enable;
+		goto close;
 	}
 
 	return dpcon;
 
-err_enable:
-err_get_attr:
-err_reset:
+close:
 	dpcon_close(priv->mc_io, 0, dpcon->mc_handle);
-err_open:
+free:
 	fsl_mc_object_free(dpcon);
 
 	return NULL;
@@ -1794,7 +1792,7 @@ static int setup_dpni(struct fsl_mc_device *ls_dev)
 	err = dpni_open(priv->mc_io, 0, ls_dev->obj_desc.id, &priv->mc_token);
 	if (err) {
 		dev_err(dev, "dpni_open() failed\n");
-		goto err_open;
+		return err;
 	}
 
 	ls_dev->mc_io = priv->mc_io;
@@ -1803,14 +1801,14 @@ static int setup_dpni(struct fsl_mc_device *ls_dev)
 	err = dpni_reset(priv->mc_io, 0, priv->mc_token);
 	if (err) {
 		dev_err(dev, "dpni_reset() failed\n");
-		goto err_reset;
+		goto close;
 	}
 
 	err = dpni_get_attributes(priv->mc_io, 0, priv->mc_token,
 				  &priv->dpni_attrs);
 	if (err) {
 		dev_err(dev, "dpni_get_attributes() failed (err=%d)\n", err);
-		goto err_get_attr;
+		goto close;
 	}
 
 	/* Configure buffer layouts */
@@ -1827,7 +1825,7 @@ static int setup_dpni(struct fsl_mc_device *ls_dev)
 				     DPNI_QUEUE_RX, &buf_layout);
 	if (err) {
 		dev_err(dev, "dpni_set_buffer_layout(RX) failed\n");
-		goto err_buf_layout;
+		goto close;
 	}
 
 	/* tx buffer */
@@ -1837,7 +1835,7 @@ static int setup_dpni(struct fsl_mc_device *ls_dev)
 				     DPNI_QUEUE_TX, &buf_layout);
 	if (err) {
 		dev_err(dev, "dpni_set_buffer_layout(TX) failed\n");
-		goto err_buf_layout;
+		goto close;
 	}
 
 	/* tx-confirm buffer */
@@ -1846,7 +1844,7 @@ static int setup_dpni(struct fsl_mc_device *ls_dev)
 				     DPNI_QUEUE_TX_CONFIRM, &buf_layout);
 	if (err) {
 		dev_err(dev, "dpni_set_buffer_layout(TX_CONF) failed\n");
-		goto err_buf_layout;
+		goto close;
 	}
 
 	/* Now that we've set our tx buffer layout, retrieve the minimum
@@ -1856,7 +1854,7 @@ static int setup_dpni(struct fsl_mc_device *ls_dev)
 				      &priv->tx_data_offset);
 	if (err) {
 		dev_err(dev, "dpni_get_tx_data_offset() failed\n");
-		goto err_data_offset;
+		goto close;
 	}
 
 	if ((priv->tx_data_offset % 64) != 0)
@@ -1868,12 +1866,9 @@ static int setup_dpni(struct fsl_mc_device *ls_dev)
 
 	return 0;
 
-err_data_offset:
-err_buf_layout:
-err_get_attr:
-err_reset:
+close:
 	dpni_close(priv->mc_io, 0, priv->mc_token);
-err_open:
+
 	return err;
 }
 
-- 
2.7.4

[PATCH 2/5] staging: fsl-dpaa2/eth: Split function

[Bogdan Purcareata]

From: Ioana Radulescu <ruxandra.radulescu@nxp.com>

Since setup_dpni() became a bit too long, move the buffer layout
configuration to a separate function.

Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>
Signed-off-by: Bogdan Purcareata <bogdan.purcareata@nxp.com>
---
 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c | 79 +++++++++++++++-----------
 1 file changed, 45 insertions(+), 34 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
index 5d2bd18..92faaaf 100644
--- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
+++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
@@ -1776,42 +1776,12 @@ static void free_dpbp(struct dpaa2_eth_priv *priv)
 	fsl_mc_object_free(priv->dpbp_dev);
 }
 
-/* Configure the DPNI object this interface is associated with */
-static int setup_dpni(struct fsl_mc_device *ls_dev)
+static int set_buffer_layout(struct dpaa2_eth_priv *priv)
 {
-	struct device *dev = &ls_dev->dev;
-	struct dpaa2_eth_priv *priv;
-	struct net_device *net_dev;
+	struct device *dev = priv->net_dev->dev.parent;
 	struct dpni_buffer_layout buf_layout = {0};
 	int err;
 
-	net_dev = dev_get_drvdata(dev);
-	priv = netdev_priv(net_dev);
-
-	/* get a handle for the DPNI object */
-	err = dpni_open(priv->mc_io, 0, ls_dev->obj_desc.id, &priv->mc_token);
-	if (err) {
-		dev_err(dev, "dpni_open() failed\n");
-		return err;
-	}
-
-	ls_dev->mc_io = priv->mc_io;
-	ls_dev->mc_handle = priv->mc_token;
-
-	err = dpni_reset(priv->mc_io, 0, priv->mc_token);
-	if (err) {
-		dev_err(dev, "dpni_reset() failed\n");
-		goto close;
-	}
-
-	err = dpni_get_attributes(priv->mc_io, 0, priv->mc_token,
-				  &priv->dpni_attrs);
-	if (err) {
-		dev_err(dev, "dpni_get_attributes() failed (err=%d)\n", err);
-		goto close;
-	}
-
-	/* Configure buffer layouts */
 	/* rx buffer */
 	buf_layout.pass_parser_result = true;
 	buf_layout.pass_frame_status = true;
@@ -1825,7 +1795,7 @@ static int setup_dpni(struct fsl_mc_device *ls_dev)
 				     DPNI_QUEUE_RX, &buf_layout);
 	if (err) {
 		dev_err(dev, "dpni_set_buffer_layout(RX) failed\n");
-		goto close;
+		return err;
 	}
 
 	/* tx buffer */
@@ -1835,7 +1805,7 @@ static int setup_dpni(struct fsl_mc_device *ls_dev)
 				     DPNI_QUEUE_TX, &buf_layout);
 	if (err) {
 		dev_err(dev, "dpni_set_buffer_layout(TX) failed\n");
-		goto close;
+		return err;
 	}
 
 	/* tx-confirm buffer */
@@ -1844,9 +1814,50 @@ static int setup_dpni(struct fsl_mc_device *ls_dev)
 				     DPNI_QUEUE_TX_CONFIRM, &buf_layout);
 	if (err) {
 		dev_err(dev, "dpni_set_buffer_layout(TX_CONF) failed\n");
+		return err;
+	}
+
+	return 0;
+}
+
+/* Configure the DPNI object this interface is associated with */
+static int setup_dpni(struct fsl_mc_device *ls_dev)
+{
+	struct device *dev = &ls_dev->dev;
+	struct dpaa2_eth_priv *priv;
+	struct net_device *net_dev;
+	int err;
+
+	net_dev = dev_get_drvdata(dev);
+	priv = netdev_priv(net_dev);
+
+	/* get a handle for the DPNI object */
+	err = dpni_open(priv->mc_io, 0, ls_dev->obj_desc.id, &priv->mc_token);
+	if (err) {
+		dev_err(dev, "dpni_open() failed\n");
+		return err;
+	}
+
+	ls_dev->mc_io = priv->mc_io;
+	ls_dev->mc_handle = priv->mc_token;
+
+	err = dpni_reset(priv->mc_io, 0, priv->mc_token);
+	if (err) {
+		dev_err(dev, "dpni_reset() failed\n");
 		goto close;
 	}
 
+	err = dpni_get_attributes(priv->mc_io, 0, priv->mc_token,
+				  &priv->dpni_attrs);
+	if (err) {
+		dev_err(dev, "dpni_get_attributes() failed (err=%d)\n", err);
+		goto close;
+	}
+
+	err = set_buffer_layout(priv);
+	if (err)
+		goto close;
+
 	/* Now that we've set our tx buffer layout, retrieve the minimum
 	 * required tx data offset.
 	 */
-- 
2.7.4

[PATCH 3/5] staging: fsl-dpaa2/eth: Don't account SWA in tx_data_offset

[Bogdan Purcareata]

When configuring the Tx buffer layout, the software annotation size is
mentioned, and MC accounts for it when configuring the frame
tx_data_offset. No need to handle it in the driver as well.

Signed-off-by: Bogdan Purcareata <bogdan.purcareata@nxp.com>
---
 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
index 92faaaf..d68c1f5 100644
--- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
+++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
@@ -1872,9 +1872,6 @@ static int setup_dpni(struct fsl_mc_device *ls_dev)
 		dev_warn(dev, "Tx data offset (%d) not a multiple of 64B\n",
 			 priv->tx_data_offset);
 
-	/* Accommodate software annotation space (SWA) */
-	priv->tx_data_offset += DPAA2_ETH_SWA_SIZE;
-
 	return 0;
 
 close:
-- 
2.7.4

Re: [PATCH 3/5] staging: fsl-dpaa2/eth: Don't account SWA in tx_data_offset

[Dan Carpenter]

On Fri, Oct 27, 2017 at 02:11:34PM +0000, Bogdan Purcareata wrote:
> When configuring the Tx buffer layout, the software annotation size is
> mentioned, and MC accounts for it when configuring the frame
> tx_data_offset. No need to handle it in the driver as well.
> 

The impact is that we allocate slightly less memory right?

regards,
dan carpenter

RE: [PATCH 3/5] staging: fsl-dpaa2/eth: Don't account SWA in tx_data_offset

[Bogdan Purcareata]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Friday, October 27, 2017 5:27 PM
> To: Bogdan Purcareata <bogdan.purcareata@nxp.com>
> Cc: Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>;
> gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@driverdev.osuosl.org
> Subject: Re: [PATCH 3/5] staging: fsl-dpaa2/eth: Don't account SWA in
> tx_data_offset
> 
> 
> On Fri, Oct 27, 2017 at 02:11:34PM +0000, Bogdan Purcareata wrote:
> > When configuring the Tx buffer layout, the software annotation size is
> > mentioned, and MC accounts for it when configuring the frame
> > tx_data_offset. No need to handle it in the driver as well.
> >
> 
> The impact is that we allocate slightly less memory right?

Yes, 64B per frame.

Bogdan P.

Re: [PATCH 3/5] staging: fsl-dpaa2/eth: Don't account SWA in tx_data_offset

[Dan Carpenter]

On Fri, Oct 27, 2017 at 02:31:22PM +0000, Bogdan Purcareata wrote:
> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > Sent: Friday, October 27, 2017 5:27 PM
> > To: Bogdan Purcareata <bogdan.purcareata@nxp.com>
> > Cc: Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>;
> > gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@driverdev.osuosl.org
> > Subject: Re: [PATCH 3/5] staging: fsl-dpaa2/eth: Don't account SWA in
> > tx_data_offset
> > 
> > 
> > On Fri, Oct 27, 2017 at 02:11:34PM +0000, Bogdan Purcareata wrote:
> > > When configuring the Tx buffer layout, the software annotation size is
> > > mentioned, and MC accounts for it when configuring the frame
> > > tx_data_offset. No need to handle it in the driver as well.
> > >
> > 
> > The impact is that we allocate slightly less memory right?
> 
> Yes, 64B per frame.

Ok.  Cool.  Please put this kind of stuff in the changelog.  At first I
thought it was maybe a buffer overflow just from reading what was in the
email without looking at the code.

regards,
dan carpenter

RE: [PATCH 3/5] staging: fsl-dpaa2/eth: Don't account SWA in tx_data_offset

[Bogdan Purcareata]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Friday, October 27, 2017 5:34 PM
> To: Bogdan Purcareata <bogdan.purcareata@nxp.com>
> Cc: Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>;
> gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@driverdev.osuosl.org
> Subject: Re: [PATCH 3/5] staging: fsl-dpaa2/eth: Don't account SWA in
> tx_data_offset
> 
> On Fri, Oct 27, 2017 at 02:31:22PM +0000, Bogdan Purcareata wrote:
> > > -----Original Message-----
> > > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > > Sent: Friday, October 27, 2017 5:27 PM
> > > To: Bogdan Purcareata <bogdan.purcareata@nxp.com>
> > > Cc: Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>;
> > > gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > devel@driverdev.osuosl.org
> > > Subject: Re: [PATCH 3/5] staging: fsl-dpaa2/eth: Don't account SWA in
> > > tx_data_offset
> > >
> > >
> > > On Fri, Oct 27, 2017 at 02:11:34PM +0000, Bogdan Purcareata wrote:
> > > > When configuring the Tx buffer layout, the software annotation size is
> > > > mentioned, and MC accounts for it when configuring the frame
> > > > tx_data_offset. No need to handle it in the driver as well.
> > > >
> > >
> > > The impact is that we allocate slightly less memory right?
> >
> > Yes, 64B per frame.
> 
> Ok.  Cool.  Please put this kind of stuff in the changelog.  At first I
> thought it was maybe a buffer overflow just from reading what was in the
> email without looking at the code.

Okay, will update in v2.

Thank you!
Bogdan P.

[PATCH 4/5] staging: fsl-dpaa2/eth: Change RX buffer alignment

[Bogdan Purcareata]

The WRIOP hardware block v1.0.0 (found on LS2080A board)
requires data in RX buffers to be aligned to 256B, but
newer revisions (e.g. on LS2088A, LS1088A) only require
64B alignment.

Check WRIOP version and decide at runtime which alignment
requirement to configure for ingress buffers.

Signed-off-by: Bogdan Purcareata <bogdan.purcareata@nxp.com>
Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>
---
 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c | 18 ++++++++++++++----
 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h | 14 +++++++++++---
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
index d68c1f5..29b4928 100644
--- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
+++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
@@ -766,11 +766,11 @@ static int add_bufs(struct dpaa2_eth_priv *priv, u16 bpid)
 		/* Allocate buffer visible to WRIOP + skb shared info +
 		 * alignment padding
 		 */
-		buf = napi_alloc_frag(DPAA2_ETH_BUF_RAW_SIZE);
+		buf = napi_alloc_frag(DPAA2_ETH_BUF_RAW_SIZE(priv));
 		if (unlikely(!buf))
 			goto err_alloc;
 
-		buf = PTR_ALIGN(buf, DPAA2_ETH_RX_BUF_ALIGN);
+		buf = PTR_ALIGN(buf, priv->rx_buf_align);
 
 		addr = dma_map_single(dev, buf, DPAA2_ETH_RX_BUF_SIZE,
 				      DMA_FROM_DEVICE);
@@ -781,7 +781,7 @@ static int add_bufs(struct dpaa2_eth_priv *priv, u16 bpid)
 
 		/* tracing point */
 		trace_dpaa2_eth_buf_seed(priv->net_dev,
-					 buf, DPAA2_ETH_BUF_RAW_SIZE,
+					 buf, DPAA2_ETH_BUF_RAW_SIZE(priv),
 					 addr, DPAA2_ETH_RX_BUF_SIZE,
 					 bpid);
 	}
@@ -1782,11 +1782,21 @@ static int set_buffer_layout(struct dpaa2_eth_priv *priv)
 	struct dpni_buffer_layout buf_layout = {0};
 	int err;
 
+	/* We need to check for WRIOP version 1.0.0, but depending on the MC
+	 * version, this number is not always provided correctly on rev1.
+	 * We need to check for both alternatives in this situation.
+	 */
+	if (priv->dpni_attrs.wriop_version == DPAA2_WRIOP_VERSION(0, 0, 0) ||
+	    priv->dpni_attrs.wriop_version == DPAA2_WRIOP_VERSION(1, 0, 0))
+		priv->rx_buf_align = DPAA2_ETH_RX_BUF_ALIGN_REV1;
+	else
+		priv->rx_buf_align = DPAA2_ETH_RX_BUF_ALIGN;
+
 	/* rx buffer */
 	buf_layout.pass_parser_result = true;
 	buf_layout.pass_frame_status = true;
 	buf_layout.private_data_size = DPAA2_ETH_SWA_SIZE;
-	buf_layout.data_align = DPAA2_ETH_RX_BUF_ALIGN;
+	buf_layout.data_align = priv->rx_buf_align;
 	buf_layout.options = DPNI_BUF_LAYOUT_OPT_PARSER_RESULT |
 			     DPNI_BUF_LAYOUT_OPT_FRAME_STATUS |
 			     DPNI_BUF_LAYOUT_OPT_PRIVATE_DATA_SIZE |
diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h
index bfbabae..374a99a 100644
--- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h
+++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h
@@ -45,6 +45,8 @@
 
 #include "dpaa2-eth-trace.h"
 
+#define DPAA2_WRIOP_VERSION(x, y, z) ((x) << 10 | (y) << 5 | (z) << 0)
+
 #define DPAA2_ETH_STORE_SIZE		16
 
 /* Maximum number of scatter-gather entries in an ingress frame,
@@ -85,7 +87,12 @@
  */
 #define DPAA2_ETH_RX_BUF_SIZE		2048
 #define DPAA2_ETH_TX_BUF_ALIGN		64
-#define DPAA2_ETH_RX_BUF_ALIGN		256
+/* Due to a limitation in WRIOP 1.0.0, the RX buffer data must be aligned
+ * to 256B. For newer revisions, the requirement is only for 64B alignment
+ */
+#define DPAA2_ETH_RX_BUF_ALIGN_REV1	256
+#define DPAA2_ETH_RX_BUF_ALIGN		64
+
 #define DPAA2_ETH_NEEDED_HEADROOM(p_priv) \
 	((p_priv)->tx_data_offset + DPAA2_ETH_TX_BUF_ALIGN)
 
@@ -93,10 +100,10 @@
  * buffers large enough to allow building an skb around them and also account
  * for alignment restrictions
  */
-#define DPAA2_ETH_BUF_RAW_SIZE \
+#define DPAA2_ETH_BUF_RAW_SIZE(priv) \
 	(DPAA2_ETH_RX_BUF_SIZE + \
 	SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) + \
-	DPAA2_ETH_RX_BUF_ALIGN)
+	(priv)->rx_buf_align)
 
 /* We are accommodating a skb backpointer and some S/G info
  * in the frame's software annotation. The hardware
@@ -318,6 +325,7 @@ struct dpaa2_eth_priv {
 	struct iommu_domain *iommu_domain;
 
 	u16 tx_qdid;
+	u16 rx_buf_align;
 	struct fsl_mc_io *mc_io;
 	/* Cores which have an affine DPIO/DPCON.
 	 * This is the cpu set on which Rx and Tx conf frames are processed
-- 
2.7.4

Re: [PATCH 4/5] staging: fsl-dpaa2/eth: Change RX buffer alignment

[Dan Carpenter]

On Fri, Oct 27, 2017 at 02:11:35PM +0000, Bogdan Purcareata wrote:
> @@ -93,10 +100,10 @@
>   * buffers large enough to allow building an skb around them and also account
>   * for alignment restrictions
>   */
> -#define DPAA2_ETH_BUF_RAW_SIZE \
> +#define DPAA2_ETH_BUF_RAW_SIZE(priv) \
>  	(DPAA2_ETH_RX_BUF_SIZE + \
>  	SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) + \
> -	DPAA2_ETH_RX_BUF_ALIGN)
> +	(priv)->rx_buf_align)
>  

Not related to this patch, but this macro is ugly.  It would be better
as function.

regards,
dan carpenter

RE: [PATCH 4/5] staging: fsl-dpaa2/eth: Change RX buffer alignment

[Bogdan Purcareata]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Friday, October 27, 2017 5:30 PM
> To: Bogdan Purcareata <bogdan.purcareata@nxp.com>
> Cc: Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>;
> gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@driverdev.osuosl.org
> Subject: Re: [PATCH 4/5] staging: fsl-dpaa2/eth: Change RX buffer alignment
> 
> On Fri, Oct 27, 2017 at 02:11:35PM +0000, Bogdan Purcareata wrote:
> > @@ -93,10 +100,10 @@
> >   * buffers large enough to allow building an skb around them and also
> account
> >   * for alignment restrictions
> >   */
> > -#define DPAA2_ETH_BUF_RAW_SIZE \
> > +#define DPAA2_ETH_BUF_RAW_SIZE(priv) \
> >  	(DPAA2_ETH_RX_BUF_SIZE + \
> >  	SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) + \
> > -	DPAA2_ETH_RX_BUF_ALIGN)
> > +	(priv)->rx_buf_align)
> >
> 
> Not related to this patch, but this macro is ugly.  It would be better
> as function.

Okay, will change the macros to inline functions in v2, where applicable.

Thank you!
Bogdan P.

Re: [PATCH 4/5] staging: fsl-dpaa2/eth: Change RX buffer alignment

[Dan Carpenter]

On Fri, Oct 27, 2017 at 02:44:37PM +0000, Bogdan Purcareata wrote:
> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > Sent: Friday, October 27, 2017 5:30 PM
> > To: Bogdan Purcareata <bogdan.purcareata@nxp.com>
> > Cc: Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>;
> > gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@driverdev.osuosl.org
> > Subject: Re: [PATCH 4/5] staging: fsl-dpaa2/eth: Change RX buffer alignment
> > 
> > On Fri, Oct 27, 2017 at 02:11:35PM +0000, Bogdan Purcareata wrote:
> > > @@ -93,10 +100,10 @@
> > >   * buffers large enough to allow building an skb around them and also
> > account
> > >   * for alignment restrictions
> > >   */
> > > -#define DPAA2_ETH_BUF_RAW_SIZE \
> > > +#define DPAA2_ETH_BUF_RAW_SIZE(priv) \
> > >  	(DPAA2_ETH_RX_BUF_SIZE + \
> > >  	SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) + \
> > > -	DPAA2_ETH_RX_BUF_ALIGN)
> > > +	(priv)->rx_buf_align)
> > >
> > 
> > Not related to this patch, but this macro is ugly.  It would be better
> > as function.
> 
> Okay, will change the macros to inline functions in v2, where applicable.
> 

You didn't need to do that, because I said it was "not related to this
change".  I try not to make people redo paches for stuff like this.  But
thanks, it looks nicer now.

regards,
dan carpenter

RE: [PATCH 4/5] staging: fsl-dpaa2/eth: Change RX buffer alignment

[Bogdan Purcareata]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Monday, October 30, 2017 10:56 AM
> To: Bogdan Purcareata <bogdan.purcareata@nxp.com>
> Cc: devel@driverdev.osuosl.org; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 4/5] staging: fsl-dpaa2/eth: Change RX buffer alignment
> 
> On Fri, Oct 27, 2017 at 02:44:37PM +0000, Bogdan Purcareata wrote:
> > > -----Original Message-----
> > > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > > Sent: Friday, October 27, 2017 5:30 PM
> > > To: Bogdan Purcareata <bogdan.purcareata@nxp.com>
> > > Cc: Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>;
> > > gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > devel@driverdev.osuosl.org
> > > Subject: Re: [PATCH 4/5] staging: fsl-dpaa2/eth: Change RX buffer alignment
> > >
> > > On Fri, Oct 27, 2017 at 02:11:35PM +0000, Bogdan Purcareata wrote:
> > > > @@ -93,10 +100,10 @@
> > > >   * buffers large enough to allow building an skb around them and also
> > > account
> > > >   * for alignment restrictions
> > > >   */
> > > > -#define DPAA2_ETH_BUF_RAW_SIZE \
> > > > +#define DPAA2_ETH_BUF_RAW_SIZE(priv) \
> > > >  	(DPAA2_ETH_RX_BUF_SIZE + \
> > > >  	SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) + \
> > > > -	DPAA2_ETH_RX_BUF_ALIGN)
> > > > +	(priv)->rx_buf_align)
> > > >
> > >
> > > Not related to this patch, but this macro is ugly.  It would be better
> > > as function.
> >
> > Okay, will change the macros to inline functions in v2, where applicable.
> >
> 
> You didn't need to do that, because I said it was "not related to this
> change".  I try not to make people redo paches for stuff like this.  But
> thanks, it looks nicer now.

I agree with you, it does look better with inline functions. I know the change
wasn't absolutely necessary, but the code is easier on the eyes in v2, so I
thought it was good enough reason to go for it.

Thanks for the feedback! :)
Bogdan P.

[PATCH 5/5] staging: fsl-dpaa2/eth: Extra headroom in RX buffers

[Bogdan Purcareata]

The needed headroom that we ask the stack to reserve for us in TX
skbs is larger than the headroom available in RX frames, which
leads to skb reallocations in forwarding scenarios involving two
DPNI interfaces.

Configure the hardware to reserve some extra space in the RX
frame headroom to avoid this situation. The value is chosen based
on the Tx frame data offset, the Rx buffer alignment value and the
netdevice required headroom.

The network stack will take care to reserve space for HH_DATA_MOD when
building the skb, so there's no need to account for it in the netdevice
needed headroom.

Signed-off-by: Bogdan Purcareata <bogdan.purcareata@nxp.com>
Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>
---
 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c | 79 +++++++++++++++-----------
 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h | 32 +++++++----
 2 files changed, 67 insertions(+), 44 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
index 29b4928..636beac 100644
--- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
+++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
@@ -135,8 +135,7 @@ static struct sk_buff *build_linear_skb(struct dpaa2_eth_priv *priv,
 
 	ch->buf_count--;
 
-	skb = build_skb(fd_vaddr, DPAA2_ETH_RX_BUF_SIZE +
-			SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
+	skb = build_skb(fd_vaddr, DPAA2_ETH_SKB_SIZE);
 	if (unlikely(!skb))
 		return NULL;
 
@@ -178,8 +177,7 @@ static struct sk_buff *build_frag_skb(struct dpaa2_eth_priv *priv,
 
 		if (i == 0) {
 			/* We build the skb around the first data buffer */
-			skb = build_skb(sg_vaddr, DPAA2_ETH_RX_BUF_SIZE +
-				SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
+			skb = build_skb(sg_vaddr, DPAA2_ETH_SKB_SIZE);
 			if (unlikely(!skb)) {
 				/* Free the first SG entry now, since we already
 				 * unmapped it and obtained the virtual address
@@ -1792,23 +1790,9 @@ static int set_buffer_layout(struct dpaa2_eth_priv *priv)
 	else
 		priv->rx_buf_align = DPAA2_ETH_RX_BUF_ALIGN;
 
-	/* rx buffer */
-	buf_layout.pass_parser_result = true;
+	/* tx buffer */
 	buf_layout.pass_frame_status = true;
 	buf_layout.private_data_size = DPAA2_ETH_SWA_SIZE;
-	buf_layout.data_align = priv->rx_buf_align;
-	buf_layout.options = DPNI_BUF_LAYOUT_OPT_PARSER_RESULT |
-			     DPNI_BUF_LAYOUT_OPT_FRAME_STATUS |
-			     DPNI_BUF_LAYOUT_OPT_PRIVATE_DATA_SIZE |
-			     DPNI_BUF_LAYOUT_OPT_DATA_ALIGN;
-	err = dpni_set_buffer_layout(priv->mc_io, 0, priv->mc_token,
-				     DPNI_QUEUE_RX, &buf_layout);
-	if (err) {
-		dev_err(dev, "dpni_set_buffer_layout(RX) failed\n");
-		return err;
-	}
-
-	/* tx buffer */
 	buf_layout.options = DPNI_BUF_LAYOUT_OPT_FRAME_STATUS |
 			     DPNI_BUF_LAYOUT_OPT_PRIVATE_DATA_SIZE;
 	err = dpni_set_buffer_layout(priv->mc_io, 0, priv->mc_token,
@@ -1827,6 +1811,36 @@ static int set_buffer_layout(struct dpaa2_eth_priv *priv)
 		return err;
 	}
 
+	/* Now that we've set our tx buffer layout, retrieve the minimum
+	 * required tx data offset.
+	 */
+	err = dpni_get_tx_data_offset(priv->mc_io, 0, priv->mc_token,
+				      &priv->tx_data_offset);
+	if (err) {
+		dev_err(dev, "dpni_get_tx_data_offset() failed\n");
+		return err;
+	}
+
+	if ((priv->tx_data_offset % 64) != 0)
+		dev_warn(dev, "Tx data offset (%d) not a multiple of 64B\n",
+			 priv->tx_data_offset);
+
+	/* rx buffer */
+	buf_layout.pass_parser_result = true;
+	buf_layout.data_align = priv->rx_buf_align;
+	buf_layout.data_head_room = DPAA2_ETH_RX_HEAD_ROOM(priv);
+	buf_layout.private_data_size = 0;
+	buf_layout.options = DPNI_BUF_LAYOUT_OPT_PARSER_RESULT |
+			     DPNI_BUF_LAYOUT_OPT_FRAME_STATUS |
+			     DPNI_BUF_LAYOUT_OPT_DATA_ALIGN |
+			     DPNI_BUF_LAYOUT_OPT_DATA_HEAD_ROOM;
+	err = dpni_set_buffer_layout(priv->mc_io, 0, priv->mc_token,
+				     DPNI_QUEUE_RX, &buf_layout);
+	if (err) {
+		dev_err(dev, "dpni_set_buffer_layout(RX) failed\n");
+		return err;
+	}
+
 	return 0;
 }
 
@@ -1868,19 +1882,6 @@ static int setup_dpni(struct fsl_mc_device *ls_dev)
 	if (err)
 		goto close;
 
-	/* Now that we've set our tx buffer layout, retrieve the minimum
-	 * required tx data offset.
-	 */
-	err = dpni_get_tx_data_offset(priv->mc_io, 0, priv->mc_token,
-				      &priv->tx_data_offset);
-	if (err) {
-		dev_err(dev, "dpni_get_tx_data_offset() failed\n");
-		goto close;
-	}
-
-	if ((priv->tx_data_offset % 64) != 0)
-		dev_warn(dev, "Tx data offset (%d) not a multiple of 64B\n",
-			 priv->tx_data_offset);
 
 	return 0;
 
@@ -2272,6 +2273,7 @@ static int netdev_init(struct net_device *net_dev)
 {
 	struct device *dev = net_dev->dev.parent;
 	struct dpaa2_eth_priv *priv = netdev_priv(net_dev);
+	u16 rx_headroom, req_headroom;
 	u8 bcast_addr[ETH_ALEN];
 	u8 num_queues;
 	int err;
@@ -2295,6 +2297,19 @@ static int netdev_init(struct net_device *net_dev)
 	 */
 	net_dev->needed_headroom = DPAA2_ETH_NEEDED_HEADROOM(priv);
 
+	/* If headroom guaranteed by hardware in the Rx frame buffer is
+	 * smaller than the Tx headroom required by the stack, issue a
+	 * one time warning. This will most likely mean skbs forwarded to
+	 * another DPAA2 network interface will get reallocated, with a
+	 * significant performance impact.
+	 */
+	req_headroom = LL_RESERVED_SPACE(net_dev) - ETH_HLEN;
+	rx_headroom = ALIGN(DPAA2_ETH_RX_HWA_SIZE +
+			    DPAA2_ETH_RX_HEAD_ROOM(priv), priv->rx_buf_align);
+	if (req_headroom > rx_headroom)
+		dev_info_once(dev, "Required headroom (%d) greater than available (%d)\n",
+			      req_headroom, rx_headroom);
+
 	/* Set MTU limits */
 	net_dev->min_mtu = 68;
 	net_dev->max_mtu = DPAA2_ETH_MAX_MTU;
diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h
index 374a99a..234d612 100644
--- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h
+++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h
@@ -82,10 +82,7 @@
  */
 #define DPAA2_ETH_BUFS_PER_CMD		7
 
-/* Hardware requires alignment for ingress/egress buffer addresses
- * and ingress buffer lengths.
- */
-#define DPAA2_ETH_RX_BUF_SIZE		2048
+/* Hardware requires alignment for ingress/egress buffer addresses */
 #define DPAA2_ETH_TX_BUF_ALIGN		64
 /* Due to a limitation in WRIOP 1.0.0, the RX buffer data must be aligned
  * to 256B. For newer revisions, the requirement is only for 64B alignment
@@ -94,16 +91,20 @@
 #define DPAA2_ETH_RX_BUF_ALIGN		64
 
 #define DPAA2_ETH_NEEDED_HEADROOM(p_priv) \
-	((p_priv)->tx_data_offset + DPAA2_ETH_TX_BUF_ALIGN)
+	((p_priv)->tx_data_offset + DPAA2_ETH_TX_BUF_ALIGN - HH_DATA_MOD)
 
-/* Hardware only sees DPAA2_ETH_RX_BUF_SIZE, but we need to allocate ingress
- * buffers large enough to allow building an skb around them and also account
- * for alignment restrictions
+/* Hardware only sees DPAA2_ETH_RX_BUF_SIZE, but the skb built around
+ * the buffer also needs space for its shared info struct, and we need
+ * to allocate enough to accommodate hardware alignment restrictions
  */
+#define DPAA2_ETH_RX_BUF_SIZE		2048
+#define DPAA2_ETH_SKB_SIZE \
+	(DPAA2_ETH_RX_BUF_SIZE + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))
 #define DPAA2_ETH_BUF_RAW_SIZE(priv) \
-	(DPAA2_ETH_RX_BUF_SIZE + \
-	SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) + \
-	(priv)->rx_buf_align)
+	(DPAA2_ETH_SKB_SIZE + (priv)->rx_buf_align)
+
+/* Hardware annotation area in RX  buffers */
+#define DPAA2_ETH_RX_HWA_SIZE		64
 
 /* We are accommodating a skb backpointer and some S/G info
  * in the frame's software annotation. The hardware
@@ -111,6 +112,13 @@
  */
 #define DPAA2_ETH_SWA_SIZE		64
 
+/* Extra headroom space requested to hardware, in order to make sure there's
+ * no realloc'ing in forwarding scenarios
+ */
+#define DPAA2_ETH_RX_HEAD_ROOM(priv) \
+	(DPAA2_ETH_NEEDED_HEADROOM(priv) -	\
+	 DPAA2_ETH_RX_HWA_SIZE)
+
 /* Must keep this struct smaller than DPAA2_ETH_SWA_SIZE */
 struct dpaa2_eth_swa {
 	struct sk_buff *skb;
@@ -141,7 +149,7 @@ struct dpaa2_eth_swa {
 					 DPAA2_FD_CTRL_FAERR)
 
 /* Annotation bits in FD CTRL */
-#define DPAA2_FD_CTRL_ASAL		0x00020000	/* ASAL = 128 */
+#define DPAA2_FD_CTRL_ASAL		0x00010000	/* ASAL = 64 */
 #define DPAA2_FD_CTRL_PTA		0x00800000
 #define DPAA2_FD_CTRL_PTV1		0x00400000
 
-- 
2.7.4