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

[Saeed Mahameed <saeed@kernel.org>]

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