[PATCH 0/5] staging: fsl-dpaa2/eth: Cleanup and minor fixes

[PATCH 0/5] staging: fsl-dpaa2/eth: Cleanup and minor fixes

[Ioana Radulescu]

One bug fix, one small functional change and a couple of
cleanup patches.

Ioana Radulescu (5):
  staging: fsl-dpaa2/eth: Fix DMA mapping direction
  staging: fsl-dpaa2/eth: Remove obsolete reference
  staging: fsl-dpaa2/eth: Remove pointless instruction
  staging: fsl-dpaa2/eth: MTU cleanup
  staging: fsl-dpaa2/eth: Remove Rx frame size check

 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c | 34 ++++++--------------------
 1 file changed, 8 insertions(+), 26 deletions(-)

-- 
2.7.4

[PATCH 1/5] staging: fsl-dpaa2/eth: Fix DMA mapping direction

[Ioana Radulescu]

We are using DMA_FROM_DEVICE when mapping RX frame buffers,
but DMA_BIDIRECTIONAL for unmap. Fix the direction for DMA
unmapping operation.

Fixes: 87eb55e418b7 ("staging: fsl-dpaa2/eth: Fix potential endless loop")

Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>
---
 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
index 3963717..537d5bb 100644
--- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
+++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
@@ -767,7 +767,7 @@ static void free_bufs(struct dpaa2_eth_priv *priv, u64 *buf_array, int count)
 	for (i = 0; i < count; i++) {
 		vaddr = dpaa2_iova_to_virt(priv->iommu_domain, buf_array[i]);
 		dma_unmap_single(dev, buf_array[i], DPAA2_ETH_RX_BUF_SIZE,
-				 DMA_BIDIRECTIONAL);
+				 DMA_FROM_DEVICE);
 		skb_free_frag(vaddr);
 	}
 }
-- 
2.7.4

[PATCH 2/5] staging: fsl-dpaa2/eth: Remove obsolete reference

[Ioana Radulescu]

Commit 2b7c86eb7bf3 ("staging: fsl-dpaa2/eth: Don't enable FAS on Tx")
removed the status field from the TX confirm frame annotation,
but a reference to it remained in the description of free_tx_fd().
Remove it.

Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>
---
 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
index 537d5bb..6331e8d 100644
--- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
+++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
@@ -522,8 +522,6 @@ static int build_single_fd(struct dpaa2_eth_priv *priv,
  * back-pointed to is also freed.
  * This can be called either from dpaa2_eth_tx_conf() or on the error path of
  * dpaa2_eth_tx().
- * Optionally, return the frame annotation status word (FAS), which needs
- * to be checked if we're on the confirmation path.
  */
 static void free_tx_fd(const struct dpaa2_eth_priv *priv,
 		       const struct dpaa2_fd *fd)
-- 
2.7.4

[PATCH 3/5] staging: fsl-dpaa2/eth: Remove pointless instruction

[Ioana Radulescu]

We don't need to call dev_set_drvdata(dev, NULL) on driver
remove since core kernel code also performs this step.

Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>
---
 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
index 6331e8d..439b260 100644
--- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
+++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
@@ -2676,7 +2676,6 @@ static int dpaa2_eth_remove(struct fsl_mc_device *ls_dev)
 
 	fsl_mc_portal_free(priv->mc_io);
 
-	dev_set_drvdata(dev, NULL);
 	free_netdev(net_dev);
 
 	dev_dbg(net_dev->dev.parent, "Removed interface %s\n", net_dev->name);
-- 
2.7.4

[PATCH 4/5] staging: fsl-dpaa2/eth: MTU cleanup

[Ioana Radulescu]

Don't set the lower MTU limit explicitly, since we use
the default value anyway.

Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>
---
 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
index 439b260..24e069c 100644
--- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
+++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
@@ -2383,8 +2383,7 @@ static int netdev_init(struct net_device *net_dev)
 		return err;
 	}
 
-	/* Set MTU limits */
-	net_dev->min_mtu = 68;
+	/* Set MTU upper limit; lower limit is 68B (default value) */
 	net_dev->max_mtu = DPAA2_ETH_MAX_MTU;
 
 	/* Set actual number of queues in the net device */
-- 
2.7.4

[PATCH 5/5] staging: fsl-dpaa2/eth: Remove Rx frame size check

[Ioana Radulescu]

Most Ethernet drivers don't enforce the MTU value as upper limit
for ingress frames. We too support receiving frames larger than
MTU, so allow that.

Remove our ndo_change_mtu implementation, letting the default
stack implementation handle things. Also, set the max frame length
allowed by hardware only once at probe time, with the largest
possible value.

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

diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
index 24e069c..4ae2371 100644
--- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
+++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
@@ -1243,25 +1243,6 @@ static void dpaa2_eth_get_stats(struct net_device *net_dev,
 	}
 }
 
-static int dpaa2_eth_change_mtu(struct net_device *net_dev, int mtu)
-{
-	struct dpaa2_eth_priv *priv = netdev_priv(net_dev);
-	int err;
-
-	/* Set the maximum Rx frame length to match the transmit side;
-	 * account for L2 headers when computing the MFL
-	 */
-	err = dpni_set_max_frame_length(priv->mc_io, 0, priv->mc_token,
-					(u16)DPAA2_ETH_L2_MAX_FRM(mtu));
-	if (err) {
-		netdev_err(net_dev, "dpni_set_max_frame_length() failed\n");
-		return err;
-	}
-
-	net_dev->mtu = mtu;
-	return 0;
-}
-
 /* Copy mac unicast addresses from @net_dev to @priv.
  * Its sole purpose is to make dpaa2_eth_set_rx_mode() more readable.
  */
@@ -1469,7 +1450,6 @@ static const struct net_device_ops dpaa2_eth_ops = {
 	.ndo_init = dpaa2_eth_init,
 	.ndo_set_mac_address = dpaa2_eth_set_addr,
 	.ndo_get_stats64 = dpaa2_eth_get_stats,
-	.ndo_change_mtu = dpaa2_eth_change_mtu,
 	.ndo_set_rx_mode = dpaa2_eth_set_rx_mode,
 	.ndo_set_features = dpaa2_eth_set_features,
 	.ndo_do_ioctl = dpaa2_eth_ioctl,
@@ -2385,6 +2365,12 @@ static int netdev_init(struct net_device *net_dev)
 
 	/* Set MTU upper limit; lower limit is 68B (default value) */
 	net_dev->max_mtu = DPAA2_ETH_MAX_MTU;
+	err = dpni_set_max_frame_length(priv->mc_io, 0, priv->mc_token,
+					(u16)DPAA2_ETH_MFL);
+	if (err) {
+		dev_err(dev, "dpni_set_max_frame_length() failed\n");
+		return err;
+	}
 
 	/* Set actual number of queues in the net device */
 	num_queues = dpaa2_eth_queue_count(priv);
-- 
2.7.4

Re: [PATCH 5/5] staging: fsl-dpaa2/eth: Remove Rx frame size check

[Dan Carpenter]

On Mon, Jul 09, 2018 at 10:01:11AM -0500, Ioana Radulescu wrote:
> @@ -2385,6 +2365,12 @@ static int netdev_init(struct net_device *net_dev)
>  
>  	/* Set MTU upper limit; lower limit is 68B (default value) */
>  	net_dev->max_mtu = DPAA2_ETH_MAX_MTU;
> +	err = dpni_set_max_frame_length(priv->mc_io, 0, priv->mc_token,
> +					(u16)DPAA2_ETH_MFL);

The cast was there in the original code so this is not a comment on this
particular patch (which seems fine) but there is no need to cast.

Generally it's best to avoid unnecessary casts.  As a human reader, I
find the cast confusing.  It indicates that DPAA2_ETH_MFL somehow
requires special handling.  Perhaps it's negative or we are trying to
truncate away the high bits.  But neither of those things really make
sense.

From a static analysis perspective if DPAA2_ETH_MFL doesn't fit nicely
then we would want to generate a warning.  But the cast explicitly
disables the check.

regards,
dan carpenter

RE: [PATCH 5/5] staging: fsl-dpaa2/eth: Remove Rx frame size check

[Ioana Ciocoi Radulescu]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Monday, July 9, 2018 10:28 PM
> To: Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com>
> Cc: gregkh@linuxfoundation.org; devel@driverdev.osuosl.org; linux-
> kernel@vger.kernel.org; Ioana Ciornei <ioana.ciornei@nxp.com>
> Subject: Re: [PATCH 5/5] staging: fsl-dpaa2/eth: Remove Rx frame size check
> 
> On Mon, Jul 09, 2018 at 10:01:11AM -0500, Ioana Radulescu wrote:
> > @@ -2385,6 +2365,12 @@ static int netdev_init(struct net_device
> *net_dev)
> >
> >  	/* Set MTU upper limit; lower limit is 68B (default value) */
> >  	net_dev->max_mtu = DPAA2_ETH_MAX_MTU;
> > +	err = dpni_set_max_frame_length(priv->mc_io, 0, priv->mc_token,
> > +					(u16)DPAA2_ETH_MFL);
> 
> The cast was there in the original code so this is not a comment on this
> particular patch (which seems fine) but there is no need to cast.
> 
> Generally it's best to avoid unnecessary casts.  As a human reader, I
> find the cast confusing.  It indicates that DPAA2_ETH_MFL somehow
> requires special handling.  Perhaps it's negative or we are trying to
> truncate away the high bits.  But neither of those things really make
> sense.
> 
> From a static analysis perspective if DPAA2_ETH_MFL doesn't fit nicely
> then we would want to generate a warning.  But the cast explicitly
> disables the check.

I really don't remember why the cast was there in the first place.
It doesn't look like it's needed anymore, DPAA2_ETH_MFL has a
positive value (around 10K) that fits just fine inside a u16.

I see Greg already applied the patch, so I'll send a separate one to
remove the cast.

Thanks,
Ioana

Re: [PATCH 5/5] staging: fsl-dpaa2/eth: Remove Rx frame size check

[Dan Carpenter]

On Tue, Jul 10, 2018 at 03:55:32PM +0000, Ioana Ciocoi Radulescu wrote:
> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > Sent: Monday, July 9, 2018 10:28 PM
> > To: Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com>
> > Cc: gregkh@linuxfoundation.org; devel@driverdev.osuosl.org; linux-
> > kernel@vger.kernel.org; Ioana Ciornei <ioana.ciornei@nxp.com>
> > Subject: Re: [PATCH 5/5] staging: fsl-dpaa2/eth: Remove Rx frame size check
> > 
> > On Mon, Jul 09, 2018 at 10:01:11AM -0500, Ioana Radulescu wrote:
> > > @@ -2385,6 +2365,12 @@ static int netdev_init(struct net_device
> > *net_dev)
> > >
> > >  	/* Set MTU upper limit; lower limit is 68B (default value) */
> > >  	net_dev->max_mtu = DPAA2_ETH_MAX_MTU;
> > > +	err = dpni_set_max_frame_length(priv->mc_io, 0, priv->mc_token,
> > > +					(u16)DPAA2_ETH_MFL);
> > 
> > The cast was there in the original code so this is not a comment on this
> > particular patch (which seems fine) but there is no need to cast.
> > 
> > Generally it's best to avoid unnecessary casts.  As a human reader, I
> > find the cast confusing.  It indicates that DPAA2_ETH_MFL somehow
> > requires special handling.  Perhaps it's negative or we are trying to
> > truncate away the high bits.  But neither of those things really make
> > sense.
> > 
> > From a static analysis perspective if DPAA2_ETH_MFL doesn't fit nicely
> > then we would want to generate a warning.  But the cast explicitly
> > disables the check.
> 
> I really don't remember why the cast was there in the first place.
> It doesn't look like it's needed anymore, DPAA2_ETH_MFL has a
> positive value (around 10K) that fits just fine inside a u16.
> 
> I see Greg already applied the patch, so I'll send a separate one to
> remove the cast.

Yeah.  I wasn't saying redo the patch.  That was there in the original
and it's not like it's a bug or anything.

regards,
dan carpenter