Re: [Intel-wired-lan] [PATCH net 1/2] iavf: add iavf_schedule_aq_request() helper

[Ahmed Zaki <ahmed.zaki@intel.com>]

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@redhat.com>
>>> Co-developed-by: Michal Schmidt <mschmidt@redhat.com>
>>> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
>>> Co-developed-by: Ivan Vecera <ivecera@redhat.com>
>>> Signed-off-by: Ivan Vecera <ivecera@redhat.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