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

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Petr Oros <poros@xxxxxxxxxx>
> Sent: Friday, October 2, 2026 3:08 PM
> To: netdev@xxxxxxxxxxxxxxx
> Cc: Oros, Petr <poros@xxxxxxxxxx>; Nguyen, Anthony L
> <anthony.l.nguyen@xxxxxxxxx>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@xxxxxxxxx>; Andrew Lunn <andrew+netdev@xxxxxxx>;
> David S. Miller <davem@xxxxxxxxxxxxx>; Eric Dumazet
> <edumazet@xxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; Paolo Abeni
> <pabeni@xxxxxxxxxx>; Lobakin, Aleksander
> <aleksander.lobakin@xxxxxxxxx>; Alexei Starovoitov <ast@xxxxxxxxxx>;
> Daniel Borkmann <daniel@xxxxxxxxxxxxx>; Jesper Dangaard Brouer
> <hawk@xxxxxxxxxx>; John Fastabend <john.fastabend@xxxxxxxxx>;
> Stanislav Fomichev <sdf@xxxxxxxxxxx>; Henry Tieman
> <henry.w.tieman@xxxxxxxxx>; Anirudh Venkataramanan
> <anirudh.venkataramanan@xxxxxxxxx>; Michal Swiatkowski
> <michal.swiatkowski@xxxxxxxxxxxxxxx>; Jesse Brandeburg
> <jbrandeb@xxxxxxxxxx>; Preethi Banala <preethi.banala@xxxxxxxxx>;
> Kiran Patil <kiran.patil@xxxxxxxxx>; Dan Nowlin
> <dan.nowlin@xxxxxxxxx>; Stephen Hemminger
> <stephen@xxxxxxxxxxxxxxxxxx>; intel-wired-lan@xxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; bpf@xxxxxxxxxxxxxxx
> Subject: [PATCH iwl-net 07/10] ice: take the switch rule AQ error from
> the response descriptor
> 
> 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)
> --
> 2.55.0

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@xxxxxxxxx>