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

[Wojciech Drewek]

On 17.05.2024 09:26, Naveen Mamindlapalli wrote:
> 
>> -----Original Message-----
>> From: Wojciech Drewek <wojciech.drewek at intel.com>
>> Sent: Thursday, May 16, 2024 7:34 PM
>> To: netdev at vger.kernel.org
>> Cc: intel-wired-lan at lists.osuosl.org
>> Subject: [PATCH iwl-net] ice: implement AQ download pkg retry
>>
>> 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 at linux.intel.com>
>> Signed-off-by: Wojciech Drewek <wojciech.drewek at 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)
>> +				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",
>> +				try_cnt);
> 
> Do you really need this dbg statement when try_cnt < 5? Is it not misleading in success case (with retries)?

Agree. I'm going to change it to "ice_aq_download_pkg number of retries: %d" as Brett suggested.

> 
> Thanks,
> Naveen
> 
>>
>>  		/* Save AQ status from download package */
>>  		if (status) {
>> --
>> 2.40.1
>>
>