RE: [EXTERNAL] Re: [PATCH net-next v2 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:

> net: atlantic: implement AQC113 L2/L3/L4 RX filter management filter management management
>
> This change implements RX filter management for the AQC113, adding
> reference-counted sharing of L3/L4 hardware slots, L2 ethertype
> programming with ART tags, firmware-reported base/count usage, and a
> MAC address helper that targets the FW-supplied l2_filters_base_index.
>
> [Low]
> The subject line contains a duplicated/triplicated phrase:

Fixed in v3.

>
>  net: atlantic: implement AQC113 L2/L3/L4 RX filter management filter management management
>
> 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);
> +
>
> [Low]
> Is there any in-tree caller of ->hw_get_version()?  A search across the
> atlantic driver and the rest of the tree does not show one, and the other
> chip ops tables (hw_atl_a0, hw_atl_b0) leave this slot NULL.  If no caller
> is introduced in this series, would it make sense to drop this hook for
> now, or add it together with the consumer?

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..7abf7fe6e32c 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;
>  	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);
> +	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);
>
>[ ... ]
>
> @@ -530,6 +580,35 @@ static int hw_atl2_hw_init_rx_path(struct aq_hw_s *self)
>  	return aq_hw_err_from_flags(self);
>  }
>  
> +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;

> [High]
> Does the initial MAC programming still land on the right slot when
> firmware reports a non-zero l2_filters_base_index?
>
> The new .hw_set_mac_address and hw_atl2_hw_init_new_rx_filters() both
> operate on priv->l2_filters_base_index.  However, hw_atl2_hw_init() still
> calls hw_atl_b0_hw_mac_addr_set(self, mac_addr) directly, and that helper
> hardcodes HW_ATL_B0_MAC (= 0).  At the same time hw_atl2_hw_init_rx_path()
> enables only slot 0 via:
>
>    for (i = HW_ATL2_MAC_MAX; i--;) {
>        hw_atl_rpfl2_uc_flr_en_set(self, (i == 0U) ? 1U : 0U, i);
>        ...
>    }
>
> If FW reports l2_filters_base_index != 0, the tagged slot has no MAC
> programmed and the enabled slot (0) carries the wrong tag.  Should
> hw_atl2_hw_init() call hw_atl2_hw_mac_addr_set() instead, or should the
> enable/base bookkeeping be reconciled?
>

Fixed in v3: hw_atl2_hw_init() now calls hw_atl2_hw_mac_addr_set()
 which writes the MAC to priv->l2_filters_base_index, and
 hw_atl2_hw_init_rx_path() enables slot priv->l2_filters_base_index
 rather than hardcoding slot 0. All three — enabled slot, MAC address,
 and UC tag — are consistent on the firmware-reported location.

> [ ... ]
>
> @@ -767,6 +846,496 @@ static struct aq_stats_s *hw_atl2_utils_get_hw_stats(struct aq_hw_s *self)
>  	return &self->curr_stats;
>  }
>  
>
> [ ... ]
>
> +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;
> +	}
> +
> +	for (i = first; i < last; i++) {
> +		u32 l3_enable_mask = HW_ATL2_RPF_L3_CMD_EN |
> +				     HW_ATL2_RPF_L3_V6_CMD_EN;
> +
> +		if (!(l3_filters[i].cmd & l3_enable_mask))
> +			return i;
> +	}
> +
> +	return -ENOSPC;
> +}

> [High]
> Can this walk off the end of the fixed-size arrays?
>
> priv->l3_v4_filters, priv->l3_v6_filters, and priv->l4_filters are all
> sized HW_ATL2_RPF_L3L4_FILTERS (= 8).  first/last come from the
> firmware-reported filter_caps_s, where base_index is a 4-bit field (0..15)
> and the counts are also 4 bits.
>
> Looking at hw_atl2_utils_get_filter_caps(), l3_v4_filter_count and
> l4_filter_count are clamped with min_t(..., HW_ATL2_RPF_L3{V4,4}_FILTERS
> - 1), but l3_v6_filter_count does not appear to be clamped at all, and
> none of the *_base_index fields are clamped.  A FW image reporting
> l3_v6_filter_count >= 8, or any base_index such that base + count > 8,
> would make this loop read priv->l3_v6_filters[i].cmd past the end, and
> hw_atl2_rxf_l3_get() / hw_atl2_rxf_l4_get() would write cmd/srcip/dstip/
> sport/dport past the end.
>
> Should all three counts be clamped to HW_ATL2_RPF_L3L4_FILTERS, and
> base_index + count bounded to HW_ATL2_RPF_L3L4_FILTERS?

Addressed in patch 5 (v3): for all three filter categories (l3_v4, l3_v6,
 l4), 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, ensuring base_index + count
never exceeds the backing array bounds.

> +
> +static void hw_atl2_rxf_l3_get(struct aq_hw_s *self,
> +			       struct hw_atl2_l3_filter *l3, int idx,
> +			       const struct hw_atl2_l3_filter *_l3)
> +{
> +	int i;
> +
> +	l3->usage++;
> +	if (l3->usage == 1) {
> +		l3->cmd = _l3->cmd;
> +		for (i = 0; i < 4; i++) {
> +			l3->srcip[i] = _l3->srcip[i];
> +			l3->dstip[i] = _l3->dstip[i];
> +		}
> +		l3->proto = _l3->proto;
> +
> +		if (l3->cmd & HW_ATL2_RPF_L3_CMD_EN) {
> +			hw_atl2_rpf_l3_v4_cmd_set(self, l3->cmd, idx);
> +			hw_atl2_rpf_l3_v4_tag_set(self, idx + 1, idx);
>
> [High]
> Can the per-filter tag value be silently truncated here?
>
> HW_ATL2_RPF_L3_V{4,6}_TAG_MSK and HW_ATL2_RPF_L4_TAG_MSK are 3-bit fields
> (max value 7), and the helpers aq_hw_write_reg_bit() mask the value to
> the field width.  idx is in [base_index, base_index + count), and with
> l3_v6_filter_count unclamped (and no clamp on any base_index), idx + 1
> can exceed 7 and quietly collapse to a tag used by another slot.
>
> When two slots end up with the same 3-bit tag, the ART input_tag/mask
> comparison can pick the lowest matching record, so a drop rule could
> match traffic intended to be forwarded (or vice versa) without any
> diagnostic.  Should idx + 1 be range-checked against the mask width, or
> the counts clamped so idx + 1 <= 7?

The patch 5 (v3) validation ensures 
base_index + count <= HW_ATL2_RPF_L3L4_FILTERS - 1, so idx is at most 6 and
idx + 1 is at most 7, which fits the 3-bit tag field without truncation.
 Tag value 0 (no tag) is never assigned since idx starts at
 base_index >= 0, making idx + 1 >= 1.

> +			hw_atl2_rpf_l3_v4_dest_addr_set(self,
> +							idx,
> +							l3->dstip[0]);
> +			hw_atl2_rpf_l3_v4_src_addr_set(self,
> +						       idx,
> +						       l3->srcip[0]);
> +		} else {
> +			hw_atl2_rpf_l3_v6_cmd_set(self, l3->cmd, idx);
> +			hw_atl2_rpf_l3_v6_tag_set(self, idx + 1, idx);
> +			hw_atl2_rpf_l3_v6_dest_addr_set(self,
> +							idx,
> +							l3->dstip);
> +			hw_atl2_rpf_l3_v6_src_addr_set(self,
> +						       idx,
> +						       l3->srcip);
> +		}
> +	}
> +}
>
> [ ... ]
>
> +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;
>
> [ ... ]
>
> +	/* 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);
> +	}
>
> [High]
> Does this guard correctly handle a family switch at the same location?
>
> priv->l3_v4_filters[] and priv->l3_v6_filters[] are separate 8-entry
> arrays.  When an existing v4 rule at data->location is reconfigured to
> v6 (or the reverse), hw_atl2_new_fl3l4_find_l3() searches the new
> family's array and is likely to return 0 as the first free slot.  If
> the previously stored l3_index was also 0, then:
>
>    if (priv->l3l4_filters[data->location].l3_index != l3_idx)  /* 0 != 0 */
>        hw_atl2_rxf_l3_get(...);   /* skipped */
>
> hw_atl2_rxf_l3_get() is skipped, so priv->l3_v6_filters[0].cmd stays 0
> and no v6 HW registers are written.  Later, hw_atl2_hw_fl3l4_set() reads
> ipv6 = 1 and l3_index = 0, picks l3 = &priv->l3_v6_filters[0], finds
> (l3->cmd & V6_CMD_EN) == 0, and programs the ART tag down the V4 path
> while the user asked for V6.
> 
> Would it be correct to include the family in the guard, or to just
> call hw_atl2_rxf_l3_get() unconditionally (it is already idempotent via
> the usage == 1 check)?

Fixed in v3. The guard now includes a family check:

if (priv->l3l4_filters[data->location].l3_index != l3_idx ||
      old_ipv6 != !!(l3.cmd & HW_ATL2_RPF_L3_V6_CMD_EN))
      hw_atl2_rxf_l3_get(...);
 
 When the family switches (v4→v6 or v6→v4), old_ipv6 differs from the
 new family flag, so get() is called regardless of whether the slot
 index happens to be the same number, ensuring the new family's HW
 registers are correctly programmed.

> +
> +	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);
> +		}
> +	}
> +	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;
> +		}
>
> [High, High]
> Does the L4-failure undo path correctly unwind L3 state in two distinct
> ways?
> 
> First, cross-family pointer aliasing.  Before the L4 find, l3_filters
> points to the new family's array (from the earlier block).  The
> intermediate "if (old_l3_index != -1)" block unconditionally reassigns
> l3_filters to the old family based on old_ipv6.  By the time the undo
> runs, l3_filters still points to the old family's array, but l3_idx is
> a new-family index:
> 
>    hw_atl2_rxf_l3_put(self, &l3_filters[l3_idx], l3_idx);
>
> For a cross-family reconfigure this decrements the wrong filter (and
> may clear its HW via hw_atl2_rpf_l3_v{4,6}_cmd_set(self, 0, idx)) while
> the actually-acquired new-family filter's refcount is leaked.  Should
> the undo re-derive l3_filters from the new family before calling put?
>
> Second, the old L3 filter is not re-acquired.  Earlier in the function,
> if old_l3_index != -1 and !is_equal(new, old), the old filter has
> already had hw_atl2_rxf_l3_put() called on it, potentially dropping
> usage to 0 and clearing HW.  The undo block restores l3_index = old and
> ipv6 = old_ipv6, but does not restore the refcount or HW state of the
> old filter.  Would the old filter need a matching rxf_l3_get() call
> here to keep priv->l3l4_filters[location] consistent with the actual
> HW/refcount state?
> 

Both concerns are addressed in v3.

For the cross-family pointer aliasing: the v3 code removes the undo
block entirely.  When hw_atl2_new_fl3l4_find_l4() returns < 0 the
function returns -EINVAL immediately — there is no code path that
would dereference l3_filters (pointing at the old family) using l3_idx
(a new-family index).

For the "old filter not re-acquired" concern: v3 uses a commit-first order.
The L3 acquisition, the old-L3 put, and the priv bookkeeping (l3_index, ipv6)
are all written before the L4 search.  Because there is no rollback,
no re-acquisition is required.  The resulting partial priv state on L4 failure
(l3_index/ipv6 updated, l4_index not yet) is never seen by
HW: hw_atl2_hw_fl3l4_set() writes the ART entry only when this function returns 0.

> [ ... ]
>
> +static int hw_atl2_hw_fl3l4_set(struct aq_hw_s *self,
> +				struct aq_rx_filter_l3l4 *data)
> +{
>
> [ ... ]
>
> +	res = hw_atl2_new_fl3l4_configure(self, data);
> +	if (res)
> +		return res;
> +
>
> [ ... ]
>
> +	if (!(data->cmd & HW_ATL_RX_ENABLE_FLTR_L3L4)) {
>
> [ ... ]
>
> +		return 0;
> +	}
> +
>
> [ ... ]
>
> +	if (data->cmd & (HW_ATL_RX_HOST << HW_ATL2_RPF_L3_L4_ACTF_SHIFT))
> +		action = HW_ATL2_ACTION_ASSIGN_QUEUE((data->cmd  &
> +						      HW_ATL2_RPF_L3_L4_RXQF_MSK) >>
> +						     HW_ATL2_RPF_L3_L4_RXQF_SHIFT);
> +	else if (data->cmd)
> +		action = HW_ATL2_ACTION_DROP;
> +	else
> +		action = HW_ATL2_ACTION_DISABLE;
>
> [Low]
> Can the final "else action = HW_ATL2_ACTION_DISABLE" branch actually be
> reached?  The function has already returned early for the case where
> HW_ATL_RX_ENABLE_FLTR_L3L4 is clear, so at this point data->cmd always
> has at least that bit set and is non-zero, which means "else if
> (data->cmd)" always fires.  Was a different condition intended here?

Fixed in v3. Dropped the unreachable else if (data->cmd) / else ACTION_DISABLE
branches — past the HW_ATL_RX_ENABLE_FLTR_L3L4 early return data->cmd is
always non-zero, so simplified to a plain if/else between ASSIGN_QUEUE and DROP.

> [ ... ]
>
> @@ -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,
>
> [ ... ]
> -- 
> This is an AI-generated review.