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

[Petr Oros <poros@redhat.com>]

On 10/2/26 15:27, sashiko-bot@kernel.org wrote:
> 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.
The send queue can not be temporarily full. Commands are sent one at a
time under sq_lock and each one waits for its completion, and the queue
is reclaimed from the FW head pointer. It only fills up once the FW has
stopped processing the AQ for 191 commands in a row, and at that point
no filter can be programmed either way.
>
>> 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.
An update modifies an existing rule and the FW does not allocate
anything for it. The VSI list allocation in that path goes through
ice_aq_alloc_free_res(), not through ice_aq_sw_rules(), so the
translation is limited to add on purpose, the same way ca1fdb885e5f
limited ENOENT to the non-add commands.

>
> 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().
  See above, that needs a send queue the FW stopped draining.