Re: [PATCH iwl-net 07/10] ice: take the switch rule AQ error from the response descriptor

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ice: conflated -ENOSPC causes silent MAC filter drop on queue full
--

commit 5b00f3f5a2fd8517b295e1041c4e6854bbdcfd81
Author: Petr Oros <poros@redhat.com>

ice: take the switch rule AQ error from the response descriptor

This commit modifies how the driver extracts switch rule AQ errors by directly
reading the return code from the descriptor that ice_aq_send_cmd() copies back,
rather than from hw->adminq.sq_last_status. This avoids races when other
commands complete and overwrite the shared last status variable.

> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
[ ... ]
> @@ -450,7 +448,7 @@ static int ice_vsi_sync_fltr(struct ice_vsi *vsi)
>  		 * should go into promiscuous mode. There should be some
>  		 * space reserved for promiscuous filters.
>  		 */
> -		if (hw->adminq.sq_last_status == LIBIE_AQ_RC_ENOSPC &&
> +		if (err == -ENOSPC &&
>  		    !test_and_set_bit(ICE_FLTR_OVERFLOW_PROMISC,
>  				      vsi->state)) {
>  			promisc_forced_on = true;

[Severity: High]
Does this change unintentionally catch transient software queue exhaustion
errors?

If ice_sq_send_cmd() returns a native software -ENOSPC because the control
send queue is temporarily full (e.g., under heavy administrative queue load),
this fallback block will now catch it.

Previously, this block was gated behind the hardware-specific
hw->adminq.sq_last_status == LIBIE_AQ_RC_ENOSPC check. By changing it to
check err == -ENOSPC, a temporary queue-full condition could be misidentified
as a permanent hardware MAC filter limit.

This would cause the driver to erroneously set the overflow state bit and
return success, dropping MAC filters and leaving the hardware permanently out
of sync with the OS.

> diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
> --- a/drivers/net/ethernet/intel/ice/ice_switch.c
> +++ b/drivers/net/ethernet/intel/ice/ice_switch.c
[ ... ]
> +	if (status) {
> +		enum libie_aq_err aq_err = le16_to_cpu(desc.retval) & 0xff;
> +
> +		if (opc != ice_aqc_opc_add_sw_rules &&
> +		    aq_err == LIBIE_AQ_RC_ENOENT)
> +			status = -ENOENT;
> +		else if (opc == ice_aqc_opc_add_sw_rules &&
> +			 aq_err == LIBIE_AQ_RC_ENOSPC)
> +			status = -ENOSPC;
> +	}

[Severity: High]
Will this translation ignore hardware MAC filter exhaustion during rule
updates?

Because the error translation is strictly gated behind
opc == ice_aqc_opc_add_sw_rules, if a rule update command (e.g., expanding
a VSI list) triggers hardware exhaustion, it appears it will return a generic
-EIO rather than -ENOSPC.

Additionally, native software -ENOSPC errors returned directly by
ice_sq_send_cmd() will pass through unchanged, leading to the conflation
issue mentioned above in ice_vsi_sync_fltr().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002130752.2597259-1-poros@redhat.com?part=7