| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <12e374aa-58a5-47a4-b54e-dcfc2f93e58f@amd.com>
Date: Wed, 14 Jun 2023 16:18:49 -0700
From: Brett Creeley <bcreeley@....com>
To: "Ertman, David M" <david.m.ertman@...el.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
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@....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
Ah, okay makes sense. Thanks for the explanation.
>
>>>
>>> + if (!(netdev->priv_flags & IFF_BONDING))
>>> + return NOTIFY_DONE;
>>> +
>
Powered by blists - more mailing lists