[PATCH v2 0/2] net: atlantic: AQC113: Fix regression on Apple silicon systems

[PATCH v2 0/2] net: atlantic: AQC113: Fix regression on Apple silicon systems

[Janne Grunau]

Hej,

commit 6c1ca112a6e4 ("net: atlantic: fix AQC113 HW init: ART, L2 filter
slot, MAC address") broke data transmission on Apple silicon systems
with integrated AQC113. The commit uses the queried filter caps from the
firmware which may result in using L2 filter slots starting from
`l2_filters_base_index`.
hw_atl_b0_hw_packet_filter_set() is not aware of this change and enables
/disables L2 filter slots starting from index 0. When the firmware
reported `l2_filters_base_index` is high enough (9 on Apple systems) it
will likely disable the L2 filter for the NIC's own MAC address
resulting in a complete loss of data transmission.
Another issue is that the number of available L2 filter slots might
smaller than AQ_HW_MULTICAST_ADDRESS_MAX (32). This is the case on Apple
systems with `l2_filter_count` == 29. To avoid -EBADRQC when L2 filters
run out query the number of filter slots from the hardware.

The fixes in this series restores Ethernet data transmission on an Apple
M1 Max Mac Studio and supersedes my previous "fix" in [1].

Link:
https://lore.kernel.org/r/20261024-net-atlantic-aq113-apple-regression-v1-1-b8305415a8b1@jannau.net [1]

Signed-off-by: Janne Grunau <j@jannau.net>
---
Changes in v2:
- Leave atl_b0 as is and copy hw_atl_b0_hw_packet_filter_set to hw_atl2
  to adapt it
- Enable L2 filters for multicast addresses correctly
- Query the number of available L2 filter slots from HW to avoid
  -EBADRQC if it exceeds `l2_filter_count - 1` and use promiscuous mode
  instead in aq_nic_set_multicast_list()
- Link to v1: https://lore.kernel.org/r/20261026-net-atlantic-aq113-apple-l2-filter-slots-v1-0-d72e99b69583@jannau.net

---
Janne Grunau (2):
      net: atlantic: AQC113: Fix hw_atl2_hw_acket_filter_set for L2 filter base index > 0
      net: atlantic: AQC113: Limit multicast filters to available slots

 drivers/net/ethernet/aquantia/atlantic/aq_hw.h     |  2 +
 drivers/net/ethernet/aquantia/atlantic/aq_nic.c    |  7 ++--
 .../ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c  |  7 ++++
 .../ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c  |  7 ++++
 .../ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c   | 46 +++++++++++++++++++++-
 5 files changed, 64 insertions(+), 5 deletions(-)
---
base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
change-id: 20261026-net-atlantic-aq113-apple-l2-filter-slots-6342515c5318

Best regards,
-- 
Janne Grunau <j@jannau.net>

[PATCH v2 1/2] net: atlantic: AQC113: Fix hw_atl2_hw_acket_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. This results in
broken networking on Apple silicon systems with AQC113. The firmware in
this systems report `l2_filters_base_index = 9`.
Copy the formerly wrapped `hw_atl_b0_hw_packet_filter_set()` to handle
this additional complexity.

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>
---
 .../ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c   | 35 +++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

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..8d7a489aa5de 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);
+	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);
+	l2_promisc = IS_FILTER_ENABLED(IFF_PROMISC) ||
+		     !!(cfg->priv_flags & BIT(AQ_HW_LOOPBACK_DMA_NET));
+	vlan_promisc = l2_promisc || cfg->is_vlan_force_promisc;
+
+	hw_atl_rpfl2promiscuous_mode_en_set(self, l2_promisc);
+
+	hw_atl_rpf_vlan_prom_mode_en_set(self, vlan_promisc);
+
+	hw_atl_rpfl2multicast_flr_en_set(self,
+					 IS_FILTER_ENABLED(IFF_ALLMULTI) &&
+					 IS_FILTER_ENABLED(IFF_MULTICAST), 0);
+
+	hw_atl_rpfl2_accept_all_mc_packets_set(self,
+					       IS_FILTER_ENABLED(IFF_ALLMULTI) &&
+					       IS_FILTER_ENABLED(IFF_MULTICAST));
+
+	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);
+
+	return aq_hw_err_from_flags(self);
 }
 
 #undef IS_FILTER_ENABLED

-- 
2.56.0

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

[netdev-bot+sashiko]

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

[PATCH v2 2/2] net: atlantic: AQC113: Limit multicast filters to available slots

[Janne Grunau]

Since commit 6c1ca112a6e4 ("net: atlantic: fix AQC113 HW init: ART,
L2 filter slot, MAC address") hw_atl2_hw_multicast_list_set() uses L2
filter slot above the firmware-assigned L2 filter slot for the MAC
address.
On Apple silicon systems with AQC113 the firmware returns
`l2_filters_base_index = 9` and `l2_filter_count = 29` which is
unfortunately less than AQ_HW_MULTICAST_ADDRESS_MAX (32).
Add a new `struct aq_hw_ops` function pointer
hw_get_num_multicast_addresses() so that atl2 variants can report the
lower number of available L2 filter slots. Use this potentially smaller
number in aq_nic_set_multicast_list() to decide when to switch to
promiscuous mode.

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/aq_hw.h            |  2 ++
 drivers/net/ethernet/aquantia/atlantic/aq_nic.c           |  7 ++++---
 drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c |  7 +++++++
 drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c |  7 +++++++
 drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c  | 11 ++++++++++-
 5 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_hw.h b/drivers/net/ethernet/aquantia/atlantic/aq_hw.h
index 338c956ad1f7..3a4465c11768 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_hw.h
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_hw.h
@@ -374,6 +374,8 @@ struct aq_hw_ops {
 
 	int (*hw_read_module_eeprom)(struct aq_hw_s *self, u8 dev_addr,
 				     u8 reg_start_addr, int len, u8 *data);
+
+	int (*hw_get_num_multicast_addresses)(struct aq_hw_s *self);
 };
 
 struct aq_fw_ops {
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index 01792f7abf96..a05e12e8661b 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -973,6 +973,7 @@ int aq_nic_set_packet_filter(struct aq_nic_s *self, unsigned int flags)
 int aq_nic_set_multicast_list(struct aq_nic_s *self, struct net_device *ndev)
 {
 	const struct aq_hw_ops *hw_ops = self->aq_hw_ops;
+	int num_multicast_addresses = hw_ops->hw_get_num_multicast_addresses(self->aq_hw);
 	struct aq_nic_cfg_s *cfg = &self->aq_nic_cfg;
 	unsigned int packet_filter = ndev->flags;
 	struct netdev_hw_addr *ha = NULL;
@@ -980,7 +981,7 @@ int aq_nic_set_multicast_list(struct aq_nic_s *self, struct net_device *ndev)
 	int err = 0;
 
 	self->mc_list.count = 0;
-	if (netdev_uc_count(ndev) > AQ_HW_MULTICAST_ADDRESS_MAX) {
+	if (netdev_uc_count(ndev) > num_multicast_addresses) {
 		packet_filter |= IFF_PROMISC;
 	} else {
 		netdev_for_each_uc_addr(ha, ndev) {
@@ -990,7 +991,7 @@ int aq_nic_set_multicast_list(struct aq_nic_s *self, struct net_device *ndev)
 
 	cfg->is_mc_list_enabled = !!(packet_filter & IFF_MULTICAST);
 	if (cfg->is_mc_list_enabled) {
-		if (i + netdev_mc_count(ndev) > AQ_HW_MULTICAST_ADDRESS_MAX) {
+		if (i + netdev_mc_count(ndev) > num_multicast_addresses) {
 			packet_filter |= IFF_ALLMULTI;
 		} else {
 			netdev_for_each_mc_addr(ha, ndev) {
@@ -1000,7 +1001,7 @@ int aq_nic_set_multicast_list(struct aq_nic_s *self, struct net_device *ndev)
 		}
 	}
 
-	if (i > 0 && i <= AQ_HW_MULTICAST_ADDRESS_MAX) {
+	if (i > 0 && i <= num_multicast_addresses) {
 		self->mc_list.count = i;
 		err = hw_ops->hw_multicast_list_set(self->aq_hw,
 						    self->mc_list.ar,
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c
index 8de2cdd09213..1d69710c6be7 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c
@@ -944,6 +944,11 @@ static int hw_atl_a0_hw_fl3l4_set(struct aq_hw_s *self,
 	return aq_hw_err_from_flags(self);
 }
 
+static int hw_atl_a0_get_num_multicast_addresses(struct aq_hw_s *self)
+{
+	return AQ_HW_MULTICAST_ADDRESS_MAX;
+}
+
 const struct aq_hw_ops hw_atl_ops_a0 = {
 	.hw_soft_reset        = hw_atl_utils_soft_reset,
 	.hw_prepare           = hw_atl_utils_initfw,
@@ -978,4 +983,6 @@ const struct aq_hw_ops hw_atl_ops_a0 = {
 	.hw_get_regs                 = hw_atl_utils_hw_get_regs,
 	.hw_get_hw_stats             = hw_atl_utils_get_hw_stats,
 	.hw_get_fw_version           = hw_atl_utils_get_fw_version,
+
+	.hw_get_num_multicast_addresses = hw_atl_a0_get_num_multicast_addresses,
 };
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..252455cfd106 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
@@ -1777,6 +1777,11 @@ static int hw_atl_b0_read_module_eeprom(struct aq_hw_s *self, u8 dev_addr,
 	return err;
 }
 
+static int hw_atl_b0_get_num_multicast_addresses(struct aq_hw_s *self)
+{
+	return AQ_HW_MULTICAST_ADDRESS_MAX;
+}
+
 const struct aq_hw_ops hw_atl_ops_b0 = {
 	.hw_soft_reset        = hw_atl_utils_soft_reset,
 	.hw_prepare           = hw_atl_utils_initfw,
@@ -1836,4 +1841,6 @@ const struct aq_hw_ops hw_atl_ops_b0 = {
 
 	.hw_get_mac_temp         = hw_atl_b0_get_mac_temp,
 	.hw_read_module_eeprom   = hw_atl_b0_read_module_eeprom,
+
+	.hw_get_num_multicast_addresses = hw_atl_b0_get_num_multicast_addresses,
 };
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 8d7a489aa5de..d0dc72bd098f 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
@@ -810,7 +810,7 @@ static int hw_atl2_hw_multicast_list_set(struct aq_hw_s *self,
 	struct aq_nic_cfg_s *cfg = self->aq_nic_cfg;
 	int err = 0;
 
-	if (count > (HW_ATL2_MAC_MAX - HW_ATL2_MAC_MIN)) {
+	if (count > (priv->l2_filter_count - 1)) {
 		err = -EBADRQC;
 		goto err_exit;
 	}
@@ -1635,6 +1635,13 @@ static int hw_atl2_hw_vlan_ctrl(struct aq_hw_s *self, bool enable)
 	return aq_hw_err_from_flags(self);
 }
 
+static int hw_atl2_get_num_multicast_addresses(struct aq_hw_s *self)
+{
+	struct hw_atl2_priv *priv = self->priv;
+
+	return min_t(int, priv->l2_filter_count - 1, AQ_HW_MULTICAST_ADDRESS_MAX);
+}
+
 const struct aq_hw_ops hw_atl2_ops = {
 	.hw_soft_reset        = hw_atl2_utils_soft_reset,
 	.hw_prepare           = hw_atl2_utils_initfw,
@@ -1694,4 +1701,6 @@ const struct aq_hw_ops hw_atl2_ops = {
 	.hw_get_clk_sel          = hw_atl2_hw_get_clk_sel,
 	.extract_hwts            = NULL,
 	.hw_extts_gpio_enable    = NULL,
+
+	.hw_get_num_multicast_addresses = hw_atl2_get_num_multicast_addresses,
 };

-- 
2.56.0

Re: [PATCH v2 2/2] net: atlantic: AQC113: Limit multicast filters to available slots

[netdev-bot+sashiko]

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

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

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 patch swaps two constant bounds for values derived from the
  firmware-reported priv->l2_filter_count, and nothing validates that…
- [High] net: atlantic: off-by-one L2 filter index in
  hw_atl2_hw_packet_filter_set()
- [Low] The commit message leaves out the concrete impact and part of the
  code change: - It only says the firmware count (29) is "unfortunately…

--

net: atlantic: AQC113: Limit multicast filters to available slots

This commit adds a hw_get_num_multicast_addresses() callback to struct
aq_hw_ops. It lets AQC113 report fewer usable L2 filter slots than
AQ_HW_MULTICAST_ADDRESS_MAX, based on the l2_filter_count that the firmware
reports. aq_nic_set_multicast_list() uses this value to decide when to fall
back, and hw_atl2_hw_multicast_list_set() now bounds the count by the same
firmware value.

> On Apple silicon systems with AQC113 the firmware returns
[ ... ]
> unfortunately less than AQ_HW_MULTICAST_ADDRESS_MAX (32).
[ ... ]
> Use this potentially smaller
> number in aq_nic_set_multicast_list() to decide when to switch to
> promiscuous mode.

[Severity: Low]
This isn't a bug, but could the commit message describe the impact and the
full change more precisely?

Before this fix, with a base index of 9 and more than 28 addresses,
hw_atl2_hw_multicast_list_set() writes L2 unicast filter indices 38..41
(0x5240..0x525C). That is past the end of the filter table, and it overlaps
HW_ATL_RPFL2MC_ENF_ADR(0..3) at 0x5250..0x525C. The message does not say
this.

The message also does not mention that the guard in
hw_atl2_hw_multicast_list_set() changes from
(HW_ATL2_MAC_MAX - HW_ATL2_MAC_MIN) to (priv->l2_filter_count - 1).

Also, for multicast the fallback in aq_nic_set_multicast_list() is
IFF_ALLMULTI, not promiscuous mode.

[ ... ]

> 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 8d7a489aa5de4..d0dc72bd098f4 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
> @@ -810,7 +810,7 @@ static int hw_atl2_hw_multicast_list_set(struct aq_hw_s *self,
>  	struct aq_nic_cfg_s *cfg = self->aq_nic_cfg;
>  	int err = 0;
>  
> -	if (count > (HW_ATL2_MAC_MAX - HW_ATL2_MAC_MIN)) {
> +	if (count > (priv->l2_filter_count - 1)) {
>  		err = -EBADRQC;
>  		goto err_exit;
>  	}

[Severity: High]
This guard assumes hw_atl2_hw_packet_filter_set() uses the same slot
layout as this function. It doesn't look like it does.

hw_atl2_hw_multicast_list_set() sets base_location to
priv->l2_filters_base_index + 1. It writes ar_mac[0..count - 1] into
slots base_location + 0 through base_location + count - 1.

aq_nic_set_multicast_list() then calls aq_nic_set_packet_filter(),
which ends up in hw_atl2_hw_packet_filter_set(). That function computes
the same base_location, but its loop starts at i = HW_ATL2_MAC_MIN (1).
It enables base_location + i when i <= cfg->mc_list_count, so the +1
offset is applied a second time.

Take the values from the commit message (base index 9, count 29) and
mc_list_count == n:

  - slot 10 holds ar_mac[0], but hw_atl2_hw_packet_filter_set() never
    touches it. It stays enabled when the list shrinks to zero or
    IFF_MULTICAST is cleared.
  - slot 10 + n gets enabled even though no address was written to it
    on this pass, so frames for whatever stale address sits there are
    accepted.
  - the last iteration, i = 28, writes the enable bit of filter 38.
    That is HW_ATL2_MAC_MAX, past the end of the L2 unicast filter
    table.

The double offset happens on every AQC113, whatever the slot counts.
num_l2_filters is also only clamped to HW_ATL2_MAC_MAX, not to
HW_ATL2_MAC_MAX - base_location. So any base index that is large
enough relative to l2_filter_count walks off the end of the table.
This path also runs from aq_nic_start(), so every open hits it, not
only rx mode changes.

This patch is meant to keep the address list within the slots the
firmware reports. Should hw_atl2_hw_packet_filter_set() start at
i = 0, stop before l2_filter_count - 1 (clamped so base_location + i
stays below HW_ATL2_MAC_MAX), and test i < cfg->mc_list_count, to
match hw_atl2_hw_multicast_list_set()? The loop appears to come from
patch 1/2 of this series, so the fix probably belongs there.

[ ... ]

> @@ -1635,6 +1635,13 @@ static int hw_atl2_hw_vlan_ctrl(struct aq_hw_s *self, bool enable)
>  	return aq_hw_err_from_flags(self);
>  }
>  
> +static int hw_atl2_get_num_multicast_addresses(struct aq_hw_s *self)
> +{
> +	struct hw_atl2_priv *priv = self->priv;
> +
> +	return min_t(int, priv->l2_filter_count - 1, AQ_HW_MULTICAST_ADDRESS_MAX);
> +}

[Severity: High]
What happens here if the firmware reports l2_filter_count == 0?

priv->l2_filter_count is unsigned int, so priv->l2_filter_count - 1 wraps
to UINT_MAX, and min_t(int, ...) then returns -1.

hw_atl2_utils_get_filter_caps() stores the firmware value without any
check. The neighbouring art_count and l2_filters_base_index fields do get
sanitized:

drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_utils_fw.c:hw_atl2_utils_get_filter_caps() {
    ...
	priv->art_count = filter_caps.rslv_tbl_count * 8;
	if (priv->art_count == 0)
		priv->art_count = HW_ATL2_ART_TOTAL_ENTRIES;
	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;
    ...
}

In aq_nic_set_multicast_list(), the unicast check is signed, so it sets
IFF_PROMISC and leaves i at 0. But i is unsigned int, so in the multicast
check the -1 is converted to UINT_MAX:

	if (i + netdev_mc_count(ndev) > num_multicast_addresses) {
		packet_filter |= IFF_ALLMULTI;
	} else {
		netdev_for_each_mc_addr(ha, ndev) {
			ether_addr_copy(self->mc_list.ar[i++],
					ha->addr);

Doesn't this mean the IFF_ALLMULTI fallback is never taken? Once more than
32 multicast addresses are joined, would the loop then overflow
self->mc_list.ar[] into the following fields of struct aq_nic_s?

The later "i <= num_multicast_addresses" check is also unsigned, so
hw_atl2_hw_multicast_list_set() gets called with count > 32. Its new guard,
"count > (priv->l2_filter_count - 1)", also compares against UINT_MAX and
rejects nothing. It would then read ar_mac[] out of bounds and program
filters at base_location + i beyond HW_ATL2_MAC_MAX.

There is also a separate case: a large l2_filter_count with a nonzero base
index. The value is clamped only to AQ_HW_MULTICAST_ADDRESS_MAX, never to
HW_ATL2_MAC_MAX - (l2_filters_base_index + 1). The guard in
hw_atl2_hw_multicast_list_set() used the hardware constant before, and now
it trusts the firmware count.

With a base of 9 and a count of 34, wouldn't slots 10..41 get programmed?
Filters 38..41 seem to land at 0x5240..0x525C, which overlaps
HW_ATL_RPFL2MC_ENF_ADR(0..3).

Would it make sense to validate l2_filter_count in
hw_atl2_utils_get_filter_caps()? That would mean handling zero and clamping
base + count to HW_ATL2_MAC_MAX, like the other fields there.

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