RE: [EXTERNAL] Re: [PATCH net-next 6/9] net: atlantic: implement AQC113 L2/L3/L4 RX filter management filter management management

[Sukhdeep Soni [C]]

On 12 May 2026, Paolo Abeni wrote:

> From: AI Reviewer <mailto:ai@xxxxxxxxxxx>

> This commit wires up L2/L3/L4 RX filter management for AQC113 hardware,
> introducing reference-counted tag sharing of L3 and L4 filters, a combined
> L3L4 configuration path that drives Action Resolver Table entries, an L2
> ethertype set/clear pair, and a MAC-setup routine keyed off a firmware
> reported L2 filter base index. It also rewires several entries in
> hw_atl2_ops and updates the initialization of the new RX filter path.

> net: atlantic: implement AQC113 L2/L3/L4 RX filter management filter management management

> The subject line appears to have a copy/paste slip: "filter management"
> is repeated and "management" is tripled.

Repeated words are removed from subject in v3.

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_hw.h b/drivers/net/ethernet/aquantia/atlantic/aq_hw.h
> index 57ea59026a2c..04fb87d4e56d 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_hw.h
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_hw.h
> @@ -236,6 +236,8 @@ struct aq_hw_ops {
>  
>  	int (*hw_stop)(struct aq_hw_s *self);
>  
> +	u32 (*hw_get_version)(struct aq_hw_s *self);
> +
> 
> Is hw_get_version used anywhere in this series?  A grep across the driver
> shows only the ops vtable assignment and this struct field; no ethtool,
> debugfs, or regs-dump code calls self->aq_hw_ops->hw_get_version().
> Should this op be introduced together with its consumer, or dropped from
> this patch?

Correct — hw_get_version has no caller anywhere in this series.
Will drop the op from aq_hw_ops and the assignment from
hw_atl2_ops in v3 and reintroduce it alongside its first consumer.

> diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
> index 0ce9caae8799..e58bfff38670 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
> 
> [ ... ]
> 
> @@ -380,6 +422,9 @@ static void hw_atl2_hw_init_new_rx_filters(struct aq_hw_s *self)
>  {
>  	u8 *prio_tc_map = self->aq_nic_cfg->prio_tc_map;
>  	struct hw_atl2_priv *priv = self->priv;
> +	u32 art_first_sec, art_last_sec;
> +	u32 art_sections;
> +	u32 art_mask = 0;
>  	u16 action;
>  	u8 index;
>  	int i;
> @@ -394,9 +439,14 @@ static void hw_atl2_hw_init_new_rx_filters(struct aq_hw_s *self)
>  	 * REC entry is used for further processing. If multiple entries match,
>  	 * the lowest REC entry, Action field will be selected.
>  	 */
> -	hw_atl2_rpf_act_rslvr_section_en_set(self, 0xFFFF);
> +	art_last_sec = priv->art_base_index / 8 + priv->art_count / 8;
> +	art_first_sec = priv->art_base_index / 8;
> +	art_mask = (BIT(art_last_sec) - 1) - (BIT(art_first_sec) - 1);
> 
> Are art_first_sec and art_last_sec bounded before being passed to BIT()?
> 
> priv->art_base_index and priv->art_count come from firmware via
> hw_atl2_utils_get_action_resolve_table_caps() (rslv_tbl_base_index and
> rslv_tbl_count are 8-bit firmware fields).  BIT(nr) expands to
> UL(1) << nr, so values that make art_last_sec >= BITS_PER_LONG are C
> undefined behavior.
> 
> The consumer register RPF_REC_TAB_EN is 16 bits wide
> (HW_ATL2_RPF_REC_TAB_EN_MSK = 0xFFFF), so values beyond 16 also silently
> produce a bogus mask even if the shift does not invoke UB.
> 
> Could the firmware-reported caps be validated or clamped here?

Correct — if firmware reports large rslv_tbl_base_index or
rslv_tbl_count, art_last_sec can exceed 31 on 32-bit or
 63 on 64-bit, making BIT(art_last_sec) undefined behavior.
The register is 16 bits wide so sections beyond 15 are
meaningless anyway. Will clamp both art_first_sec and
art_last_sec to (16) before the BIT() calls in v3.

> +	art_sections = hw_atl2_rpf_act_rslvr_section_en_get(self) | art_mask;
> +	hw_atl2_rpf_act_rslvr_section_en_set(self, art_sections);
> +	hw_atl2_rpf_l3_v6_v4_select_set(self, 1);
>  	hw_atl2_rpfl2_uc_flr_tag_set(self, HW_ATL2_RPF_TAG_BASE_UC,
> -				     HW_ATL2_MAC_UC);
> +				     priv->l2_filters_base_index);
>  	hw_atl2_rpfl2_bc_flr_tag_set(self, HW_ATL2_RPF_TAG_BASE_UC);
> 
> [ ... ]
> 
> +static int hw_atl2_hw_mac_addr_set(struct aq_hw_s *self, const u8 *mac_addr)
> +{
> +	struct hw_atl2_priv *priv = self->priv;
> +	u32 location = priv->l2_filters_base_index;
> +	unsigned int h = 0U;
> +	unsigned int l = 0U;
> +	int err = 0;
> +
> +	if (!mac_addr) {
> +		err = -EINVAL;
> +		goto err_exit;
> +	}
> +	h = (mac_addr[0] << 8) | (mac_addr[1]);
> +	l = (mac_addr[2] << 24) | (mac_addr[3] << 16) |
> +		(mac_addr[4] << 8) | mac_addr[5];
> +
> +	hw_atl_rpfl2_uc_flr_en_set(self, 0U, location);
> +	hw_atl_rpfl2unicast_dest_addresslsw_set(self, l, location);
> +	hw_atl_rpfl2unicast_dest_addressmsw_set(self, h, location);
> +	hw_atl_rpfl2unicast_flr_act_set(self, 1U, location);
> +	hw_atl2_rpfl2_uc_flr_tag_set(self, HW_ATL2_RPF_TAG_BASE_UC, location);
> +	hw_atl_rpfl2_uc_flr_en_set(self, 1U, location);
> +
> +	err = aq_hw_err_from_flags(self);
> +
> +err_exit:
> +	return err;
> +}
> 
> Is the MAC-address programming path self-consistent at init when
> priv->l2_filters_base_index != 0?
> 
> hw_atl2_hw_init() continues to call hw_atl_b0_hw_mac_addr_set(self,
> mac_addr), which writes slot HW_ATL_B0_MAC (=0) with no UC tag.
> 
> hw_atl2_hw_init_new_rx_filters() tags slot priv->l2_filters_base_index
> with HW_ATL2_RPF_TAG_BASE_UC but does not write a MAC into it.
> 
> The new hw_atl2_hw_mac_addr_set() only runs when userspace issues
> ndo_set_mac_address.
> 
> Does this leave the device with slot 0 enabled and carrying the initial
> MAC but no UC tag, while slot priv->l2_filters_base_index has the UC tag
> but no address?  The commit message describes this as "Add MAC address
> setup using firmware-provided L2 filter base index" but
> hw_atl2_hw_init() still routes to the hardcoded slot.

Fixed in v3: change hw_atl2_hw_init() to call
 hw_atl2_hw_mac_addr_set() so the MAC is always
written to the firmware-provided slot from the start.

> +static int hw_atl2_new_fl3l4_configure(struct aq_hw_s *self,
> +				       struct aq_rx_filter_l3l4 *data)
> +{
> +	struct hw_atl2_priv *priv = self->priv;
> +	s8 old_l3_index = priv->l3l4_filters[data->location].l3_index;
> +	s8 old_l4_index = priv->l3l4_filters[data->location].l4_index;
> +	u8 old_ipv6 = priv->l3l4_filters[data->location].ipv6;
> +	struct hw_atl2_l3_filter *l3_filters;
> 
> [ ... ]
> 
> +	/* find L3 and L4 filters */
> +	if (l3.cmd & (HW_ATL2_RPF_L3_CMD_EN | HW_ATL2_RPF_L3_V6_CMD_EN)) {
> +		l3_idx = hw_atl2_new_fl3l4_find_l3(self, &l3);
> +		if (l3_idx < 0)
> +			return l3_idx;
> +
> +		if (l3.cmd & HW_ATL2_RPF_L3_V6_CMD_EN)
> +			l3_filters = priv->l3_v6_filters;
> +		else
> +			l3_filters = priv->l3_v4_filters;
> +
> +		if (priv->l3l4_filters[data->location].l3_index != l3_idx)
> +			hw_atl2_rxf_l3_get(self, &l3_filters[l3_idx],
> +					   l3_idx, &l3);
> +	}
> +
> +	if (old_l3_index != -1) {
> +		if (old_ipv6)
> +			l3_filters = priv->l3_v6_filters;
> +		else
> +			l3_filters = priv->l3_v4_filters;
> +
> +		if (!(hw_atl2_rxf_l3_is_equal(&l3,
> +					      &l3_filters[old_l3_index]))) {
> +			hw_atl2_rxf_l3_put(self,
> +					   &l3_filters[old_l3_index],
> +					   old_l3_index);
> +		}
> +	}
> 
> Note that l3_filters has been reassigned here based on old_ipv6.
> 

Acknowledged. The reassignment of l3_filters based on old_ipv6 is
intentional — it is used correctly to release the old L3 slot via
hw_atl2_rxf_l3_put(&l3_filters[old_l3_index], ...). The problematic
use of this same pointer with l3_idx (a new-family index) existed
only in the rollback block, which has been removed in v3.

> +	if (l3.cmd & HW_ATL2_RPF_L3_V6_CMD_EN)
> +		priv->l3l4_filters[data->location].ipv6 = 1;
> +	else
> +		priv->l3l4_filters[data->location].ipv6 = 0;
> +	priv->l3l4_filters[data->location].l3_index = l3_idx;
> +
> +	if (l4.cmd & HW_ATL2_RPF_L4_CMD_EN) {
> +		l4_idx = hw_atl2_new_fl3l4_find_l4(self, &l4);
> +		if (l4_idx < 0) {
> +			/* Undo L3 acquisition */
> +			if (l3_idx >= 0) {
> +				hw_atl2_rxf_l3_put(self, &l3_filters[l3_idx], l3_idx);
> +				priv->l3l4_filters[data->location].l3_index = old_l3_index;
> +				priv->l3l4_filters[data->location].ipv6 = old_ipv6;
> +			}
> +			return -EINVAL;
> +		}
> 
> Can this rollback corrupt the L3 refcounts?
> 
> If the new filter is IPv4 and the old one was IPv6 (or vice versa),
> l3_filters points at the old family's array by the time this undo runs,
> but l3_idx was allocated from the new family:
> 
>    if (l3.cmd & HW_ATL2_RPF_L3_V6_CMD_EN)
>         l3_filters = priv->l3_v6_filters;
>   else
>         l3_filters = priv->l3_v4_filters;
>     ...
>     /* later overwritten using old_ipv6 */
>     if (old_ipv6)
>         l3_filters = priv->l3_v6_filters;
>     else
>         l3_filters = priv->l3_v4_filters;
>     ...
>     hw_atl2_rxf_l3_put(self, &l3_filters[l3_idx], l3_idx);
> 
> This decrements usage in the wrong array and, on reaching zero, clears
> the wrong hardware register via hw_atl2_rpf_l3_v{4,6}_cmd_set().
> 
> Also, earlier in the function the old L3 may already have been put when
> its content differed from the new L3.  The rollback does not re-get() the
> old L3, so priv->l3l4_filters[data->location].l3_index is restored to
> old_l3_index while the corresponding slot has already been cleared if
> its usage was 1.
> 
> Finally, the new L3 get() is conditional:
> 
>    if (priv->l3l4_filters[data->location].l3_index != l3_idx)
>         hw_atl2_rxf_l3_get(self, &l3_filters[l3_idx], l3_idx, &l3);
> 
> but the rollback's put() is unconditional on l3_idx >= 0.  Re-submitting
> the same filter to the same location would skip the get(); a subsequent
> L4 failure then puts a reference the call never took, potentially
> underflowing the shared slot's usage count and clearing a filter another
> location still relies on.
> 
> Could this rollback path track whether get() was actually performed, and
> either hold on to the matched l3_filters pointer from the new family or
> re-derive it from the new l3.cmd instead of old_ipv6?
 
 Both issues are in the L4-failure undo block which has been removed in v3.
The rollback was added during upstreaming and is not present in the
reference driver. We've simplified the driver behavior —
returning -EINVAL on L4 failure with no rollback attempt. A proper transactional
rollback addressing cross-family pointer re-derivation and deferred old-L3
release will be done as a follow-up.

> +
> +		if (priv->l3l4_filters[data->location].l4_index != l4_idx)
> +			hw_atl2_rxf_l4_get(self, &priv->l4_filters[l4_idx],
> +					   l4_idx, &l4);
> +	}
> 
> [ ... ]
> 
> +static int hw_atl2_new_fl3l4_find_l3(struct aq_hw_s *self,
> +				     struct hw_atl2_l3_filter *l3)
> +{
> +	struct hw_atl2_priv *priv = self->priv;
> +	struct hw_atl2_l3_filter *l3_filters;
> +	int i, first, last;
> +
> +	if (l3->cmd & HW_ATL2_RPF_L3_V6_CMD_EN) {
> +		l3_filters = priv->l3_v6_filters;
> +		first = priv->l3_v6_filter_base_index;
> +		last = priv->l3_v6_filter_base_index +
> +		       priv->l3_v6_filter_count;
> +	} else {
> +		l3_filters = priv->l3_v4_filters;
> +		first = priv->l3_v4_filter_base_index;
> +		last = priv->l3_v4_filter_base_index +
> +		       priv->l3_v4_filter_count;
> +	}
> +	for (i = first; i < last; i++) {
> +		if (hw_atl2_rxf_l3_is_equal(&l3_filters[i], l3))
> +			return i;
> +	}
> 
> Can this index the static arrays out of bounds?
> 
> The backing arrays are fixed size:
> 
>     struct hw_atl2_l3_filter l3_v4_filters[HW_ATL2_RPF_L3L4_FILTERS];
>    struct hw_atl2_l3_filter l3_v6_filters[HW_ATL2_RPF_L3L4_FILTERS];
>    struct hw_atl2_l4_filter l4_filters[HW_ATL2_RPF_L3L4_FILTERS];
> 
> but l3_v4_filter_base_index, l3_v6_filter_base_index, l4_filter_base_index
> and the corresponding counts come from firmware.  Looking at
> hw_atl2_utils_get_filter_caps() (referenced elsewhere in the series),
> l3_v6_filter_count is not clamped at all, unlike l3_v4_filter_count and
> l4_filter_count which get reduced by one against
> HW_ATL2_RPF_L3V4_FILTERS / HW_ATL2_RPF_L4_FILTERS.  None of the
> base_index fields are validated against the fixed-size arrays either.
> 
> A firmware value that places first+count past HW_ATL2_RPF_L3L4_FILTERS
> yields out-of-bounds reads here and out-of-bounds writes via
> hw_atl2_rxf_l3_get() / _put().  The same concern applies to
> hw_atl2_new_fl3l4_find_l4().
> 
> Could these indices be sanitized at probe time?

Addressed in patch 5 (v3): for each filter category,
if base_index >= HW_ATL2_RPF_L3L4_FILTERS the count is forced to zero,
otherwise count is capped to HW_ATL2_RPF_L3L4_FILTERS - 1 - base_index,
so base_index + count never exceeds the backing array

> [ ... ]
> 
> +static int hw_atl2_hw_fl2_set(struct aq_hw_s *self,
> +			      struct aq_rx_filter_l2 *data)
> +{
> +	struct hw_atl2_priv *priv = self->priv;
> +	u32 mask = HW_ATL2_RPF_TAG_ET_MASK;
> +	u32 req_tag = 0;
> +	u16 action = 0;
> +	u32 location;
> +	u8 index;
> +	int tag;
> +
> +	location = priv->etype_filter_base_index + data->location;
> +	hw_atl_rpf_etht_flr_set(self, data->ethertype, location);
> +	hw_atl_rpf_etht_user_priority_en_set(self,
> +					     !!data->user_priority_en,
> +					     location);
> +	if (data->user_priority_en) {
> +		hw_atl_rpf_etht_user_priority_set(self,
> +						  data->user_priority,
> +						  location);
> +		req_tag |= data->user_priority << HW_ATL2_RPF_TAG_PCP_OFFSET;
> +		mask |= HW_ATL2_RPF_TAG_PCP_MASK;
> +	}
> +
> +	if (data->queue < 0) {
> +		hw_atl_rpf_etht_flr_act_set(self, 0U, location);
> +		hw_atl_rpf_etht_rx_queue_en_set(self, 0U, location);
> +		action = HW_ATL2_ACTION_DROP;
> +	} else {
> +		hw_atl_rpf_etht_flr_act_set(self, 1U, location);
> +		hw_atl_rpf_etht_rx_queue_en_set(self, 1U, location);
> +		hw_atl_rpf_etht_rx_queue_set(self, data->queue, location);
> +		action = HW_ATL2_ACTION_ASSIGN_QUEUE(data->queue);
> +	}
> +
> +	tag = hw_atl2_filter_tag_get(priv->etype_policy,
> +				     priv->etype_filter_tag_top,
> +				     action);
> +
> +	if (tag < 0)
> +		return -ENOSPC;
> 
> Two concerns about this path:
> 
> First, if location already held an ethertype filter with a previous tag,
> is that tag ever released?

> aq_add_del_fether() in aq_filters.c calls hw_filter_l2_set() directly on
> replacement (without an intervening hw_filter_l2_clear()), so each
> overwrite of an existing location invokes hw_atl2_filter_tag_get() for
> the new action but never hw_atl2_filter_tag_put() for the old one.  The
> etype_policy[] usage counters will diverge from the set of tags actually
> live in hardware, and after a small number of changes with distinct
> actions the pool (bounded by etype_filter_tag_top) is exhausted and
> set() starts returning -ENOSPC.

> Second, on the -ENOSPC path above, the per-location ethertype,
> user_priority_en/user_priority, flr_act, rx_queue_en and rx_queue
> registers have already been written for the new filter while the old
> etht_flr_en bit and the old tag are still set in hardware.  Should this
> error path leave the hardware state unchanged, or disable the filter
> before returning?

Not a concern in practice. hw_filter_l2_set() is always preceded by
hw_filter_l2_clear(), which explicitly calls
hw_atl_rpf_etht_flr_en_set(self, 0U, location) and sets the ART entry
to HW_ATL2_ACTION_DISABLE before fl2_set() touches any register.
On -ENOSPC, the partial register writes (ethertype, priority, queue) are
present in hardware but the filter enable bit and ART entry are both
already disabled — no active filter with a stale tag, no traffic impact.

> +
> +	req_tag |= tag << HW_ATL2_RPF_TAG_ET_OFFSET;
> +	hw_atl2_rpf_etht_flr_tag_set(self, tag, location);
> +	index = priv->art_base_index + HW_ATL2_RPF_ET_PCP_USER_INDEX +
> +		data->location;
> +	hw_atl2_act_rslvr_table_set(self, index, req_tag, mask, action);
> +
> +	hw_atl_rpf_etht_flr_en_set(self, 1U, location);
> +
> +	return aq_hw_err_from_flags(self);
> +}

> [ ... ]

> @@ -825,7 +1394,7 @@ static int hw_atl2_hw_vlan_ctrl(struct aq_hw_s *self, bool enable)
>  const struct aq_hw_ops hw_atl2_ops = {
>  	.hw_soft_reset        = hw_atl2_utils_soft_reset,
>  	.hw_prepare           = hw_atl2_utils_initfw,
> -	.hw_set_mac_address   = hw_atl_b0_hw_mac_addr_set,
> +	.hw_set_mac_address   = hw_atl2_hw_mac_addr_set,
>  	.hw_init              = hw_atl2_hw_init,
>  	.hw_reset             = hw_atl2_hw_reset,
>  	.hw_start             = hw_atl_b0_hw_start,
> @@ -834,6 +1403,7 @@ const struct aq_hw_ops hw_atl2_ops = {
>  	.hw_ring_rx_start     = hw_atl_b0_hw_ring_rx_start,
>  	.hw_ring_rx_stop      = hw_atl_b0_hw_ring_rx_stop,
>  	.hw_stop              = hw_atl2_hw_stop,
> +	.hw_get_version       = hw_atl2_get_hw_version,
> 
> See the earlier note; hw_get_version does not appear to have any caller
> in this series.

Yes, it's been dropped!
-- 
This is an AI-generated review.