Re: [PATCH 1/2] net: atlantic: AQC113: Fix hw_packet_filter_set for l2 filter base index > 0

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0

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] The fix is incomplete because the two functions use different
  slot numbering.

Pre-existing issues:
- [Medium] On the ATL2 path, hw_atl_b0_hw_packet_filter_set_int() still
  stops at HW_ATL_B0_MAC_MAX (33), but the AQC113 L2 filter table has…

--

net: atlantic: AQC113: Fix hw_packet_filter_set for l2 filter base index > 0

This adds a base_location argument to the B0 packet filter helper, now
hw_atl_b0_hw_packet_filter_set_int(). With it, the unicast filter enable
loop can start at a slot other than HW_ATL_B0_MAC_MIN.

The ATL2 wrapper passes l2_filters_base_index + 1. On AQC113 parts with a
non-zero base index, the loop then no longer touches the reserved firmware
slots or the MAC address filter.

> diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
> index 6c25ad264b19d..5c67edf8030c7 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c

[ ... ]

> @@ -1065,7 +1066,7 @@ int hw_atl_b0_hw_packet_filter_set(struct aq_hw_s *self,
>  	hw_atl_rpfl2broadcast_en_set(self, IS_FILTER_ENABLED(IFF_BROADCAST));
>  
>  
> -	for (i = HW_ATL_B0_MAC_MIN; i < HW_ATL_B0_MAC_MAX; ++i)
> +	for (i = base_location; i < HW_ATL_B0_MAC_MAX; ++i)
>  		hw_atl_rpfl2_uc_flr_en_set(self,
>  					   (cfg->is_mc_list_enabled &&
>  					    (i <= cfg->mc_list_count)) ?

[Severity: High]
Now that base_location can differ from HW_ATL_B0_MAC_MIN, is the
i <= cfg->mc_list_count test still correct? Here i is an absolute slot
number that starts at base_location. cfg->mc_list_count is just a count
of entries.

hw_atl2_hw_multicast_list_set() programs and enables entries at
base_location + i, for i in 0..count-1:

hw_atl2.c:hw_atl2_hw_multicast_list_set() {
    u32 base_location = priv->l2_filters_base_index + 1;
    ...
		hw_atl_rpfl2_uc_flr_en_set(self, (cfg->is_mc_list_enabled),
					   base_location + i);
    ...
}

On the AQC113 with l2_filters_base_index = 9, base_location is 10:

- With mc_list_count = 3, i <= 3 is false for every i >= 10. Slots
  10..12 are disabled right after being enabled.

- With mc_list_count = n >= 10, slots 10..n stay enabled. The last 9
  programmed entries (n+1..n+9) are disabled.

aq_nic_set_multicast_list() calls hw_multicast_list_set() and then
hw_packet_filter_set() straight away:

aq_nic_set_multicast_list()
    hw_ops->hw_multicast_list_set()        /* enables slots 10.. */
    aq_nic_set_packet_filter()
        hw_atl2_hw_packet_filter_set()
            hw_atl_b0_hw_packet_filter_set_int()   /* disables them */

aq_nic_start() calls them in the same order. So this happens on every
interface start and every rx-mode change.

Unless ALLMULTI or PROMISC is set, hw_atl2_hw_new_rx_filter_promisc()
programs the ART to drop frames that have no UC/ALLMC tag.

Would that drop IPv6 solicited-node and all-nodes multicast, other joined
multicast groups, and secondary unicast MACs such as macvlan?

The commit message says the patch makes sure packet_filter_set:

  "neither touches reserved firmware filters or disables filters configured
  in hw_atl2.c."

These are filters configured in hw_atl2.c. The second patch in the series
does not seem to change this loop either.

Would a relative test such as i < base_location + cfg->mc_list_count fix
this? On B0, base_location == HW_ATL_B0_MAC_MIN, so behaviour there would
not change.

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. The old
loop had the same B0 upper bound, and the shifted slot layout came from
6c1ca112a6e4.

Should the upper bound still be HW_ATL_B0_MAC_MAX (33) when this runs for
ATL2? The AQC113 L2 filter table has HW_ATL2_MAC_MAX (38) entries, and
hw_atl2_hw_init_rx_path() walks all 38.

With base_location = 10, a list of 24 or more entries makes
hw_atl2_hw_multicast_list_set() enable slot 33 or higher. The bound later
in the series is count <= l2_filter_count - 1.

This loop never reaches slots 33..37. hw_atl2_hw_multicast_list_set() only
rewrites the slots in the current list.

The list can shrink, IFF_MULTICAST can be cleared, or the count can drop
to 0, in which case aq_nic_set_multicast_list() skips
hw_multicast_list_set(). In any of these cases, can the enable bits in
slots 33..37 stay set? The NIC would then keep accepting frames for
addresses that are no longer subscribed.

The new helper only takes a start index. Could it also take an end or a
count, for example bounded by priv->l2_filter_count?

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