Message-ID: <MW5PR11MB5811D84BB9769C3FE5D0AEA8DD5AA@MW5PR11MB5811.namprd11.prod.outlook.com>
Date: Wed, 14 Jun 2023 22:58:19 +0000
From: "Ertman, David M" <david.m.ertman@...el.com>
To: Brett Creeley <bcreeley@....com>, "intel-wired-lan@...ts.osuosl.org"
	<intel-wired-lan@...ts.osuosl.org>
CC: "daniel.machon@...rochip.com" <daniel.machon@...rochip.com>,
	"simon.horman@...igine.com" <simon.horman@...igine.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH iwl-next v4 04/10] ice: implement lag netdev event handler

> -----Original Message-----
> From: Brett Creeley <bcreeley@....com>
> Sent: Wednesday, June 14, 2023 2:24 PM
> To: Ertman, David M <david.m.ertman@...el.com>; intel-wired-
> lan@...ts.osuosl.org
> Cc: daniel.machon@...rochip.com; simon.horman@...igine.com;
> netdev@...r.kernel.org
> Subject: Re: [PATCH iwl-next v4 04/10] ice: implement lag netdev event
> handler
> 
> On 6/9/2023 2:16 PM, Dave Ertman wrote:
> > Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
> >
> >
> > The event handler for LAG will create a work item to place on the ordered
> > workqueue to be processed.
> >
> > Add in defines for training packets and new recipes to be used by the
> > switching block of the HW for LAG packet steering.
> >
> > Update the ice_lag struct to reflect the new processing methodology.
> >
> > Signed-off-by: Dave Ertman <david.m.ertman@...el.com>
> > ---
> >   drivers/net/ethernet/intel/ice/ice_lag.c | 125 ++++++++++++++++++++--
> -
> >   drivers/net/ethernet/intel/ice/ice_lag.h |  30 +++++-
> >   2 files changed, 141 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c
> b/drivers/net/ethernet/intel/ice/ice_lag.c
> > index 73bfc5cd8b37..529abfb904d0 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_lag.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_lag.c
> 
> [...]
> 
> > +/**
> > + * ice_lag_process_event - process a task assigned to the lag_wq
> > + * @work: pointer to work_struct
> > + */
> > +static void ice_lag_process_event(struct work_struct *work)
> > +{
> > +       struct netdev_notifier_changeupper_info *info;
> > +       struct ice_lag_work *lag_work;
> > +       struct net_device *netdev;
> > +       struct list_head *tmp, *n;
> > +       struct ice_pf *pf;
> > +
> > +       lag_work = container_of(work, struct ice_lag_work, lag_task);
> > +       pf = lag_work->lag->pf;
> > +
> > +       mutex_lock(&pf->lag_mutex);
> > +       lag_work->lag->netdev_head = &lag_work->netdev_list.node;
> > +
> > +       switch (lag_work->event) {
> > +       case NETDEV_CHANGEUPPER:
> > +               info = &lag_work->info.changeupper_info;
> > +               if (ice_is_feature_supported(pf, ICE_F_SRIOV_LAG))
> > +                       ice_lag_changeupper_event(lag_work->lag, info);
> > +               break;
> > +       case NETDEV_BONDING_INFO:
> > +               ice_lag_info_event(lag_work->lag, &lag_work-
> >info.bonding_info);
> > +               break;
> > +       case NETDEV_UNREGISTER:
> > +               if (ice_is_feature_supported(pf, ICE_F_SRIOV_LAG)) {
> > +                       netdev = lag_work->info.bonding_info.info.dev;
> > +                       if ((netdev == lag_work->lag->netdev ||
> > +                            lag_work->lag->primary) && lag_work->lag->bonded)
> > +                               ice_lag_unregister(lag_work->lag, netdev);
> > +               }
> > +               break;
> > +       default:
> > +               break;
> > +       }
> > +
> > +       /* cleanup resources allocated for this work item */
> > +       list_for_each_safe(tmp, n, &lag_work->netdev_list.node) {
> > +               struct ice_lag_netdev_list *entry;
> > +
> > +               entry = list_entry(tmp, struct ice_lag_netdev_list, node);
> > +               list_del(&entry->node);
> > +               kfree(entry);
> > +       }
> > +       lag_work->lag->netdev_head = NULL;
> > +
> > +       mutex_unlock(&pf->lag_mutex);
> > +
> > +       kfree(work);
> 
> Should this be freeing lag_work instead?

Nice catch!!!  You are right, lag_work is what is allocated not it's element work!

> 
> > +}
> > +
> >   /**
> >    * ice_lag_event_handler - handle LAG events from netdev
> >    * @notif_blk: notifier block registered by this netdev
> > @@ -299,31 +351,79 @@ ice_lag_event_handler(struct notifier_block
> *notif_blk, unsigned long event,
> >                        void *ptr)
> >   {
> >          struct net_device *netdev = netdev_notifier_info_to_dev(ptr);
> > +       struct net_device *upper_netdev;
> > +       struct ice_lag_work *lag_work;
> >          struct ice_lag *lag;
> >
> > -       lag = container_of(notif_blk, struct ice_lag, notif_block);
> > +       if (!netif_is_ice(netdev))
> > +               return NOTIFY_DONE;
> > +
> > +       if (event != NETDEV_CHANGEUPPER && event !=
> NETDEV_BONDING_INFO &&
> > +           event != NETDEV_UNREGISTER)
> > +               return NOTIFY_DONE;
> 
> Would it make more sense to prevent the work item and any related work
> if the ice_is_feature_supported(pf, ICE_F_SRIOV_LAG) check is moved to
> this function along with the events that require that feature?
> 
> Something like:
> 
> if ((event == NETDEV_CHANGEUPPER || event == NETDEV_UNREGISTER)
> &&
>       !ice_is_feature_supported(pf, ICE_F_SRIOV_LAG))
> 	return NOTIFY_DONE;
> 

Even if SRIOV is not supported, there are still tasks that need to be performed for bonding
events - e.g. unplug the RDMA aux devices, so we don't want to avoid creating a workqueue
entry when feature not supported. Which makes me notice that ice_lag_changeupper_event
is under a feature check and it should not be here.

Will change it.  Changes coming in v5.
DaveE

> >
> > +       if (!(netdev->priv_flags & IFF_BONDING))
> > +               return NOTIFY_DONE;
> > +

Powered by blists - more mailing lists