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

[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: 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?

[ ... ]

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

[ ... ]

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

[ ... ]

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

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

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

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

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

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