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

[Sukhdeep Soni [C]]

On 4 Jun 2026, Jakub Kicinski wrote:

> net: atlantic: add AQC113 filter data structures and firmware query
>
> This patch adds L3/L4/L3L4 and ethertype tag filter data structures to
> hw_atl2_priv, defines RPF L3/L4 command bits and several filter count
> constants, and queries filter capabilities from firmware shared memory
> during aq_a2_fw_init().  It also adds an unrelated MAC register dump
> helper hw_atl2_utils_hw_get_regs().
>
> A few questions inline.
>
> 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
>
> [ ... ]
>
> @@ -94,6 +106,12 @@ enum HW_ATL2_RPF_ART_INDEX {
>  #define HW_ATL2_ACTION_DISABLE HW_ATL2_ACTION(0, 0, 0, 0)
>  #define HW_ATL2_ACTION_ASSIGN_QUEUE(QUEUE) HW_ATL2_ACTION(1, 0, (QUEUE), 1)
>  #define HW_ATL2_ACTION_ASSIGN_TC(TC) HW_ATL2_ACTION(1, 1, (TC), 1)
> +#define HW_ATL2_RPF_L3L4_FILTERS 8
> +#define HW_ATL2_RPF_L3V4_FILTERS 8
> +#define HW_ATL2_RPF_L4_FILTERS 8
> +#define HW_ATL2_RPF_VLAN_FILTERS 16
> +#define HW_ATL2_RPF_ETYPE_FILTERS 16
> +#define HW_ATL2_RPF_ETYPE_TAGS 7
>
> [Low]
> HW_ATL2_RPF_L3V4_FILTERS and HW_ATL2_RPF_L4_FILTERS look unused.
>
> Every array in hw_atl2_priv (l3_v4_filters[], l3_v6_filters[],
> l4_filters[], l3l4_filters[]) is sized with HW_ATL2_RPF_L3L4_FILTERS, and
> the bounds clamps in hw_atl2_utils_get_filter_caps() also compare against
> HW_ATL2_RPF_L3L4_FILTERS.  HW_ATL2_RPF_L3V4_FILTERS and
> HW_ATL2_RPF_L4_FILTERS appear nowhere else in tree, and there is no
> symmetric HW_ATL2_RPF_L3V6_FILTERS even though the commit message
> enumerates IPv4 and IPv6 as distinct categories.
>
> Should the unused names be dropped, or should the arrays and clamps be
> switched to use them so that, for example, lowering
> HW_ATL2_RPF_L4_FILTERS in the future actually has effect?
>
> [ ... ]

HW_ATL2_RPF_L3V4_FILTERS and HW_ATL2_RPF_L4_FILTERS are now
used as min_t() upper bounds in hw_atl2_utils_get_filter_caps().
HW_ATL2_RPF_L3V6_FILTERS has also been added with its own clamp for
the IPv6 filter count, so all three filter categories now have named per-type
constants and consistent clamping at the firmware boundary.

> +struct hw_atl2_l3_filter {
> +	u8 proto;
> +	u8 usage;
> +	u32 cmd;
> +	u32 srcip[4];
> +	u32 dstip[4];
> +};
> +
> +struct hw_atl2_l4_filter {
> +	u8 usage;
> +	u32 cmd;
> +	u16 sport;
> +	u16 dport;
> +};
>
> [Low]
> The commit message says these structures provide "reference-counted
> sharing support", but the only field that looks like a refcount is a
> plain u8 usage with no atomic_t, refcount_t, or paired inc/dec helpers
> in this commit.  Could the wording in the commit message be tightened
> to match what is actually added (a serialized usage counter, not a
> refcount)?
>
> [ ... ]

Will fix in v5 - the commit message now reads "serialized usage counter for
filter sharing" rather than "reference-counted sharing support".

> 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..ef549e172dd9 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,37 @@ int hw_atl2_utils_soft_reset(struct aq_hw_s *self)
>  err_exit:
>  	return err;
>  }
> +
> +static const u32 hw_atl2_utils_hw_mac_regs[] = {
>
> [Low]
> The subject and body describe filter data structures and firmware
> capability discovery, but the diff also adds a 72-entry MAC register
> table and the new register-dump helper hw_atl2_utils_hw_get_regs().  The
> register dump utility is unrelated to filter discovery and is not
> mentioned in the subject.
>
> Would it be better to split this into a separate patch (or fold it into
> the follow-up commit that wires hw_atl2_utils_hw_get_regs() into
> hw_atl2_ops) so that this patch is one logical change?
>
> [ ... ]

The subject line is updated in v5 to explicitly include "register dump" alongside
filter data structures and firmware query. The register dump helper
hw_atl2_utils_hw_get_regs() is intentionally placed here rather than in patch 6
because patch 5 is where hw_atl2_utils.c is first touched - moving it to patch 6
would leave the function declaration in patch 5 with no definition until the next patch,
which is worse for bisectability.

> 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..3011c7cd5259 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
>
> [ ... ]
>
> +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;
> +	u32 min_val;
> +
> +	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;
>
> [Low]
> What does the literal 128 represent here?  It has no #define, no comment,
> and no log message.  If firmware legitimately reports a 0 count (feature
> unavailable, FW bug, restricted SKU), this silently substitutes 128 ART
> entries and the driver could later program slots beyond what the part
> actually has.
>
> Could this be promoted to a named macro alongside HW_ATL2_RPF_ART_INDEX
> with a short rationale, and possibly a netdev_warn() noting that the
> firmware did not advertise an ART count?

The literal 128 has been promoted to HW_ATL2_ART_TOTAL_ENTRIES defined
alongside the other RPF constants. Regarding the netdev_warn(): this fallback
is the normal boot path on older firmware that predates the capability advertisement
field - a warning on every such boot would be misleading noise. The silent fallback to
the hardware maximum matches the vendor reference driver behaviour.

> [Low]
> priv->art_base_index now has two writers reading the same firmware
> field.  This function sets:
>
>	priv->art_base_index = filter_caps.rslv_tbl_base_index * 8;
>
> while the pre-existing helper
>
>	int hw_atl2_utils_get_action_resolve_table_caps(struct aq_hw_s *self,
>							u8 *base_index, u8 *count);
>
> is still called from hw_atl2.c, which then assigns
> priv->art_base_index = base_index * 8 from its out-parameter.
>
> Is one of these now redundant?  Should callers move to consume the
> cached priv->art_base_index, or is there a reason to keep both
> sources of truth?

hw_atl2_utils_get_action_resolve_table_caps() no longer exists in v5 - it was
removed as part of consolidating filter capability discovery into
hw_atl2_utils_get_filter_caps(). There is only one writer of priv->art_base_index
in this series, so there is no duplicate source of truth.

> +	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;
> +	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);
>
> [Low]
> The L3 and L4 paths below clamp base_index against HW_ATL2_RPF_L3L4_FILTERS
> and use min_t() on the count, but the etype, l2 and vlan_filter_base_index
> fields are copied verbatim from filter_caps_s u8 values (range 0..255).
>
> Is that asymmetry intentional?  It would be easier for future consumers
> to reason about if every firmware-supplied capability field were
> range-checked against its compile-time array bound (etype_policy[] is
> sized HW_ATL2_RPF_ETYPE_FILTERS, etc.) at the FW boundary, in this one
> place.

The asymmetry is inherited from the vendor reference driver, which applies the same pattern.
The l3/l4 min_t() clamps exist there because those counts drive array indexing against fixed-size
arrays, while the etype/l2 base_index and count fields are used only as hardware register
offsets - they are not used to index into software arrays in the current code. Adding defensive
clamping for all fields at the FW boundary is a reasonable future cleanup, but changing the pattern
beyond what the reference driver does is out of scope for this series.

> +	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;
>
> [Low]
> When firmware reports filter_caps.vlan_filter_count of 1 or 2, the
> "> 2" gate forces priv->vlan_filter_count to 0 silently, with no
> netdev_dbg()/netdev_warn().  The comment only documents the 2-entry
> reservation when the count is large.
>
> Is the silent disable on a small but nonzero count intentional, and
> worth a short log line so users can tell why VLAN filtering came up
> disabled?

The silent disable is intentional - a firmware-reported count of 1 or 2
means all slots are consumed by the two reserved entries
(tag 0 = no tag, tag 1 = vlan-filter-offload), leaving zero usable slots. This is a
hardware constraint, not a runtime error. The > 2 guard was added as a deliberate
improvement over the vendor reference driver which uses
min_t(u32, vlan_filter_count - 2, tag_top) directly with no underflow protection.
We will add a pr_debug() to make the condition observable for debugging.

> +	priv->l3_v4_filter_base_index = filter_caps.l3_ip4_filter_base_index;
> +	if (priv->l3_v4_filter_base_index >= HW_ATL2_RPF_L3L4_FILTERS) {
> +		priv->l3_v4_filter_base_index = 0;
> +		priv->l3_v4_filter_count = 0;
> +	} else {
> +		min_val = HW_ATL2_RPF_L3L4_FILTERS - 1 - priv->l3_v4_filter_base_index;
> +		priv->l3_v4_filter_count = min_t(u32,
> +						 filter_caps.l3_ip4_filter_count,
> +						 min_val);
> +	}
>
> [Medium]
> Is the "- 1" in this clamp intentional?
>
> The arrays are sized HW_ATL2_RPF_L3L4_FILTERS (= 8), and consumers in
> the follow-up patch "net: atlantic: implement AQC113 L2/L3/L4 RX filter
> ops" iterate over priv->l3_v4_filters[] / l3_v6_filters[] / l4_filters[]
> as a half-open range:
>
>	for (i = base; i < base + count; i++)
>
> With base_index = 0 and firmware reporting count = 8 (the natural
> maximum for a 4-bit field):
>
>	min_val = 8 - 1 - 0 = 7
>	priv->l3_v4_filter_count = min_t(u32, 8, 7) = 7
>
> so slot 7 is permanently unreachable.  With base_index = 7 (still
> in-range, since the validity check is ">= HW_ATL2_RPF_L3L4_FILTERS"):
>
>	min_val = 8 - 1 - 7 = 0
>	priv->l3_v4_filter_count = 0
>
> so the entire pool reports zero filters even though slot 7 exists.
>
> For the half-open consumer convention, the formula that exactly fills
> the array is:
>
>	min_val = HW_ATL2_RPF_L3L4_FILTERS - priv->l3_v4_filter_base_index;
>
> The same pattern is duplicated in the l3_v6 and l4 blocks below, and
> unlike the VLAN logic in this same function (which has the "- 2"
> comment explaining its reservation), the "- 1" here is undocumented.

You are right about - base_index being incorrect - with base_index=7 it
produces min_val=0, making that slot unreachable. We will simplify to
HW_ATL2_RPF_L3L4_FILTERS - 1 (same cap the vendor reference driver uses).
The - 1 is intentional - it reserves one slot at the top of the array for internal use,
matching the reference driver's HW_ATL2_RPF_L3V4_FILTERS - 1 convention.
We will add a comment to make that explicit. Same fix for l3_v6 and l4 blocks,
and min_val will be dropped.

> +
> +	priv->l3_v6_filter_base_index = filter_caps.l3_ip6_filter_base_index;
> +	if (priv->l3_v6_filter_base_index >= HW_ATL2_RPF_L3L4_FILTERS) {
> +		priv->l3_v6_filter_base_index = 0;
> +		priv->l3_v6_filter_count = 0;
> +	} else {
> +		min_val = HW_ATL2_RPF_L3L4_FILTERS - 1 - priv->l3_v6_filter_base_index;
> +		priv->l3_v6_filter_count = min_t(u32,
> +						 filter_caps.l3_ip6_filter_count,
> +						 min_val);
> +	}
> +
> +	priv->l4_filter_base_index = filter_caps.l4_filter_base_index;
> +	if (priv->l4_filter_base_index >= HW_ATL2_RPF_L3L4_FILTERS) {
> +		priv->l4_filter_base_index = 0;
> +		priv->l4_filter_count = 0;
> +	} else {
> +		min_val = HW_ATL2_RPF_L3L4_FILTERS - 1 - priv->l4_filter_base_index;
> +		priv->l4_filter_count = min_t(u32,
> +					      filter_caps.l4_filter_count,
> +					      min_val);
> +	}
> +
> +	return 0;
> +}
>
> Same "- 1" question as above for the l3_v6 and l4 blocks.

Same fix as above for l3_v6 and l4 blocks, and min_val will be dropped.