RE: [PATCH 5/5] staging: fsl-dpaa2/eth: Remove Rx frame size check
> -Original Message- > From: Dan Carpenter [mailto:dan.carpen...@oracle.com] > Sent: Monday, July 9, 2018 10:28 PM > To: Ioana Ciocoi Radulescu > Cc: gre...@linuxfoundation.org; de...@driverdev.osuosl.org; linux- > ker...@vger.kernel.org; Ioana Ciornei > 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 5/5] staging: fsl-dpaa2/eth: Remove Rx frame size check
On Tue, Jul 10, 2018 at 03:55:32PM +, Ioana Ciocoi Radulescu wrote: > > -Original Message- > > From: Dan Carpenter [mailto:dan.carpen...@oracle.com] > > Sent: Monday, July 9, 2018 10:28 PM > > To: Ioana Ciocoi Radulescu > > Cc: gre...@linuxfoundation.org; de...@driverdev.osuosl.org; linux- > > ker...@vger.kernel.org; Ioana Ciornei > > 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 5/5] staging: fsl-dpaa2/eth: Remove Rx frame size check
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
---
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
