On 10/2/26 15:27, [email protected] 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 <[email protected]>
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.
