| Message ID | 20201106001220.68130-7-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 | warning | WARNING: Possible unnecessary 'out of memory' message |
| 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 Thu, 5 Nov 2020 16:12:18 -0800 Shannon Nelson wrote: > + work = kzalloc(sizeof(*work), GFP_ATOMIC); > + if (!work) { > + netdev_err(lif->netdev, "%s OOM\n", __func__); > + return; > + } Can you drop this message (can be a follow up, since you're just moving it). AFAICT ATOMIC doesn't imply NOWARN so the message is redundant no?
On 11/6/20 9:03 AM, Jakub Kicinski wrote: > On Thu, 5 Nov 2020 16:12:18 -0800 Shannon Nelson wrote: >> + work = kzalloc(sizeof(*work), GFP_ATOMIC); >> + if (!work) { >> + netdev_err(lif->netdev, "%s OOM\n", __func__); >> + return; >> + } > Can you drop this message (can be a follow up, since you're just moving > it). > > AFAICT ATOMIC doesn't imply NOWARN so the message is redundant no? Yes, this can probably be cleaned up. There are several of these left over from the very early version of this driver that I'd like to clean up, but haven't yet bubbled up high enough on my priority-vs-time list. I'll try to get to them in the next week or so. sln
On Thu, 2020-11-05 at 16:12 -0800, Shannon Nelson wrote: > The _ionic_lif_rx_mode() is only used once and really doesn't > need to be broken out. > > Signed-off-by: Shannon Nelson <snelson@pensando.io> > --- > .../net/ethernet/pensando/ionic/ionic_lif.c | 38 ++++++++--------- > -- > 1 file changed, 16 insertions(+), 22 deletions(-) > > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c > b/drivers/net/ethernet/pensando/ionic/ionic_lif.c > index a0d26fe4cbc3..ef092ee33e59 100644 > --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c > +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c > @@ -1129,29 +1129,10 @@ static void ionic_lif_rx_mode(struct > ionic_lif *lif, unsigned int rx_mode) > lif->rx_mode = rx_mode; > } > > -static void _ionic_lif_rx_mode(struct ionic_lif *lif, unsigned int > rx_mode, > - bool from_ndo) > -{ > - struct ionic_deferred_work *work; > - > - if (from_ndo) { > - work = kzalloc(sizeof(*work), GFP_ATOMIC); > - if (!work) { > - netdev_err(lif->netdev, "%s OOM\n", __func__); > - return; > - } > - work->type = IONIC_DW_TYPE_RX_MODE; > - work->rx_mode = rx_mode; > - netdev_dbg(lif->netdev, "deferred: rx_mode\n"); > - ionic_lif_deferred_enqueue(&lif->deferred, work); > - } else { > - ionic_lif_rx_mode(lif, rx_mode); > - } > -} > - > static void ionic_set_rx_mode(struct net_device *netdev, bool > from_ndo) > { > struct ionic_lif *lif = netdev_priv(netdev); > + struct ionic_deferred_work *work; > unsigned int nfilters; > unsigned int rx_mode; > > @@ -1197,8 +1178,21 @@ static void ionic_set_rx_mode(struct > net_device *netdev, bool from_ndo) > rx_mode &= ~IONIC_RX_MODE_F_ALLMULTI; > } > > - if (lif->rx_mode != rx_mode) > - _ionic_lif_rx_mode(lif, rx_mode, from_ndo); > + if (lif->rx_mode != rx_mode) { > + if (from_ndo) { > + work = kzalloc(sizeof(*work), GFP_ATOMIC); > + if (!work) { > + netdev_err(lif->netdev, "%s OOM\n", > __func__); > + return; > + } > + work->type = IONIC_DW_TYPE_RX_MODE; > + work->rx_mode = rx_mode; > + netdev_dbg(lif->netdev, "deferred: rx_mode\n"); > + ionic_lif_deferred_enqueue(&lif->deferred, > work); > + } else { > + ionic_lif_rx_mode(lif, rx_mode); > + } > + } > } You could move this logic one level up and totally eliminate the if condition ionic_set_rx_mode_needed() { //sync driver data base return lif->rx_mode != rx_mode; } ndo_set_rx_mode() { if (!ionic_set_rx_mode_needed()) return; // no change; schedule_work(set_rx_mode_hw); } none_ndo_set_rx_mode() { if (!ionic_set_rx_mode_needed()) return; // no change; set_rx_mode_hw(); } Future improvement: One more thing I've noticed about you current ionic_set_rx_mode() is that in case of from_ndo, when it syncs mac addresses it will schedule a deferred mac address update work to hw per address. which i think is an overkill, a simpler design which will totally eliminate the need for from_ndo flags, is to do similar to the above but with a minor change. ionic_set_rx_mode_needed() { // Just sync driver mac table here and update hw later // in one deferred work rather than scheduling multi work addr_changed = ionic_dev_uc_sync(); addr_changed |= ionic_dev_mc_sync(); rx_mode_changed = sync_driver_rx_mode(rx_mode); return rx_mode_changed || addr_changed; } /* might sleep */ set_rx_mode_hw() { commit_addr_change_to_hw(); commit_rx_mode_changes_to_hw(); } ndo_set_rx_mode() { if (!ionic_set_rx_mode_needed()) return; // no change; schedule_work(set_rx_mode_hw); } none_ndo_set_rx_mode() { if (!ionic_set_rx_mode_needed()) return; // no change; set_rx_mode_hw(); }
On 11/6/20 1:33 PM, Saeed Mahameed wrote: > On Thu, 2020-11-05 at 16:12 -0800, Shannon Nelson wrote: >> The _ionic_lif_rx_mode() is only used once and really doesn't >> need to be broken out. >> >> Signed-off-by: Shannon Nelson <snelson@pensando.io> >> --- >> .../net/ethernet/pensando/ionic/ionic_lif.c | 38 ++++++++--------- >> -- >> 1 file changed, 16 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c >> b/drivers/net/ethernet/pensando/ionic/ionic_lif.c >> index a0d26fe4cbc3..ef092ee33e59 100644 >> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c >> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c >> @@ -1129,29 +1129,10 @@ static void ionic_lif_rx_mode(struct >> ionic_lif *lif, unsigned int rx_mode) >> lif->rx_mode = rx_mode; >> } >> >> -static void _ionic_lif_rx_mode(struct ionic_lif *lif, unsigned int >> rx_mode, >> - bool from_ndo) >> -{ >> - struct ionic_deferred_work *work; >> - >> - if (from_ndo) { >> - work = kzalloc(sizeof(*work), GFP_ATOMIC); >> - if (!work) { >> - netdev_err(lif->netdev, "%s OOM\n", __func__); >> - return; >> - } >> - work->type = IONIC_DW_TYPE_RX_MODE; >> - work->rx_mode = rx_mode; >> - netdev_dbg(lif->netdev, "deferred: rx_mode\n"); >> - ionic_lif_deferred_enqueue(&lif->deferred, work); >> - } else { >> - ionic_lif_rx_mode(lif, rx_mode); >> - } >> -} >> - >> static void ionic_set_rx_mode(struct net_device *netdev, bool >> from_ndo) >> { >> struct ionic_lif *lif = netdev_priv(netdev); >> + struct ionic_deferred_work *work; >> unsigned int nfilters; >> unsigned int rx_mode; >> >> @@ -1197,8 +1178,21 @@ static void ionic_set_rx_mode(struct >> net_device *netdev, bool from_ndo) >> rx_mode &= ~IONIC_RX_MODE_F_ALLMULTI; >> } >> >> - if (lif->rx_mode != rx_mode) >> - _ionic_lif_rx_mode(lif, rx_mode, from_ndo); >> + if (lif->rx_mode != rx_mode) { >> + if (from_ndo) { >> + work = kzalloc(sizeof(*work), GFP_ATOMIC); >> + if (!work) { >> + netdev_err(lif->netdev, "%s OOM\n", >> __func__); >> + return; >> + } >> + work->type = IONIC_DW_TYPE_RX_MODE; >> + work->rx_mode = rx_mode; >> + netdev_dbg(lif->netdev, "deferred: rx_mode\n"); >> + ionic_lif_deferred_enqueue(&lif->deferred, >> work); >> + } else { >> + ionic_lif_rx_mode(lif, rx_mode); >> + } >> + } >> } > You could move this logic one level up and totally eliminate the if > condition > > ionic_set_rx_mode_needed() { > //sync driver data base > return lif->rx_mode != rx_mode; > } > > ndo_set_rx_mode() { > if (!ionic_set_rx_mode_needed()) > return; // no change; > schedule_work(set_rx_mode_hw); > } > > none_ndo_set_rx_mode() { > if (!ionic_set_rx_mode_needed()) > return; // no change; > set_rx_mode_hw(); > } Hmm... yes, that's possible, but I like keeping that bit of logic together with the rest in the main set_rx_mode block. > Future improvement: > > One more thing I've noticed about you current ionic_set_rx_mode() > is that in case of from_ndo, when it syncs mac addresses it will > schedule a deferred mac address update work to hw per address. which i > think is an overkill, This is much less of an issue with the recent change in ionic_lif_deferred_work() to run through the whole work list in one deferred_work session. sln > a simpler design which will totally eliminate the > need for from_ndo flags, is to do similar to the above but with a minor > change. > > ionic_set_rx_mode_needed() { > // Just sync driver mac table here and update hw later > // in one deferred work rather than scheduling multi work > addr_changed = ionic_dev_uc_sync(); > addr_changed |= ionic_dev_mc_sync(); > rx_mode_changed = sync_driver_rx_mode(rx_mode); > > return rx_mode_changed || addr_changed; > } > > /* might sleep */ > set_rx_mode_hw() { > commit_addr_change_to_hw(); > commit_rx_mode_changes_to_hw(); > } > > ndo_set_rx_mode() { > if (!ionic_set_rx_mode_needed()) > return; // no change; > schedule_work(set_rx_mode_hw); > } > > none_ndo_set_rx_mode() { > if (!ionic_set_rx_mode_needed()) > return; // no change; > set_rx_mode_hw(); > } >
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c index a0d26fe4cbc3..ef092ee33e59 100644 --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c @@ -1129,29 +1129,10 @@ static void ionic_lif_rx_mode(struct ionic_lif *lif, unsigned int rx_mode) lif->rx_mode = rx_mode; } -static void _ionic_lif_rx_mode(struct ionic_lif *lif, unsigned int rx_mode, - bool from_ndo) -{ - struct ionic_deferred_work *work; - - if (from_ndo) { - work = kzalloc(sizeof(*work), GFP_ATOMIC); - if (!work) { - netdev_err(lif->netdev, "%s OOM\n", __func__); - return; - } - work->type = IONIC_DW_TYPE_RX_MODE; - work->rx_mode = rx_mode; - netdev_dbg(lif->netdev, "deferred: rx_mode\n"); - ionic_lif_deferred_enqueue(&lif->deferred, work); - } else { - ionic_lif_rx_mode(lif, rx_mode); - } -} - static void ionic_set_rx_mode(struct net_device *netdev, bool from_ndo) { struct ionic_lif *lif = netdev_priv(netdev); + struct ionic_deferred_work *work; unsigned int nfilters; unsigned int rx_mode; @@ -1197,8 +1178,21 @@ static void ionic_set_rx_mode(struct net_device *netdev, bool from_ndo) rx_mode &= ~IONIC_RX_MODE_F_ALLMULTI; } - if (lif->rx_mode != rx_mode) - _ionic_lif_rx_mode(lif, rx_mode, from_ndo); + if (lif->rx_mode != rx_mode) { + if (from_ndo) { + work = kzalloc(sizeof(*work), GFP_ATOMIC); + if (!work) { + netdev_err(lif->netdev, "%s OOM\n", __func__); + return; + } + work->type = IONIC_DW_TYPE_RX_MODE; + work->rx_mode = rx_mode; + netdev_dbg(lif->netdev, "deferred: rx_mode\n"); + ionic_lif_deferred_enqueue(&lif->deferred, work); + } else { + ionic_lif_rx_mode(lif, rx_mode); + } + } } static void ionic_ndo_set_rx_mode(struct net_device *netdev)
The _ionic_lif_rx_mode() is only used once and really doesn't need to be broken out. Signed-off-by: Shannon Nelson <snelson@pensando.io> --- .../net/ethernet/pensando/ionic/ionic_lif.c | 38 ++++++++----------- 1 file changed, 16 insertions(+), 22 deletions(-)