RE: [EXTERNAL] Re: [PATCH net-next 5/9] net: atlantic: add AQC113 filter data structures and firmware query and firmware query firmware query

[Sukhdeep Soni [C]]

On 12 May 2026, Paolo Abeni wrote:

> net: atlantic: add AQC113 filter data structures and firmware query and firmware query firmware query
> 
> This patch adds filter data structures, RPF L3/L4 command bits, filter
> count constants, and a firmware capability query used to populate filter
> base indices and counts stored in hw_atl2_priv. It also adds a MAC
> register dump helper for debug support.
> 
> > net: atlantic: add AQC113 filter data structures and firmware query and firmware query firmware query
> 
> The subject line has "firmware query" repeated three times.  Similar
> duplication appears on other subjects in this series (for example
> "filter management filter management management", "(Antigua) (Antigua)",
> "TX path setup TX path setup").  Would it be possible to regenerate the
> series so each subject reads cleanly?
> 

Fixed in v3!

> > diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_internal.h
> >b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_internal.h
> > index 5a89bb8722f9..fc086d84fb91 100644
> > --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_internal.h
> > +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_internal.h
> 
> [ ... ]
> 
> > @@ -119,9 +137,54 @@ enum HW_ATL2_RPF_RSS_HASH_TYPE {
>  
> >  #define HW_ATL_MCAST_FLT_ANY_TO_HOST 0x00010FFFU
> >  
> > +struct hw_atl2_l3_filter {
> > +	u8 proto;
> > +	u8 usage;
> > +	u32 cmd;
> > +	u32 srcip[4];
> > +	u32 dstip[4];
> > +};
> 
> [ ... ]
> 
> >  struct hw_atl2_priv {
> > +	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];
> > +	struct hw_atl2_l3l4_filter l3l4_filters[HW_ATL2_RPF_L3L4_FILTERS];
> > +	struct hw_atl2_tag_policy etype_policy[HW_ATL2_RPF_ETYPE_FILTERS];
> >  	struct statistics_s last_stats;
> >  	unsigned int art_base_index;
> > +	unsigned int art_count;
> > +	unsigned int l2_filters_base_index;
> > +	unsigned int l2_filter_count;
> > +	unsigned int etype_filter_base_index;
> > +	unsigned int etype_filter_count;
> > +	unsigned int etype_filter_tag_top;
> > +	unsigned int vlan_filter_base_index;
> > +	unsigned int vlan_filter_count;
> > +	unsigned int l3_v4_filter_base_index;
> > +	unsigned int l3_v4_filter_count;
> > +	unsigned int l3_v6_filter_base_index;
> > +	unsigned int l3_v6_filter_count;
> > +	unsigned int l4_filter_base_index;
> > +	unsigned int l4_filter_count;
> >  };
> 
> The backing arrays l3_v4_filters[], l3_v6_filters[], l4_filters[] and
> etype_policy[] are fixed-size (HW_ATL2_RPF_L3L4_FILTERS == 8, and
> HW_ATL2_RPF_ETYPE_FILTERS == 16), but the *_count and *_base_index
> fields are populated from firmware below.  See the next comment in
> hw_atl2_utils_get_filter_caps() about validation.
>
> > diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_utils.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_utils.c
> > index 0fe6257d9c08..ffd723dcfb63 100644
> > --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_utils.c
> > +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_utils.c
> > @@ -128,3 +128,36 @@ int hw_atl2_utils_soft_reset(struct aq_hw_s *self)
> >  err_exit:
> >  	return err;
> >  }
> > +
> > +static const u32 hw_atl2_utils_hw_mac_regs[] = {
> > +	0x00005580U, 0x00005590U, 0x000055B0U, 0x000055B4U,
> > 
> > [ ... array of 72 entries ... ]
> > 
> > > +	0x00007CD0U, 0x00007CD4U, 0x00007CD8U, 0x00007CDCU,
> > +};
> > +
> > +int hw_atl2_utils_hw_get_regs(struct aq_hw_s *self,
> > +			      const struct aq_hw_caps_s *aq_hw_caps,
> > +			      u32 *regs_buff)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < aq_hw_caps->mac_regs_count; i++)
> > +		regs_buff[i] = aq_hw_read_reg(self,
> > +					      hw_atl2_utils_hw_mac_regs[i]);
> > +	return 0;
> > +}
> 
> This isn't a bug today, since hw_atl2_caps_aqc113.mac_regs_count is 72
> and hw_atl2_utils_hw_mac_regs[] has 72 entries, but would a
> BUILD_BUG_ON() or a runtime min_t() tying the loop bound to
> ARRAY_SIZE(hw_atl2_utils_hw_mac_regs) be worth adding?  If a future
> variant bumps mac_regs_count without updating the table, the loop
> would read past the rodata array and issue readl() on unintended
> register offsets.
> 
> Also, hw_atl2_utils_hw_get_regs() only ever returns 0.  Does it need
> to be declared as returning int, or could it be void?

Will add the min_t(ARRAY_SIZE(...)) guard to protect against future
mac_regs_count increases without a matching table update. The int
return type must stay — it is mandated by the hw_get_regs function
pointer in aq_hw_ops shared with the A0/B0 implementations.

> > diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_utils_fw.c
> > b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_utils_fw.c
> > index 7370e3f76b62..546b48f897d3 100644
> > --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_utils_fw.c
> > +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_utils_fw.c
> 
> [ ... ]
> 
> > @@ -606,6 +610,54 @@ u32 hw_atl2_utils_get_fw_version(struct aq_hw_s *self)
> >  	       version.bundle.build;
> >  }
> >  
> > +int hw_atl2_utils_get_filter_caps(struct aq_hw_s *self)
> > +{
> > +	struct hw_atl2_priv *priv = self->priv;
> > +	struct filter_caps_s filter_caps;
> > +	u32 tag_top;
> > +	int err;
> > +
> > +	err = hw_atl2_shared_buffer_read_safe(self, filter_caps, &filter_caps);
> > +	if (err)
> > +		return err;
> > +
> > +	priv->art_base_index = filter_caps.rslv_tbl_base_index * 8;
> > +	priv->art_count = filter_caps.rslv_tbl_count * 8;
> > +	if (priv->art_count == 0)
> > +		priv->art_count = 128;
> > +	priv->l2_filters_base_index = filter_caps.l2_filters_base_index;
> > +	priv->l2_filter_count = filter_caps.l2_filter_count;
> > +	priv->etype_filter_base_index = filter_caps.ethertype_filter_base_index;
> > +	priv->etype_filter_count = filter_caps.ethertype_filter_count;
> 
> Should priv->l2_filter_count and priv->etype_filter_count also be
> clamped against their backing arrays the way the v4/L4 paths below
> are?  The etype_policy[] array is sized HW_ATL2_RPF_ETYPE_FILTERS
> (16), and the base indices are accepted verbatim from firmware
> without any check that base_index + count stays within the array.
>

etype_filter_count is only used to compute etype_filter_tag_top, which
is always clamped to HW_ATL2_RPF_ETYPE_TAGS or HW_ATL2_RPF_ETYPE_TAGS >> 1
regardless of the firmware value — no array index is derived from it.  l2_filter_count
is similarly not used as an array index anywhere in this series. No clamping is needed
for either field.

> > +	priv->etype_filter_tag_top =
> > +		(priv->etype_filter_count >= HW_ATL2_RPF_ETYPE_TAGS) ?
> > +		 (HW_ATL2_RPF_ETYPE_TAGS) : (HW_ATL2_RPF_ETYPE_TAGS >> 1);
> > +	priv->vlan_filter_base_index = filter_caps.vlan_filter_base_index;
> > +	/* 0 - no tag, 1 - reserved for vlan-filter-offload filters */
> > +	tag_top =
> > +		  (filter_caps.vlan_filter_count == HW_ATL2_RPF_VLAN_FILTERS) ?
> > +		  (HW_ATL2_RPF_VLAN_FILTERS - 2) :
> > +		  (HW_ATL2_RPF_VLAN_FILTERS / 2 - 2);
> > +
> > +	if (filter_caps.vlan_filter_count > 2)
> > +		priv->vlan_filter_count = min_t(u32,
> > +						filter_caps.vlan_filter_count - 2,
> > +						tag_top);
> > +	else
> > +		priv->vlan_filter_count = 0;
> > +
> > +	priv->l3_v4_filter_base_index = filter_caps.l3_ip4_filter_base_index;
> > +	priv->l3_v4_filter_count = min_t(u32, filter_caps.l3_ip4_filter_count,
> > +					 HW_ATL2_RPF_L3V4_FILTERS - 1);
> > +	priv->l3_v6_filter_base_index = filter_caps.l3_ip6_filter_base_index;
> > +	priv->l3_v6_filter_count = filter_caps.l3_ip6_filter_count;
> > 
> Can priv->l3_v6_filter_count overflow l3_v6_filters[]?  The l3_v4 and
> l4 counts just above/below are clamped:
> 
>     priv->l3_v4_filter_count = min_t(u32, filter_caps.l3_ip4_filter_count,
>                                      HW_ATL2_RPF_L3V4_FILTERS - 1);
>    ...
>     priv->l4_filter_count = min_t(u32, filter_caps.l4_filter_count,
>                                   HW_ATL2_RPF_L4_FILTERS - 1);
> 
> but l3_v6_filter_count takes filter_caps.l3_ip6_filter_count directly.
> The backing array is
> 
>     struct hw_atl2_l3_filter l3_v6_filters[HW_ATL2_RPF_L3L4_FILTERS];
> 
> and HW_ATL2_RPF_L3L4_FILTERS is 8.  The l3_ip6_filter_count field in
> struct filter_caps_s is wider than 3 bits, so the firmware can report
> a count above 8.
> 
> The consumer added in the follow-up patch in this series then walks
> priv->l3_v6_filters[] using the unclamped count:
> 
>     first = priv->l3_v6_filter_base_index;
>     last  = priv->l3_v6_filter_base_index + priv->l3_v6_filter_count;
>     for (i = first; i < last; i++)
>         if (hw_atl2_rxf_l3_is_equal(&l3_filters[i], l3))
>             return i;
> 
> and hw_atl2_rxf_l3_get() writes through &l3_filters[idx]
> (l3->usage++, l3->cmd = ...).  If firmware reports an oversized
> l3_ip6_filter_count, or any non-zero base_index that pushes the
> window past slot 7, doesn't this read and write past
> priv->l3_v6_filters[] into the adjacent priv fields?
> 
> The same question applies to priv->l3_v4_filter_base_index and
> priv->l3_v6_filter_base_index — they are stored verbatim from
> firmware with no check that base_index + count stays within the
> corresponding backing array.  Would it make sense to validate the
> full window for every filter category?
> 

You are correct — the missing clamp on l3_v6_filter_count was a bug.
Fixed in v3: all three filter categories (l3_v4, l3_v6, l4) now apply the
same validation — 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.

> +	priv->l4_filter_base_index = filter_caps.l4_filter_base_index;
> +	priv->l4_filter_count = min_t(u32, filter_caps.l4_filter_count,
> +				      HW_ATL2_RPF_L4_FILTERS - 1);
> +
> +	return 0;
> +}
> -- 
> This is an AI-generated review.