Re: [Intel-wired-lan] [PATCH iwl-net] ice: implement AQ download pkg retry

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 5/17/24 09:49, Wojciech Drewek wrote:
> 
> 
> On 16.05.2024 18:36, Brett Creeley wrote:
>>
>>
>> On 5/16/2024 7:04 AM, Wojciech Drewek wrote:
>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>
>>>
>>> ice_aqc_opc_download_pkg (0x0C40) AQ sporadically returns error due
>>> to FW issue. Fix this by retrying five times before moving to
>>> Safe Mode.
>>>
>>> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
>>> Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
>>> ---
>>>    drivers/net/ethernet/intel/ice/ice_ddp.c | 19 +++++++++++++++++--
>>>    1 file changed, 17 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c
>>> index ce5034ed2b24..19e2111fcf08 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_ddp.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
>>> @@ -1339,6 +1339,7 @@ ice_dwnld_cfg_bufs_no_lock(struct ice_hw *hw, struct ice_buf *bufs, u32 start,
>>>
>>>           for (i = 0; i < count; i++) {
>>>                   bool last = false;
>>> +               int try_cnt = 0;
>>>                   int status;
>>>
>>>                   bh = (struct ice_buf_hdr *)(bufs + start + i);
>>> @@ -1346,8 +1347,22 @@ ice_dwnld_cfg_bufs_no_lock(struct ice_hw *hw, struct ice_buf *bufs, u32 start,
>>>                   if (indicate_last)
>>>                           last = ice_is_last_download_buffer(bh, i, count);
>>>
>>> -               status = ice_aq_download_pkg(hw, bh, ICE_PKG_BUF_SIZE, last,
>>> -                                            &offset, &info, NULL);
>>> +               while (try_cnt < 5) {
>>> +                       status = ice_aq_download_pkg(hw, bh, ICE_PKG_BUF_SIZE,
>>> +                                                    last, &offset, &info,
>>> +                                                    NULL);
>>> +                       if (hw->adminq.sq_last_status != ICE_AQ_RC_ENOSEC &&
>>> +                           hw->adminq.sq_last_status != ICE_AQ_RC_EBADSIG)
>>
>> Are these the only 2 sporadic errors that FW will return?
> 
> Yes, that's right. We don't want to retry in case of other errors since those might be valid.

I would say that those are the only two non-sporadic errors ;)

> 
>>
>>> +                               break;
>>> +
>>> +                       try_cnt++;
>>> +                       msleep(20);
>>> +               }
>>> +
>>> +               if (try_cnt)
>>> +                       dev_dbg(ice_hw_to_dev(hw),
>>> +                               "ice_aq_download_pkg failed, number of retries: %d\n",

s/retries/attempts/
(as retries = attempts + 1 ;))

>>> +                               try_cnt);
>>
>> If try_cnt is non-zero it doesn't mean the last download failed, it just means one or more attempts to download failed right? Maybe just "ice_aq_download_pkg number of retries: %d" since the if (status) check below will print on failure?
> 
> Sounds reasonable, we want this log only because we want to know if we hit this sporadic failure.
> 
>>
>>>
>>>                   /* Save AQ status from download package */
>>>                   if (status) {
>>> -- 
>>> 2.40.1
>>>
>>>