Re: [PATCH v2 net-next 6/8] ionic: flatten calls to ionic_lif_rx_mode

[Shannon Nelson]

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 <snel...@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();
}