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. > 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/[email protected]?part=7
