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

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

[Janne Grunau]

hw_atl2_hw_packet_filter_set() wraps hw_atl_b0_hw_packet_filter_set()
which has no concept of `l2_filters_base_index` and enables/disables
filters from `HW_ATL_B0_MAC_MIN` (1) to `HW_ATL_B0_MAC_MAX` (33). After
commit 6c1ca112a6e4 ("net: atlantic: fix AQC113 HW init: ART, L2 filter
slot, MAC address") this may result in disabling filters set in
hw_atl2.c including the l2 filter for the MAC address.
The AQC113 adapters integrated in some Apple silicon desktop systems
report 9 as l2_filters_base_index which results in broken network.
The l2 filters set in hw_atl2.c are disabled after
hw_packet_filter_set() calls.
Pass the base location to hw_atl_b0_hw_packet_filter_set() to ensure it
neither touches reserved firmware filters or disables filters configured
in hw_atl2.c.

Fixes: 6c1ca112a6e4 ("net: atlantic: fix AQC113 HW init: ART, L2 filter slot, MAC address")
Cc: stable@vger.kernel.org
Signed-off-by: Janne Grunau <j@jannau.net>
---
 drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c | 14 +++++++++++---
 drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.h |  6 ++++--
 drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c  |  5 ++++-
 3 files changed, 19 insertions(+), 6 deletions(-)

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 6c25ad264b19..5c67edf8030c 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
@@ -1038,8 +1038,9 @@ int hw_atl_b0_hw_irq_read(struct aq_hw_s *self, u64 *mask)
 
 #define IS_FILTER_ENABLED(_F_) ((packet_filter & (_F_)) ? 1U : 0U)
 
-int hw_atl_b0_hw_packet_filter_set(struct aq_hw_s *self,
-				   unsigned int packet_filter)
+int hw_atl_b0_hw_packet_filter_set_int(struct aq_hw_s *self,
+				       unsigned int base_location,
+				       unsigned int packet_filter)
 {
 	struct aq_nic_cfg_s *cfg = self->aq_nic_cfg;
 	unsigned int i = 0U;
@@ -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)) ?
@@ -1076,6 +1077,13 @@ int hw_atl_b0_hw_packet_filter_set(struct aq_hw_s *self,
 
 #undef IS_FILTER_ENABLED
 
+static int hw_atl_b0_hw_packet_filter_set(struct aq_hw_s *self,
+					  unsigned int packet_filter)
+{
+	return hw_atl_b0_hw_packet_filter_set_int(self, HW_ATL_B0_MAC_MIN,
+						  packet_filter);
+}
+
 static int hw_atl_b0_hw_multicast_list_set(struct aq_hw_s *self,
 					   u8 ar_mac
 					   [AQ_HW_MULTICAST_ADDRESS_MAX]
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.h b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.h
index 5298846dd9f7..9ea469d3330f 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.h
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.h
@@ -69,7 +69,9 @@ int hw_atl_b0_hw_irq_enable(struct aq_hw_s *self, u64 mask);
 int hw_atl_b0_hw_irq_disable(struct aq_hw_s *self, u64 mask);
 int hw_atl_b0_hw_irq_read(struct aq_hw_s *self, u64 *mask);
 
-int hw_atl_b0_hw_packet_filter_set(struct aq_hw_s *self,
-				   unsigned int packet_filter);
+int hw_atl_b0_hw_packet_filter_set_int(struct aq_hw_s *self,
+				       unsigned int base_location,
+				       unsigned int packet_filter);
+
 
 #endif /* HW_ATL_B0_H */
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 25dd7150aaaf..82be3bb2e76d 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,12 @@ 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 hw_atl2_priv *priv = self->priv;
+	u32 base_location = priv->l2_filters_base_index + 1;
 	hw_atl2_hw_new_rx_filter_promisc(self, IS_FILTER_ENABLED(IFF_PROMISC));
 
-	return hw_atl_b0_hw_packet_filter_set(self, packet_filter);
+	return hw_atl_b0_hw_packet_filter_set_int(self, base_location,
+						  packet_filter);
 }
 
 #undef IS_FILTER_ENABLED

-- 
2.56.0

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

[netdev-bot+sashiko]

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