linux-kernel - Re: [Intel-wired-lan] [PATCH net 1/2] iavf: add iavf_schedule_aq

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <c4838e77-5688-10a0-8a3e-cf1dda12237c@intel.com>
Date:   Thu, 7 Sep 2023 06:42:17 -0600
From:   Ahmed Zaki <ahmed.zaki@...el.com>
To:     <poros@...hat.com>, <netdev@...r.kernel.org>
CC:     <ivecera@...hat.com>, <intel-wired-lan@...ts.osuosl.org>,
        <jesse.brandeburg@...el.com>, <linux-kernel@...r.kernel.org>,
        <edumazet@...gle.com>, <anthony.l.nguyen@...el.com>,
        <kuba@...nel.org>, <pabeni@...hat.com>, <davem@...emloft.net>
Subject: Re: [Intel-wired-lan] [PATCH net 1/2] iavf: add
 iavf_schedule_aq_request() helper


On 2023-09-07 01:01, Petr Oros wrote:
> Ahmed Zaki píše v St 06. 09. 2023 v 09:32 -0600:
>> On 2023-09-06 08:14, Petr Oros wrote:
>>> Add helper for set iavf aq request AVF_FLAG_AQ_* and imediately
>>> schedule watchdog_task. Helper will be used in cases where it is
>>> necessary to run aq requests asap
>>>
>>> Signed-off-by: Petr Oros <poros@...hat.com>
>>> Co-developed-by: Michal Schmidt <mschmidt@...hat.com>
>>> Signed-off-by: Michal Schmidt <mschmidt@...hat.com>
>>> Co-developed-by: Ivan Vecera <ivecera@...hat.com>
>>> Signed-off-by: Ivan Vecera <ivecera@...hat.com>
>>> ---
>>>    drivers/net/ethernet/intel/iavf/iavf.h         |  2 +-
>>>    drivers/net/ethernet/intel/iavf/iavf_ethtool.c |  2 +-
>>>    drivers/net/ethernet/intel/iavf/iavf_main.c    | 10 ++++------
>>>    3 files changed, 6 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h
>>> b/drivers/net/ethernet/intel/iavf/iavf.h
>>> index 85fba85fbb232b..e110ba3461857b 100644
>>> --- a/drivers/net/ethernet/intel/iavf/iavf.h
>>> +++ b/drivers/net/ethernet/intel/iavf/iavf.h
>>> @@ -521,7 +521,7 @@ void iavf_down(struct iavf_adapter *adapter);
>>>    int iavf_process_config(struct iavf_adapter *adapter);
>>>    int iavf_parse_vf_resource_msg(struct iavf_adapter *adapter);
>>>    void iavf_schedule_reset(struct iavf_adapter *adapter, u64
>>> flags);
>>> -void iavf_schedule_request_stats(struct iavf_adapter *adapter);
>>> +void iavf_schedule_aq_request(struct iavf_adapter *adapter, u64
>>> flags);
>>>    void iavf_schedule_finish_config(struct iavf_adapter *adapter);
>>>    void iavf_reset(struct iavf_adapter *adapter);
>>>    void iavf_set_ethtool_ops(struct net_device *netdev);
>>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
>>> b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
>>> index a34303ad057d00..90397293525f71 100644
>>> --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
>>> +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
>>> @@ -362,7 +362,7 @@ static void iavf_get_ethtool_stats(struct
>>> net_device *netdev,
>>>          unsigned int i;
>>>    
>>>          /* Explicitly request stats refresh */
>>> -       iavf_schedule_request_stats(adapter);
>>> +       iavf_schedule_aq_request(adapter,
>>> IAVF_FLAG_AQ_REQUEST_STATS);
>>>    
>>>          iavf_add_ethtool_stats(&data, adapter,
>>> iavf_gstrings_stats);
>>>    
>>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
>>> b/drivers/net/ethernet/intel/iavf/iavf_main.c
>>> index 7b300c86ceda73..86d472dfdbc10c 100644
>>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
>>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
>>> @@ -314,15 +314,13 @@ void iavf_schedule_reset(struct iavf_adapter
>>> *adapter, u64 flags)
>>>    }
>>>    
>>>    /**
>>> - * iavf_schedule_request_stats - Set the flags and schedule
>>> statistics request
>>> + * iavf_schedule_aq_request - Set the flags and schedule aq
>>> request
>>>     * @adapter: board private structure
>>> - *
>>> - * Sets IAVF_FLAG_AQ_REQUEST_STATS flag so iavf_watchdog_task()
>>> will explicitly
>>> - * request and refresh ethtool stats
>>> + * @flags: requested aq flags
>>>     **/
>>> -void iavf_schedule_request_stats(struct iavf_adapter *adapter)
>>> +void iavf_schedule_aq_request(struct iavf_adapter *adapter, u64
>>> flags)
>>>    {
>>> -       adapter->aq_required |= IAVF_FLAG_AQ_REQUEST_STATS;
>>> +       adapter->aq_required |= flags;
>>>          mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
>>>    }
>>>    
>> There are other places where the helper can be used without
>> functional
>> changes, e.g. iavf_add_fdir_ethtool() , iavf_replace_primary_mac()
>> and
>> couple of other places. In all of them, mod_delayed_work() is called
>> after setting the AQ flag. For the sake of consistency, can you use
>> the
>> helper there too?
> These two commits is fixes for issue -> net. But on
> iavf_add_fdir_ethtool and iavf_replace_primary_mac is mod_delayed_work
> called after spin_unlock_bh ->
> looks like no functional chages but i would like be sure and better
> will send this to net-next. Are you ok with this?
>
It is usually better to use the helper in the same commit that 
introduces it, but no problem. I am OK with sending this later to next.

Thanks,

Ahmed