Re: [PATCH iwl-next v4 04/10] ice: implement lag netdev event handler

[Brett Creeley <bcreeley@amd.com>]

On 6/14/2023 3:58 PM, Ertman, David M wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
>> -----Original Message-----
>> From: Brett Creeley <bcreeley@amd.com>
>> Sent: Wednesday, June 14, 2023 2:24 PM
>> To: Ertman, David M <david.m.ertman@intel.com>; intel-wired-
>> lan@lists.osuosl.org
>> Cc: daniel.machon@microchip.com; simon.horman@corigine.com;
>> netdev@vger.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@intel.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

Ah, okay makes sense. Thanks for the explanation.

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