Re: [PATCH v2 1/2] net: atlantic: AQC113: Fix hw_atl2_hw_acket_filter_set for L2 filter base index > 0

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 2 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Off-by-one in the slot index of the new enable/disable loop in
  hw_atl2_hw_packet_filter_set()…
- [Medium] hw_atl2_hw_packet_filter_set() can write past the end of the
  38-entry L2 unicast filter table.
- [Medium] hw_atl2_hw_packet_filter_set() now bounds its secondary-filter
  loop by priv->l2_filter_count.
- [Low] The commit message does not match the change.
- [Low] After this patch, hw_atl_b0_hw_packet_filter_set() has no
  external user.

--

net: atlantic: AQC113: Fix hw_atl2_hw_acket_filter_set for L2 filter base index > 0

This patch replaces the call from hw_atl2_hw_packet_filter_set() to
hw_atl_b0_hw_packet_filter_set() with an open-coded copy. The copy takes
l2_filters_base_index into account, so it no longer disables the L2
filters that hw_atl2.c programs, including the one for the MAC address.
This fixes broken networking on Apple silicon systems whose firmware
reports l2_filters_base_index = 9.

> net: atlantic: AQC113: Fix hw_atl2_hw_acket_filter_set for L2 filter base index > 0

[ ... ]

> Copy the formerly wrapped `hw_atl_b0_hw_packet_filter_set()` to handle
> this additional complexity.

[Severity: Low]
This isn't a bug, but there's a typo in the subject line
(hw_atl2_hw_acket_filter_set).

Also, the new code is not a plain copy of hw_atl_b0_hw_packet_filter_set().
The loop bound used to be the fixed HW_ATL_B0_MAC_MIN..HW_ATL_B0_MAC_MAX
range. It now depends on the l2_filter_count that firmware reports. The
slot index is also rebased onto l2_filters_base_index. Both changes affect
AQC113 systems with a base index of 0 too.

Could the commit message mention these differences?

> 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 25dd7150aaaf2..8d7a489aa5de4 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
> @@ -759,9 +759,42 @@ static int hw_atl2_hw_ring_tx_init(struct aq_hw_s *self,
>  static int hw_atl2_hw_packet_filter_set(struct aq_hw_s *self,
>  					unsigned int packet_filter)
>  {
> +	struct aq_nic_cfg_s *cfg = self->aq_nic_cfg;
> +	struct hw_atl2_priv *priv = self->priv;
> +	u32 base_location = priv->l2_filters_base_index + 1;
> +	unsigned int num_l2_filters = min_t(unsigned int, HW_ATL2_MAC_MAX,
> +					    priv->l2_filter_count);

[Severity: Medium]
What happens here if firmware reports l2_filter_count == 0? Then
num_l2_filters is 0 and the loop further down never runs. The secondary
filter enables would stop following IFF_MULTICAST and mc_list_count.

For example, clearing IFF_MULTICAST when there are no secondary unicast
addresses would leave enabled the slots that
hw_atl2_hw_multicast_list_set() wrote.

hw_atl2_utils_get_filter_caps() gives a zero art_count a default, but
l2_filter_count gets no fallback:

hw_atl2_utils_get_filter_caps() {
    ...
	if (priv->art_count == 0)
		priv->art_count = HW_ATL2_ART_TOTAL_ENTRIES;
    ...
	priv->l2_filter_count = filter_caps.l2_filter_count;
    ...
}

The b0 loop covered slots 1..32 no matter what the firmware reported.

Later in the series this gets worse. hw_atl2_get_num_multicast_addresses()
returns min_t(int, l2_filter_count - 1, 32), which is -1 when the count is
0. The num_multicast_addresses check in aq_nic_set_multicast_list() is an
unsigned comparison, so -1 becomes UINT_MAX. Wouldn't the check then never
fire, so that every multicast address is copied into the 32 entry
mc_list.ar?

> +	unsigned int i = 0U;
> +	u32 vlan_promisc;
> +	u32 l2_promisc;
> +
>  	hw_atl2_hw_new_rx_filter_promisc(self, IS_FILTER_ENABLED(IFF_PROMISC));
>  
> -	return hw_atl_b0_hw_packet_filter_set(self, packet_filter);

[Severity: Low]
This isn't a bug, but once this call is removed,
hw_atl_b0_hw_packet_filter_set() is not used anywhere outside
hw_atl_b0.c. Could it be made static again, and its prototype dropped
from hw_atl_b0.h?

[ ... ]

> +	hw_atl_rpfl2broadcast_en_set(self, IS_FILTER_ENABLED(IFF_BROADCAST));
> +
> +	for (i = HW_ATL2_MAC_MIN; i < num_l2_filters; ++i)
> +		hw_atl_rpfl2_uc_flr_en_set(self,
> +					   (cfg->is_mc_list_enabled &&
> +					    (i <= cfg->mc_list_count)) ?
> +					   1U : 0U, base_location + i);

[Severity: High]
Is the slot index off by one here? base_location already equals
l2_filters_base_index + 1, and the loop starts at HW_ATL2_MAC_MIN (1).
The loop therefore manages slots base + 2 .. base + num_l2_filters. It
enables slots base + 2 .. base + 1 + mc_list_count.

hw_atl2_hw_multicast_list_set() writes entry k (counting from 0) to
base_location + k:

hw_atl2_hw_multicast_list_set() {
    ...
	u32 base_location = priv->l2_filters_base_index + 1;
    ...
		u32 i = cfg->mc_list_count;
    ...
		hw_atl_rpfl2unicast_dest_addresslsw_set(self, l,
							base_location + i);
    ...
}

Its entries therefore occupy base + 1 .. base + mc_list_count. In the b0
original, the enable loop used slot i for 1 <= i <= mc_list_count, which
matches the slots its own list_set writes.

With the new indexing, nothing here ever clears slot base + 1. When the
address count drops to 0, aq_nic_set_multicast_list() skips
hw_multicast_list_set() and only calls aq_nic_set_packet_filter(). That
happens when the last secondary unicast address or multicast group is
removed, or when multicast is turned off. Would the NIC keep accepting the
stale first secondary address in those cases?

The loop also enables slot base + 1 + mc_list_count, which was not
programmed in this round. If the list shrinks from N to M entries, that
slot still holds the old entry M. Can the hardware keep accepting an
address the stack has already removed?

This happens for every l2_filters_base_index, including 0. AQC113 boards
that worked before could regress, and Cc: stable would carry that to
them. Should the index be priv->l2_filters_base_index + i instead? The
final patch of the series doesn't seem to change this loop.

[Severity: Medium]
Can this loop also write past the end of the 38 entry L2 unicast filter
table? The highest slot it touches is
l2_filters_base_index + min(HW_ATL2_MAC_MAX, l2_filter_count). Nothing
checks that value against HW_ATL2_MAC_MAX, and
hw_atl2_utils_get_filter_caps() clamps only the base index:

hw_atl2_utils_get_filter_caps() {
    ...
	priv->l2_filters_base_index = filter_caps.l2_filters_base_index;
	if (priv->l2_filters_base_index >= HW_ATL2_MAC_MAX)
		priv->l2_filters_base_index = HW_ATL2_MAC_UC;
	priv->l2_filter_count = filter_caps.l2_filter_count;
    ...
}

The commit message of the next patch in the series ("net: atlantic:
AQC113: Limit multicast filters to available slots") says Apple firmware
reports base 9 and count 29. With those values the loop reaches slot 38.
Each call then does a read-modify-write of bit 31 at
HW_ATL_RPFL2UC_ENF_ADR(38) = 0x5244, which is beyond the last table entry
at 0x523C.

Firmware that reports a larger count with a nonzero base would go further,
into HW_ATL_RPFL2MC_ENF_ADR(1) at 0x5254 and the VLAN TPID register at
0x5284. Bit 31 of those registers would then follow mc_list_count. The b0
loop never went beyond slot 32.

Should the bound also enforce
l2_filters_base_index + l2_filter_count <= HW_ATL2_MAC_MAX?

> +
> +	return aq_hw_err_from_flags(self);
>  }
>  
>  #undef IS_FILTER_ENABLED

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108-net-atlantic-aq113-apple-l2-filter-slots-v2-0-0b988d031c78%40jannau.net