From: Przemek Kitszel <przemyslaw.kitszel@intel•com>
To: Maciej Fijalkowski <maciej.fijalkowski@intel•com>
Cc: Tony Nguyen <anthony.l.nguyen@intel•com>, <davem@davemloft•net>,
<kuba@kernel•org>, <pabeni@redhat•com>, <edumazet@google•com>,
<netdev@vger•kernel.org>,
Piotr Gardocki <piotrx.gardocki@intel•com>,
"Michal Swiatkowski" <michal.swiatkowski@linux•intel.com>,
Rafal Romanowski <rafal.romanowski@intel•com>
Subject: Re: [PATCH net-next 2/3] iavf: fix err handling for MAC replace
Date: Tue, 6 Jun 2023 13:59:41 +0200 [thread overview]
Message-ID: <9cfeffae-e0f8-0cd1-2f62-b28d3227869c@intel.com> (raw)
In-Reply-To: <ZH8JBgiZAvNdfg4+@boxer>
On 6/6/23 12:23, Maciej Fijalkowski wrote:
> On Tue, Jun 06, 2023 at 12:14:49PM +0200, Przemek Kitszel wrote:
>> On 6/5/23 21:17, Maciej Fijalkowski wrote:
>>> On Fri, Jun 02, 2023 at 10:13:01AM -0700, Tony Nguyen wrote:
>>>> From: Przemek Kitszel <przemyslaw.kitszel@intel•com>
>>>>
>>>> Defer removal of current primary MAC until a replacement is successfully added.
>>>> Previous implementation would left filter list with no primary MAC.
>>>
>>> and this opens up for what kind of issues? do you mean that
>>> iavf_add_filter() could break and existing primary filter has been marked
>>> for removal?
>>
>> Yes, prior to the patch the flow was:
>> 1. mark all MACs non-primary;
>> 2. mark current HW MAC for removal;
>> 3. try to add new MAC, say it fails, so that's an end with -ENOMEM;
>> 4. ::is_primary and ::remove fields for the ::mac_filter_list, alongside
>> with ::aq_required are left modified, to be finalized next time
>> user/watchdog processes that.
>>
>> For me it was enough to treat it as a bug, and for sure a "bad smell".
>
> Thanks,
> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel•com>
>
>>
>>
>>>
>>>> This was found while reading the code.
>>>>
>>>> The patch takes advantage of the fact that there can only be a single primary
>>>> MAC filter at any time.
>>>>
>>>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel•com>
>>>> Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel•com>
>>>> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux•intel.com>
>>>> Tested-by: Rafal Romanowski <rafal.romanowski@intel•com>
>>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel•com>
>>>> ---
>>>> drivers/net/ethernet/intel/iavf/iavf_main.c | 42 ++++++++++-----------
>>>> 1 file changed, 19 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
>>>> index 420aaca548a0..3a78f86ba4f9 100644
>>>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
>>>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
>>>> @@ -1010,40 +1010,36 @@ int iavf_replace_primary_mac(struct iavf_adapter *adapter,
>>>
>>> from what i'm looking at, iavf_replace_primary_mac() could be scoped only
>>> to iavf_main.c and become static func.
>>>
>>
>> makes sense, thanks
>
> are you going to followup on this? probably there are some more low
> hanging fruits out in iavf such as this one.
Sure, after some digging it looks like static-whats-possible warrants
another patch ;) (will send soon as separate series)
>
>>
>>>> const u8 *new_mac)
>>>> {
>>>> struct iavf_hw *hw = &adapter->hw;
>>>> - struct iavf_mac_filter *f;
>>>> + struct iavf_mac_filter *new_f;
>>>> + struct iavf_mac_filter *old_f;
>>>> spin_lock_bh(&adapter->mac_vlan_list_lock);
>>>> - list_for_each_entry(f, &adapter->mac_filter_list, list) {
>>>> - f->is_primary = false;
>>>> + new_f = iavf_add_filter(adapter, new_mac);
>>>> + if (!new_f) {
>>>> + spin_unlock_bh(&adapter->mac_vlan_list_lock);
>>>> + return -ENOMEM;
>>>> }
>>>> - f = iavf_find_filter(adapter, hw->mac.addr);
>>>> - if (f) {
>>>> - f->remove = true;
>>>> + old_f = iavf_find_filter(adapter, hw->mac.addr);
>>>> + if (old_f) {
>>>> + old_f->is_primary = false;
>>>> + old_f->remove = true;
>>>> adapter->aq_required |= IAVF_FLAG_AQ_DEL_MAC_FILTER;
>>>> }
>>>> -
>>>> - f = iavf_add_filter(adapter, new_mac);
>>>> -
>>>> - if (f) {
>>>> - /* Always send the request to add if changing primary MAC
>>>> - * even if filter is already present on the list
>>>> - */
>>>> - f->is_primary = true;
>>>> - f->add = true;
>>>> - adapter->aq_required |= IAVF_FLAG_AQ_ADD_MAC_FILTER;
>>>> - ether_addr_copy(hw->mac.addr, new_mac);
>>>> - }
>>>> + /* Always send the request to add if changing primary MAC,
>>>> + * even if filter is already present on the list
>>>> + */
>>>> + new_f->is_primary = true;
>>>> + new_f->add = true;
>>>> + adapter->aq_required |= IAVF_FLAG_AQ_ADD_MAC_FILTER;
>>>> + ether_addr_copy(hw->mac.addr, new_mac);
>>>> spin_unlock_bh(&adapter->mac_vlan_list_lock);
>>>> /* schedule the watchdog task to immediately process the request */
>>>> - if (f) {
>>>> - mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
>>>> - return 0;
>>>> - }
>>>> - return -ENOMEM;
>>>> + mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
>>>> + return 0;
>>>> }
>>>> /**
>>>> --
>>>> 2.38.1
>>>>
>>>>
>>
next prev parent reply other threads:[~2023-06-06 12:01 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-02 17:12 [PATCH net-next 0/3][pull request] Intel Wired LAN Driver Updates 2023-06-02 (iavf) Tony Nguyen
2023-06-02 17:13 ` [PATCH net-next 1/3] iavf: add check for current MAC address in set_mac callback Tony Nguyen
2023-06-03 14:06 ` Simon Horman
2023-06-05 19:02 ` Maciej Fijalkowski
2023-06-06 9:22 ` Piotr Gardocki
2023-06-06 10:21 ` Maciej Fijalkowski
2023-06-06 12:54 ` Alexander Lobakin
2023-06-06 17:24 ` Jakub Kicinski
2023-06-07 10:29 ` Piotr Gardocki
2023-06-07 16:38 ` Jakub Kicinski
2023-06-07 20:22 ` Maciej Fijalkowski
2023-06-02 17:13 ` [PATCH net-next 2/3] iavf: fix err handling for MAC replace Tony Nguyen
2023-06-03 14:07 ` Simon Horman
2023-06-05 19:17 ` Maciej Fijalkowski
2023-06-06 10:14 ` Przemek Kitszel
2023-06-06 10:23 ` Maciej Fijalkowski
2023-06-06 11:59 ` Przemek Kitszel [this message]
2023-06-07 13:57 ` Przemek Kitszel
2023-06-07 19:08 ` Fijalkowski, Maciej
2023-06-16 7:09 ` Przemek Kitszel
2023-06-16 17:13 ` Tony Nguyen
2023-06-02 17:13 ` [PATCH net-next 3/3] iavf: remove mask from iavf_irq_enable_queues() Tony Nguyen
2023-06-03 14:07 ` Simon Horman
2023-06-05 19:25 ` Maciej Fijalkowski
2023-06-05 19:56 ` Ahmed Zaki
2023-06-06 10:26 ` Maciej Fijalkowski
2023-06-06 15:23 ` Ahmed Zaki
2023-06-06 16:01 ` Romanowski, Rafal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9cfeffae-e0f8-0cd1-2f62-b28d3227869c@intel.com \
--to=przemyslaw.kitszel@intel$(echo .)com \
--cc=anthony.l.nguyen@intel$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=edumazet@google$(echo .)com \
--cc=kuba@kernel$(echo .)org \
--cc=maciej.fijalkowski@intel$(echo .)com \
--cc=michal.swiatkowski@linux$(echo .)intel.com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
--cc=piotrx.gardocki@intel$(echo .)com \
--cc=rafal.romanowski@intel$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox