RE: [EXTERNAL] Re: [PATCH net-next v3 6/12] net: atlantic: fix AQC113 HW init: ART sections, L2 filter slot, MAC address

[Sukhdeep Soni [C]]

On 22nd May, 2026 Jakub Kicinski wrote:

>net: atlantic: fix AQC113 HW init: ART sections, L2 filter slot, MAC address
>
> This patch reworks hw_atl2 init to use firmware-reported filter layout
> fields (art_base_index, art_count, l2_filters_base_index) instead of
> hardcoded constants, adds a hw_atl2-specific MAC address setter that
> programs the FW-assigned L2 filter slot, and wires .hw_get_regs into
> hw_atl2_ops.
>
> A few questions inline.
> 
> 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..db0035fb04c7 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
> @@ -95,12 +96,21 @@ static int hw_atl2_hw_reset(struct aq_hw_s *self)
>  {
>  	struct hw_atl2_priv *priv = self->priv;
>  	int err;
> +	int i;
>  
>  	err = hw_atl2_utils_soft_reset(self);
>  	if (err)
>  		return err;
>  
> -	memset(priv, 0, sizeof(*priv));
> +	memset(&priv->last_stats, 0, sizeof(priv->last_stats));
> +	memset(priv->l3_v4_filters, 0, sizeof(priv->l3_v4_filters));
> +	memset(priv->l3_v6_filters, 0, sizeof(priv->l3_v6_filters));
> +	memset(priv->l4_filters, 0, sizeof(priv->l4_filters));
> +	memset(priv->etype_policy, 0, sizeof(priv->etype_policy));
> +	for (i = 0; i < HW_ATL2_RPF_L3L4_FILTERS; i++) {
> +		priv->l3l4_filters[i].l3_index = -1;
> +		priv->l3l4_filters[i].l4_index = -1;
> +	}
>
> [Low]
> The commit message says the change replaces the unconditional priv memset
> with selective field clears "to preserve firmware-reported layout
> (art_base_index, l2_filters_base_index, etc.) across resets".
>
> Is that description accurate?
>
> hw_atl2_hw_reset() runs hw_atl2_utils_soft_reset() right above this hunk,
> which calls aq_fw_ops->init() = aq_a2_fw_init(), which ends up calling
> hw_atl2_utils_get_filter_caps() and unconditionally repopulates
> art_base_index, art_count, l2_filters_base_index, l2_filter_count, the
> etype_filter_*, vlan_filter_*, l3_v4_filter_*, l3_v6_filter_* and
> l4_filter_* fields from the FW shared buffer.
>
> The full memset was therefore harmless for those fields. The durable
> behavioral change here looks like the new -1 sentinel for
> l3l4_filters[i].l3_index/l4_index (since 0 is a valid filter index).
>
> Could the changelog be reworded to say what the fix actually is?

Thank you for the careful analysis. The commit message will be
corrected in v4 to accurately describe the actual behavioral change:
the selective memset replaces the unconditional memset(priv, 0)
specifically to allow the -1 sentinel initialization for
l3l4_filters[i].l3_index/l4_index, since 0 is a valid filter index and
cannot serve as a "not assigned" marker.

> @@ -380,6 +390,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 +407,16 @@ 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 = min_t(u32,
> +			     priv->art_base_index / 8 + priv->art_count / 8,
> +			     16U);
> +	art_first_sec = min_t(u32, priv->art_base_index / 8, 16U);
> +	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);
>
> [Medium]
> Can art_mask collapse to zero here?
>
> If FW reports rslv_tbl_base_index >= 16 (so priv->art_base_index =
> 8 * rslv_tbl_base_index >= 128), then both clamped values become 16:
>
>    art_last_sec  = min_t(u32, 16+something, 16U) = 16
>    art_first_sec = min_t(u32, 16, 16U)           = 16
>    art_mask      = (BIT(16) - 1) - (BIT(16) - 1) = 0
>
> Since this code runs after a soft reset, the read-back of
> HW_ATL2_RPF_REC_TAB_EN via hw_atl2_rpf_act_rslvr_section_en_get() is
> typically 0, so art_sections = 0 | 0 = 0 and the entire Action Resolver
> Table ends up disabled.
>
> hw_atl2_utils_get_filter_caps() does not validate rslv_tbl_base_index
> (only art_count == 0 is clamped to 128), so nothing else catches this.
>
> Should there be a fallback path (warn and write 0xFFFF, or refuse to
> proceed) when the computed mask is 0? The previous unconditional 0xFFFF
> write was immune to this case.

You are correct that art_mask can collapse to zero when
rslv_tbl_base_index >= 16. This will be handled in v4 by
the guard immediately after:
 
  art_mask = (BIT(art_last_sec) - 1) - (BIT(art_first_sec) - 1);
  if (!art_mask) {
      /* ART base index is out of range, enabling all sections */
      art_mask = 0xFFFF;
  }
When the FW-reported base is out of range, we fall back to
enabling all 16 sections - matching the previous unconditional
0xFFFF write - so the ART is never left disabled. The guard was
added precisely to handle this degenerate case.

> +	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);
>
> [High]
> Is priv->l2_filters_base_index validated anywhere before being used as a
> filter slot index?
>
> filter_caps_s::l2_filters_base_index is a 6-bit field (encodable values
> 0..63), and hw_atl2_utils_get_filter_caps() copies it verbatim into
> priv->l2_filters_base_index without clamping. The same function does
> clamp the L3/L4 base/count fields against HW_ATL2_RPF_L3L4_FILTERS, so
> the omission here looks unintentional.
> 
> Two failure modes follow from FW-supplied values:
> 
>  - If FW reports a value in [HW_ATL2_MAC_MIN, HW_ATL2_MAC_MAX-1]
>    (i.e. [1, 37]), the UC slot collides with the multicast slot range
>    used by hw_atl2_hw_multicast_list_set() and the host MAC entry and a
>    multicast entry overwrite each other.
>
>  - If FW reports a value >= HW_ATL2_MAC_MAX (38), the loop in
>    hw_atl2_hw_init_rx_path() never matches, so no UC slot is enabled
>    and the device cannot accept its own MAC.
>
> The previous code used the literal HW_ATL2_MAC_UC (0), which was always
> in range. Should hw_atl2_utils_get_filter_caps() (or this site) bound
> priv->l2_filters_base_index against HW_ATL2_MAC_MAX before it is used?

Good catch on the analysis. This will be addressed in the v4 :
hw_atl2_utils_get_filter_caps() now clamps the value at the point of assignment:
 
  priv->l2_filters_base_index = filter_caps.l2_filters_base_index;
  if (priv->l2_filters_base_index >= HW_ATL2_MAC_MAX)
      priv->l2_filters_base_index = HW_ATL2_MAC_UC;
 
This is the right place to fix it - once at the source - rather than
at each of the three consumer sites. The fallback to HW_ATL2_MAC_UC (0)
preserves the previous safe behavior when FW reports an out-of-range value.

> @@ -484,6 +504,7 @@ static int hw_atl2_act_rslvr_table_set(struct aq_hw_s *self, u8 location,
>  static int hw_atl2_hw_init_rx_path(struct aq_hw_s *self)
>  {
>  	struct aq_nic_cfg_s *cfg = self->aq_nic_cfg;
> +	struct hw_atl2_priv *priv = self->priv;
>  	int i;
>  
> @@ -499,7 +520,9 @@ static int hw_atl2_hw_init_rx_path(struct aq_hw_s *self)
>  
>  	/* Multicast filters */
>  	for (i = HW_ATL2_MAC_MAX; i--;) {
> -		hw_atl_rpfl2_uc_flr_en_set(self, (i == 0U) ? 1U : 0U, i);
> +		hw_atl_rpfl2_uc_flr_en_set(self,
> +					   (i == priv->l2_filters_base_index) ?
> +					   1U : 0U, i);
>  		hw_atl_rpfl2unicast_flr_act_set(self, 1U, i);
>  	}
>
> [High]
> This is the second consumer of the unvalidated priv->l2_filters_base_index
> referenced above. If FW reports a value outside [0, HW_ATL2_MAC_MAX), the
> (i == priv->l2_filters_base_index) condition never holds for any i in the
> loop and no UC slot is enabled at all.

This is the same root cause as the previous comment - both consumer
sites are protected by the clamp added in hw_atl2_utils_get_filter_caps():

> @@ -530,6 +553,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;
> +	unsigned int h;
> +	unsigned int l;
> +	int err;
> +
> +	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);
>
> [High]
> Same concern here: location = priv->l2_filters_base_index is used directly
> in HW_ATL_RPFL2UC_DAFLSW_ADR(filter) / HW_ATL_RPFL2UC_DAFMSW_ADR(filter),
> which compute base + filter * stride. With a FW-supplied value beyond the
> L2 UC filter array, these writes will land at MMIO offsets outside the
> intended register range.
>
> Should location be bounds-checked against HW_ATL2_MAC_MAX (or whatever the
> hardware's actual L2 UC filter count is) before being used as an MMIO
> index?

This is the third consumer of priv->l2_filters_base_index - all three are
protected by the single clamp in hw_atl2_utils_get_filter_caps():

> @@ -825,7 +877,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,
>
> [Low]
> Could this patch be split, and could each piece carry a Fixes: tag?
>
> There appear to be five logically independent fixes folded together:
>
>  - selective memset plus the l3l4_filters l3_index/l4_index = -1 sentinel
>  - ART section enable computation from FW-reported base/count
>  - rpf_l3_v6_v4_select enable
>  - replacing the A1 hw_atl_b0_hw_mac_addr_set with a hw_atl2-specific
>    setter that targets the FW-assigned slot
>  - wiring .hw_get_regs into hw_atl2_ops
>
> The .hw_set_mac_address swap in particular is a runtime behavioral change
> for any FW reporting l2_filters_base_index != 0 (the host MAC moves slots,
> and hw_atl2_hw_init_rx_path() now enables only that slot instead of
> slot 0), so it would seem to deserve its own commit and Fixes: tag for
> backporting and bisection purposes.

The five items are tightly interdependent: the -1 sentinel makes the
l3l4 slot tracking reliable, the ART mask computation uses art_base_index
and art_count from the same FW caps read, the rpf_l3_v6_v4_select enable
is part of the same RX filter init path, and hw_atl2_hw_mac_addr_set targets
l2_filters_base_index from that same caps read - all flowing from the single
hw_atl2_utils_get_filter_caps() extension. Splitting them into five commits
would require each to carry forward partially-applied state and would make
bisection harder, not easier.
 
Regarding Fixes: tags: this patch introduces new hw_atl2-specific behavior
that was never upstream (the driver is new in this series), so there is no prior commit to tag.