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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
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?

> @@ -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.

> +	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?

> @@ -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.

> @@ -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?

> @@ -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.