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

[Ivan Vecera]

On October 2, 2026 3:07:49 PM GMT+02:00, Petr Oros <poros@xxxxxxxxxx> wrote:
>ice_aq_sw_rules() decides whether a rule removal failed with ENOENT by
>reading hw->adminq.sq_last_status after ice_aq_send_cmd() returned. That
>field is shared by every admin queue user and is only stable while the
>send queue lock is held. Another AQ command completing in between can
>overwrite it, so a failed removal is reported as a generic error, and a
>successful one can even be turned into -ENOENT, because the check is
>not limited to the failure case. A caller that sees -ENOENT keeps its
>rule bookkeeping while the rule is gone from the hardware.
>
>ice_vsi_sync_fltr() has the same problem with ENOSPC. It checks
>sq_last_status only after it has freed the list of filters it tried to
>add. Every entry is a separate devres allocation, so freeing a list of
>thousands of entries takes seconds, and by then the value usually
>belongs to a different command, so the MAC filter overflow handling is
>never entered. With debug prints of the error and sq_last_status added
>to both places:
>
>  ice_aq_sw_rules: opc 0x2a0 status -5 aq 16
>  ice 0000:04:00.0 enp4s0f0np0: Failed to add MAC filters err -5 aq 0
>
>Use the retval of the descriptor that ice_aq_send_cmd() copies back,
>only when the command failed, translate ENOSPC on add to -ENOSPC and
>let ice_vsi_sync_fltr() check the return code instead of
>sq_last_status.
>
>i40e fixed the same kind of race in commit 53a9e346e159 ("i40e: Fix race
>condition while adding/deleting MAC/VLAN filters").
>
>Fixes: ca1fdb885e5f ("ice: return correct error code from ice_aq_sw_rules")
>Assisted-by: LLM
>Signed-off-by: Petr Oros <poros@xxxxxxxxxx>
>---
> drivers/net/ethernet/intel/ice/ice_main.c   |  4 +---
> drivers/net/ethernet/intel/ice/ice_switch.c | 13 ++++++++++---
> 2 files changed, 11 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>index 8a21f87eb6ca21..ceb9fec2af21e7 100644
>--- a/drivers/net/ethernet/intel/ice/ice_main.c
>+++ b/drivers/net/ethernet/intel/ice/ice_main.c
>@@ -396,8 +396,6 @@ static int ice_vsi_sync_fltr(struct ice_vsi *vsi)
> 	struct device *dev = ice_pf_to_dev(vsi->back);
> 	struct net_device *netdev = vsi->netdev;
> 	bool promisc_forced_on = false;
>-	struct ice_pf *pf = vsi->back;
>-	struct ice_hw *hw = &pf->hw;
> 	u32 changed_flags = 0;
> 	int err;
> 
>@@ -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;
>diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
>index 239d4d9633baa6..ae96a2003d5c5e 100644
>--- a/drivers/net/ethernet/intel/ice/ice_switch.c
>+++ b/drivers/net/ethernet/intel/ice/ice_switch.c
>@@ -1959,9 +1959,16 @@ ice_aq_sw_rules(struct ice_hw *hw, void *rule_list, u16 rule_list_sz,
> 	desc.flags |= cpu_to_le16(LIBIE_AQ_FLAG_RD);
> 	cmd->num_rules_fltr_entry_index = cpu_to_le16(num_rules);
> 	status = ice_aq_send_cmd(hw, &desc, rule_list, rule_list_sz, cd);
>-	if (opc != ice_aqc_opc_add_sw_rules &&
>-	    hw->adminq.sq_last_status == LIBIE_AQ_RC_ENOENT)
>-		status = -ENOENT;
>+	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;
>+	}
> 
> 	if (!status) {
> 		if (opc == ice_aqc_opc_add_sw_rules)

Reviewed-by: Ivan Vecera <ivecera@xxxxxxxxxx>