diff mbox series

[net,6/8] ice: implement AQ download pkg retry

Message ID 20240528-net-2024-05-28-intel-net-fixes-v1-6-dc8593d2bbc6@intel.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Intel Wired LAN Driver Updates 2024-05-28 (e1000e, i40e, ice) | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 45 this patch: 45
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers fail 1 blamed authors not CCed: anthony.l.nguyen@intel.com; 5 maintainers not CCed: pabeni@redhat.com edumazet@google.com anthony.l.nguyen@intel.com jesse.brandeburg@intel.com intel-wired-lan@lists.osuosl.org
netdev/build_clang fail Errors and warnings before: 36 this patch: 36
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn fail Errors and warnings before: 45 this patch: 45
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 31 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 142 this patch: 142
netdev/source_inline success Was 0 now: 0

Commit Message

Jacob Keller May 28, 2024, 10:06 p.m. UTC 
From: Wojciech Drewek <wojciech.drewek@intel.com>

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.

Fixes: c76488109616 ("ice: Implement Dynamic Device Personalization (DDP) download")
Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
Reviewed-by: Brett Creeley <brett.creeley@amd.com>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ddp.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

Comments

Jakub Kicinski May 30, 2024, 1:51 a.m. UTC | #1 
On Tue, 28 May 2024 15:06:09 -0700 Jacob Keller wrote:
> +		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 number of retries: %d\n",
> +				try_cnt);
>  

That's not a great wait loop. Last iteration sleeps for 20msec and then
gives up without checking the condition.
Jacob Keller May 30, 2024, 4:50 p.m. UTC | #2 
On 5/29/2024 6:51 PM, Jakub Kicinski wrote:
> On Tue, 28 May 2024 15:06:09 -0700 Jacob Keller wrote:
>> +		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 number of retries: %d\n",
>> +				try_cnt);
>>  
> 
> That's not a great wait loop. Last iteration sleeps for 20msec and then
> gives up without checking the condition.

Yea, that seems rather silly.

@Wojciech, would you please look into this?
Wojciech Drewek May 31, 2024, 8:25 a.m. UTC | #3 
On 30.05.2024 18:50, Jacob Keller wrote:
> 
> 
> On 5/29/2024 6:51 PM, Jakub Kicinski wrote:
>> On Tue, 28 May 2024 15:06:09 -0700 Jacob Keller wrote:
>>> +		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 number of retries: %d\n",
>>> +				try_cnt);
>>>  
>>
>> That's not a great wait loop. Last iteration sleeps for 20msec and then
>> gives up without checking the condition.
> 
> Yea, that seems rather silly.
> 
> @Wojciech, would you please look into this?

Sure, I'll send next version
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c
index ce5034ed2b24..77b81e5a5a44 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 number of retries: %d\n",
+				try_cnt);
 
 		/* Save AQ status from download package */
 		if (status) {