Re: [Intel-wired-lan] [PATCH iwl-net 1/2] ice: reintroduce retry mechanism for indirect AQ

[Paul Menzel <pmenzel@molgen.mpg.de>]

[Cc: +Michal]

Dear Dawid, dear Jakub,


Am 13.01.26 um 20:38 schrieb Dawid Osuchowski:
> From: Jakub Staniszewski <jakub.staniszewski@linux.intel.com>
> 
> Add retry mechanism for indirect Admin Queue (AQ) commands. To do so we
> need to keep the command buffer.
> 
> This technically reverts commit 43a630e37e25
> ("ice: remove unused buffer copy code in ice_sq_send_cmd_retry()"),
> but combines it with a fix in the logic by using a kmemdup() call,
> making it more robust and less likely to break in the future due to
> programmer error.
> 
> Cc: Michal Schmidt <mschmidt@redhat.com>
> Cc: stable@vger.kernel.org
> Fixes: 3056df93f7a8 ("ice: Re-send some AQ commands, as result of EBUSY AQ error")
> Signed-off-by: Jakub Staniszewski <jakub.staniszewski@linux.intel.com>
> Co-developed-by: Dawid Osuchowski <dawid.osuchowski@linux.intel.com>
> Signed-off-by: Dawid Osuchowski <dawid.osuchowski@linux.intel.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
> Ccing Michal, given they are the author of the "reverted" commit.

At least Michal was not in the (visible) Cc: list

>   drivers/net/ethernet/intel/ice/ice_common.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
> index a400bf4f239a..aab00c44e9b2 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -1879,34 +1879,40 @@ ice_sq_send_cmd_retry(struct ice_hw *hw, struct ice_ctl_q_info *cq,
>   {
>   	struct libie_aq_desc desc_cpy;
>   	bool is_cmd_for_retry;
> +	u8 *buf_cpy = NULL;
>   	u8 idx = 0;
>   	u16 opcode;
>   	int status;
>   
>   	opcode = le16_to_cpu(desc->opcode);
>   	is_cmd_for_retry = ice_should_retry_sq_send_cmd(opcode);
>   	memset(&desc_cpy, 0, sizeof(desc_cpy));
>   
>   	if (is_cmd_for_retry) {
> -		/* All retryable cmds are direct, without buf. */
> -		WARN_ON(buf);
> +		if (buf) {
> +			buf_cpy = kmemdup(buf, buf_size, GFP_KERNEL);
> +			if (!buf_cpy)
> +				return -ENOMEM;
> +		}
>   
>   		memcpy(&desc_cpy, desc, sizeof(desc_cpy));
>   	}
>   
>   	do {
>   		status = ice_sq_send_cmd(hw, cq, desc, buf, buf_size, cd);
>   
>   		if (!is_cmd_for_retry || !status ||
>   		    hw->adminq.sq_last_status != LIBIE_AQ_RC_EBUSY)
>   			break;
>   
> +		if (buf_cpy)
> +			memcpy(buf, buf_cpy, buf_size);
>   		memcpy(desc, &desc_cpy, sizeof(desc_cpy));
> -

Unrelated change?

>   		msleep(ICE_SQ_SEND_DELAY_TIME_MS);
>   
>   	} while (++idx < ICE_SQ_SEND_MAX_EXECUTE);
>   
> +	kfree(buf_cpy);
>   	return status;
>   }

The diff looks good otherwise.

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul