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

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.com>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
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?

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

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

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

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