| Message ID | 20201104223354.63856-6-snelson@pensando.io |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Driver Reviewers |
| Headers | show |
| Series | ionic updates | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | success | Link |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Clearly marked for net-next |
| jkicinski/subject_prefix | success | Link |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | success | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 22 lines checked |
| jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
On Wed, 2020-11-04 at 14:33 -0800, Shannon Nelson wrote: > We should be using the multicast sync routines for the > multicast filters. > > Fixes: 1800eee16676 ("net: ionic: Replace in_interrupt() usage.") > Signed-off-by: Shannon Nelson <snelson@pensando.io> > --- > drivers/net/ethernet/pensando/ionic/ionic_lif.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c > b/drivers/net/ethernet/pensando/ionic/ionic_lif.c > index 28044240caf2..a58bb572b23b 100644 > --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c > +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c > @@ -1158,6 +1158,14 @@ static void ionic_dev_uc_sync(struct > net_device *netdev, bool from_ndo) > > } > > +static void ionic_dev_mc_sync(struct net_device *netdev, bool > from_ndo) > +{ > + if (from_ndo) > + __dev_mc_sync(netdev, ionic_ndo_addr_add, > ionic_ndo_addr_del); > + else > + __dev_mc_sync(netdev, ionic_addr_add, ionic_addr_del); > +} > + I don't see any point of this function since it is used in one place. just unfold it in the caller and you will endup with less code. also keep in mind passing boolean to functions is usually a bad idea, and only complicates things, keep things simple and explicit, let the caller do what is necessary to be done, so if you must do this if condition, do it at the caller level. and for a future patch i strongly recommend to remove this from_ndo flag, it is really straight forward to do for this function 1) you can just pass _addr_add/del function pointers directly to ionic_set_rx_mode 2) remove _ionic_lif_rx_mode from ionic_set_rx_mode and unfold it in the caller since the function is basically one giant if condition which is called only from two places. > static void ionic_set_rx_mode(struct net_device *netdev, bool > from_ndo) > { > struct ionic_lif *lif = netdev_priv(netdev); > @@ -1189,7 +1197,7 @@ static void ionic_set_rx_mode(struct net_device > *netdev, bool from_ndo) > } > > /* same for multicast */ > - ionic_dev_uc_sync(netdev, from_ndo); > + ionic_dev_mc_sync(netdev, from_ndo); > nfilters = le32_to_cpu(lif->identity->eth.max_mcast_filters); > if (netdev_mc_count(netdev) > nfilters) { > rx_mode |= IONIC_RX_MODE_F_ALLMULTI;
On 11/4/20 5:18 PM, Saeed Mahameed wrote: > On Wed, 2020-11-04 at 14:33 -0800, Shannon Nelson wrote: >> We should be using the multicast sync routines for the >> multicast filters. >> >> Fixes: 1800eee16676 ("net: ionic: Replace in_interrupt() usage.") >> Signed-off-by: Shannon Nelson <snelson@pensando.io> >> --- >> drivers/net/ethernet/pensando/ionic/ionic_lif.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c >> b/drivers/net/ethernet/pensando/ionic/ionic_lif.c >> index 28044240caf2..a58bb572b23b 100644 >> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c >> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c >> @@ -1158,6 +1158,14 @@ static void ionic_dev_uc_sync(struct >> net_device *netdev, bool from_ndo) >> >> } >> >> +static void ionic_dev_mc_sync(struct net_device *netdev, bool >> from_ndo) >> +{ >> + if (from_ndo) >> + __dev_mc_sync(netdev, ionic_ndo_addr_add, >> ionic_ndo_addr_del); >> + else >> + __dev_mc_sync(netdev, ionic_addr_add, ionic_addr_del); >> +} >> + > I don't see any point of this function since it is used in one place. > just unfold it in the caller and you will endup with less code. > > also keep in mind passing boolean to functions is usually a bad idea, > and only complicates things, keep things simple and explicit, let the > caller do what is necessary to be done, so if you must do this if > condition, do it at the caller level. > > and for a future patch i strongly recommend to remove this from_ndo > flag, it is really straight forward to do for this function > 1) you can just pass _addr_add/del function pointers directly > to ionic_set_rx_mode > 2) remove _ionic_lif_rx_mode from ionic_set_rx_mode and unfold it in > the caller since the function is basically one giant if condition which > is called only from two places. This was specifically following work that was done a couple of weeks ago by Thomas Gleixner et al to clean up questionable uses of in_interrupt(), similar to how they used booleans to patch mlx5 and other drivers. They split this out, but later I noticed this issue with how multicast got handled. I agree, I'm not thrilled with the new booleans either, which is part of the reason for patch 6/6 in this series. Yes, I can pull these back into ionic_set_rx_mode() for a v2 patch, which will clean up some of this. I'll look at those future patch ideas: (2) is easy enough and I might just add it to this patch series, but I'm not sure about (1) yet, partly because I like the current separation of knowledge. Thanks, sln > >> static void ionic_set_rx_mode(struct net_device *netdev, bool >> from_ndo) >> { >> struct ionic_lif *lif = netdev_priv(netdev); >> @@ -1189,7 +1197,7 @@ static void ionic_set_rx_mode(struct net_device >> *netdev, bool from_ndo) >> } >> >> /* same for multicast */ >> - ionic_dev_uc_sync(netdev, from_ndo); >> + ionic_dev_mc_sync(netdev, from_ndo); >> nfilters = le32_to_cpu(lif->identity->eth.max_mcast_filters); >> if (netdev_mc_count(netdev) > nfilters) { >> rx_mode |= IONIC_RX_MODE_F_ALLMULTI;
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c index 28044240caf2..a58bb572b23b 100644 --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c @@ -1158,6 +1158,14 @@ static void ionic_dev_uc_sync(struct net_device *netdev, bool from_ndo) } +static void ionic_dev_mc_sync(struct net_device *netdev, bool from_ndo) +{ + if (from_ndo) + __dev_mc_sync(netdev, ionic_ndo_addr_add, ionic_ndo_addr_del); + else + __dev_mc_sync(netdev, ionic_addr_add, ionic_addr_del); +} + static void ionic_set_rx_mode(struct net_device *netdev, bool from_ndo) { struct ionic_lif *lif = netdev_priv(netdev); @@ -1189,7 +1197,7 @@ static void ionic_set_rx_mode(struct net_device *netdev, bool from_ndo) } /* same for multicast */ - ionic_dev_uc_sync(netdev, from_ndo); + ionic_dev_mc_sync(netdev, from_ndo); nfilters = le32_to_cpu(lif->identity->eth.max_mcast_filters); if (netdev_mc_count(netdev) > nfilters) { rx_mode |= IONIC_RX_MODE_F_ALLMULTI;
We should be using the multicast sync routines for the multicast filters. Fixes: 1800eee16676 ("net: ionic: Replace in_interrupt() usage.") Signed-off-by: Shannon Nelson <snelson@pensando.io> --- drivers/net/ethernet/pensando/ionic/ionic_lif.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)