[PATCH net 00/11] net: au1000: Bug fixes

[PATCH net 00/11] net: au1000: Bug fixes

[Florian Fainelli]

This patch series contains bug fixes that were found during code
inspection and also during fixing the open/close/open issue that was met
on the MTX-1 platform.

These patches were tested on a Meshcube MTX-1 board using targeted tests
to ensure the multicast and open/close/open bugs were fixed.

Florian Fainelli (11):
  net: au1000: recycle RX descriptors on allocation failure
  net: au1000: clear all-multicast mode when restoring hash filtering
  net: au1000: disconnect the PHY before freeing the MDIO bus
  net: au1000: report MDIO transaction timeouts
  net: au1000: serialize TX completion with packet submission
  net: au1000: assign a valid MAC address without platform data
  net: au1000: fix buffer descriptor free list insertion
  net: au1000: restore receive filtering after MAC initialization
  net: au1000: restore RX buffer addresses when restarting the MAC
  net: au1000: keep the MAC stopped until the PHY link is ready
  net: phy: smsc: wait for LAN83C183 power-down recovery

 drivers/net/ethernet/amd/au1000_eth.c | 74 ++++++++++++++++++++-------
 drivers/net/phy/Kconfig               |  2 +-
 drivers/net/phy/smsc.c                | 26 ++++++++++
 3 files changed, 83 insertions(+), 19 deletions(-)

-- 
2.53.0

[PATCH net 01/11] net: au1000: recycle RX descriptors on allocation failure

[Florian Fainelli]

An skb allocation failure retries the same completed descriptor forever
in hard IRQ context. Return the buffer to DMA and advance the ring even
when the packet must be dropped.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/amd/au1000_eth.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
index 8682323d693f..cd25248f4e3a 100644
--- a/drivers/net/ethernet/amd/au1000_eth.c
+++ b/drivers/net/ethernet/amd/au1000_eth.c
@@ -788,7 +788,7 @@ static int au1000_rx(struct net_device *dev)
 			skb = netdev_alloc_skb(dev, frmlen + 2);
 			if (!skb) {
 				dev->stats.rx_dropped++;
-				continue;
+				goto next;
 			}
 			skb_reserve(skb, 2);	/* 16 byte IP header align */
 			skb_copy_to_linear_data(skb,
@@ -820,6 +820,7 @@ static int au1000_rx(struct net_device *dev)
 				pr_cont("\n");
 			}
 		}
+next:
 		prxd->buff_stat = lower_32_bits(pDB->dma_addr) | RX_DMA_ENABLE;
 		aup->rx_head = (aup->rx_head + 1) & (NUM_RX_DMA - 1);
 		wmb(); /* drain writebuffer */
-- 
2.53.0

[PATCH net 02/11] net: au1000: clear all-multicast mode when restoring hash filtering

[Florian Fainelli]

MAC_PASS_ALL_MULTI remains set after IFF_ALLMULTI is cleared or the
multicast list shrinks below the filter limit. Clear it when programming
the selective multicast hash so unwanted multicast traffic is filtered
again.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/amd/au1000_eth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
index cd25248f4e3a..58bc8e1b539d 100644
--- a/drivers/net/ethernet/amd/au1000_eth.c
+++ b/drivers/net/ethernet/amd/au1000_eth.c
@@ -1042,7 +1042,7 @@ static void au1000_multicast_list(struct net_device *dev)
 					(long *)mc_filter);
 		writel(mc_filter[1], &aup->mac->multi_hash_high);
 		writel(mc_filter[0], &aup->mac->multi_hash_low);
-		reg &= ~MAC_PROMISCUOUS;
+		reg &= ~(MAC_PROMISCUOUS | MAC_PASS_ALL_MULTI);
 		reg |= MAC_HASH_MODE;
 	}
 	writel(reg, &aup->mac->control);
-- 
2.53.0

[PATCH net 03/11] net: au1000: disconnect the PHY before freeing the MDIO bus

[Florian Fainelli]

Balance phy_connect() on probe failure and removal. Otherwise PHY
interrupts, device references and the attachment to the freed netdev
survive teardown. Disconnect before unregistering the bus while its MDIO
callbacks and MAC registers remain usable.

Fixes: 0638dec01e89 ("[PATCH] net: au1000_eth: PHY framework conversion")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/amd/au1000_eth.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
index 58bc8e1b539d..70319b432182 100644
--- a/drivers/net/ethernet/amd/au1000_eth.c
+++ b/drivers/net/ethernet/amd/au1000_eth.c
@@ -1287,6 +1287,8 @@ static int au1000_probe(struct platform_device *pdev)
 	return 0;
 
 err_out:
+	if (dev->phydev)
+		phy_disconnect(dev->phydev);
 	if (aup->mii_bus)
 		mdiobus_unregister(aup->mii_bus);
 
@@ -1334,6 +1336,8 @@ static void au1000_remove(struct platform_device *pdev)
 	struct resource *base, *macen;
 
 	unregister_netdev(dev);
+	if (dev->phydev)
+		phy_disconnect(dev->phydev);
 	mdiobus_unregister(aup->mii_bus);
 	mdiobus_free(aup->mii_bus);
 
-- 
2.53.0

[PATCH net 04/11] net: au1000: report MDIO transaction timeouts

[Florian Fainelli]

The MDIO write callback reports success even if the controller stays
busy and no write is issued. Propagate timeout errors and wait for write
completion so failure of the final transaction is reported to phylib as
well. Return ETIMEDOUT for read timeouts too.

Fixes: 0638dec01e89 ("[PATCH] net: au1000_eth: PHY framework conversion")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/amd/au1000_eth.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
index 70319b432182..a507a524370f 100644
--- a/drivers/net/ethernet/amd/au1000_eth.c
+++ b/drivers/net/ethernet/amd/au1000_eth.c
@@ -278,7 +278,7 @@ static int au1000_mdio_read(struct net_device *dev, int phy_addr, int reg)
 		mdelay(1);
 		if (--timedout == 0) {
 			netdev_err(dev, "read_MII busy timeout!!\n");
-			return -1;
+			return -ETIMEDOUT;
 		}
 	}
 
@@ -292,14 +292,14 @@ static int au1000_mdio_read(struct net_device *dev, int phy_addr, int reg)
 		mdelay(1);
 		if (--timedout == 0) {
 			netdev_err(dev, "mdio_read busy timeout!!\n");
-			return -1;
+			return -ETIMEDOUT;
 		}
 	}
 	return readl(mii_data_reg);
 }
 
-static void au1000_mdio_write(struct net_device *dev, int phy_addr,
-			      int reg, u16 value)
+static int au1000_mdio_write(struct net_device *dev, int phy_addr,
+			     int reg, u16 value)
 {
 	struct au1000_private *aup = netdev_priv(dev);
 	u32 *const mii_control_reg = &aup->mac->mii_control;
@@ -311,7 +311,7 @@ static void au1000_mdio_write(struct net_device *dev, int phy_addr,
 		mdelay(1);
 		if (--timedout == 0) {
 			netdev_err(dev, "mdio_write busy timeout!!\n");
-			return;
+			return -ETIMEDOUT;
 		}
 	}
 
@@ -320,6 +320,17 @@ static void au1000_mdio_write(struct net_device *dev, int phy_addr,
 
 	writel(value, mii_data_reg);
 	writel(mii_control, mii_control_reg);
+
+	timedout = 20;
+	while (readl(mii_control_reg) & MAC_MII_BUSY) {
+		mdelay(1);
+		if (--timedout == 0) {
+			netdev_err(dev, "mdio_write busy timeout!!\n");
+			return -ETIMEDOUT;
+		}
+	}
+
+	return 0;
 }
 
 static int au1000_mdiobus_read(struct mii_bus *bus, int phy_addr, int regnum)
@@ -344,8 +355,7 @@ static int au1000_mdiobus_write(struct mii_bus *bus, int phy_addr, int regnum,
 	 */
 	au1000_enable_mac(dev, 0);
 
-	au1000_mdio_write(dev, phy_addr, regnum, value);
-	return 0;
+	return au1000_mdio_write(dev, phy_addr, regnum, value);
 }
 
 static int au1000_mdiobus_reset(struct mii_bus *bus)
-- 
2.53.0

[PATCH net 05/11] net: au1000: serialize TX completion with packet submission

[Florian Fainelli]

TX completion runs in hard IRQ context while ndo_start_xmit updates the
same descriptors and ring state without locking. In particular, an
interrupt between netif_stop_queue() and setting tx_full can miss the
wakeup and leave the queue stopped until the watchdog runs. Use the
existing IRQ-safe device lock in both paths to serialize descriptor
reclamation and queue state changes.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/amd/au1000_eth.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
index a507a524370f..90887dac26bc 100644
--- a/drivers/net/ethernet/amd/au1000_eth.c
+++ b/drivers/net/ethernet/amd/au1000_eth.c
@@ -873,7 +873,9 @@ static void au1000_tx_ack(struct net_device *dev)
 {
 	struct au1000_private *aup = netdev_priv(dev);
 	struct tx_dma *ptxd;
+	unsigned long flags;
 
+	spin_lock_irqsave(&aup->lock, flags);
 	ptxd = aup->tx_dma_ring[aup->tx_tail];
 
 	while (ptxd->buff_stat & TX_T_DONE) {
@@ -890,6 +892,8 @@ static void au1000_tx_ack(struct net_device *dev)
 			netif_wake_queue(dev);
 		}
 	}
+
+	spin_unlock_irqrestore(&aup->lock, flags);
 }
 
 /*
@@ -973,17 +977,20 @@ static netdev_tx_t au1000_tx(struct sk_buff *skb, struct net_device *dev)
 	u32 buff_stat;
 	struct db_dest *pDB;
 	int i;
+	unsigned long flags;
 
 	netif_dbg(aup, tx_queued, dev, "tx: aup %x len=%d, data=%p, head %d\n",
 				(unsigned)aup, skb->len,
 				skb->data, aup->tx_head);
 
+	spin_lock_irqsave(&aup->lock, flags);
 	ptxd = aup->tx_dma_ring[aup->tx_head];
 	buff_stat = ptxd->buff_stat;
 	if (buff_stat & TX_DMA_ENABLE) {
 		/* We've wrapped around and the transmitter is still busy */
 		netif_stop_queue(dev);
 		aup->tx_full = 1;
+		spin_unlock_irqrestore(&aup->lock, flags);
 		return NETDEV_TX_BUSY;
 	} else if (buff_stat & TX_T_DONE) {
 		au1000_update_tx_stats(dev, ptxd->status);
@@ -1012,6 +1019,7 @@ static netdev_tx_t au1000_tx(struct sk_buff *skb, struct net_device *dev)
 	wmb(); /* drain writebuffer */
 	dev_kfree_skb(skb);
 	aup->tx_head = (aup->tx_head + 1) & (NUM_TX_DMA - 1);
+	spin_unlock_irqrestore(&aup->lock, flags);
 	return NETDEV_TX_OK;
 }
 
-- 
2.53.0

[PATCH net 06/11] net: au1000: assign a valid MAC address without platform data

[Florian Fainelli]

The supported no-platform-data path leaves dev_addr as all zeroes. The
device can register but eth_validate_addr() prevents it from opening.
Generate a random address just as for platform data containing an
invalid address.

Fixes: 7718f2c27dbd ("au1000_eth: use eth_hw_addr_random() instead of random_ether_addr()")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/amd/au1000_eth.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
index 90887dac26bc..c0e38a507f89 100644
--- a/drivers/net/ethernet/amd/au1000_eth.c
+++ b/drivers/net/ethernet/amd/au1000_eth.c
@@ -1196,6 +1196,7 @@ static int au1000_probe(struct platform_device *pdev)
 		dev_info(&pdev->dev, "no platform_data passed,"
 					" PHY search on MAC0\n");
 		aup->phy1_search_mac0 = 1;
+		eth_hw_addr_random(dev);
 	} else {
 		if (is_valid_ether_addr(pd->mac)) {
 			eth_hw_addr_set(dev, pd->mac);
-- 
2.53.0

[PATCH net 07/11] net: au1000: fix buffer descriptor free list insertion

[Florian Fainelli]

ReleaseDB links the old head to the returned descriptor and makes the
returned descriptor the new head without updating its stale next
pointer. This loses free descriptors and can create cycles. Link the
returned descriptor to the old head instead, matching GetFreeDB and the
list initialization.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/amd/au1000_eth.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
index c0e38a507f89..f8976e324f0e 100644
--- a/drivers/net/ethernet/amd/au1000_eth.c
+++ b/drivers/net/ethernet/amd/au1000_eth.c
@@ -583,9 +583,7 @@ static struct db_dest *au1000_GetFreeDB(struct au1000_private *aup)
 
 static void au1000_ReleaseDB(struct au1000_private *aup, struct db_dest *pDB)
 {
-	struct db_dest *pDBfree = aup->pDBfree;
-	if (pDBfree)
-		pDBfree->pnext = pDB;
+	pDB->pnext = aup->pDBfree;
 	aup->pDBfree = pDB;
 }
 
-- 
2.53.0

[PATCH net 08/11] net: au1000: restore receive filtering after MAC initialization

[Florian Fainelli]

MAC initialization overwrites the receive mode, dropping promiscuous and
multicast settings after a watchdog reset without notifying the network
stack. Reapply the current netdev receive mode after initialization.
Protect the control register read-modify-write with the device lock so
it cannot overwrite a concurrent link adjustment or MAC stop.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/amd/au1000_eth.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
index f8976e324f0e..6b817d18329b 100644
--- a/drivers/net/ethernet/amd/au1000_eth.c
+++ b/drivers/net/ethernet/amd/au1000_eth.c
@@ -684,6 +684,8 @@ static const struct ethtool_ops au1000_ethtool_ops = {
 	.set_link_ksettings = phy_ethtool_set_link_ksettings,
 };
 
+static void au1000_multicast_list(struct net_device *dev);
+
 /*
  * Initialize the interface.
  *
@@ -742,6 +744,9 @@ static int au1000_init(struct net_device *dev)
 	wmb(); /* drain writebuffer */
 
 	spin_unlock_irqrestore(&aup->lock, flags);
+	netif_addr_lock_bh(dev);
+	au1000_multicast_list(dev);
+	netif_addr_unlock_bh(dev);
 	return 0;
 }
 
@@ -1038,8 +1043,10 @@ static void au1000_multicast_list(struct net_device *dev)
 {
 	struct au1000_private *aup = netdev_priv(dev);
 	u32 reg;
+	unsigned long flags;
 
 	netif_dbg(aup, drv, dev, "%s: flags=%x\n", __func__, dev->flags);
+	spin_lock_irqsave(&aup->lock, flags);
 	reg = readl(&aup->mac->control);
 	if (dev->flags & IFF_PROMISC) {			/* Set promiscuous. */
 		reg |= MAC_PROMISCUOUS;
@@ -1062,6 +1069,7 @@ static void au1000_multicast_list(struct net_device *dev)
 		reg |= MAC_HASH_MODE;
 	}
 	writel(reg, &aup->mac->control);
+	spin_unlock_irqrestore(&aup->lock, flags);
 }
 
 static const struct net_device_ops au1000_netdev_ops = {
-- 
2.53.0

[PATCH net 09/11] net: au1000: restore RX buffer addresses when restarting the MAC

[Florian Fainelli]

The MAC DMA engine updates each receive descriptor's address as it
transfers data. Clearing the descriptor control bits during reset does
not restore the buffer's starting address.

au1000_init() only set RX_DMA_ENABLE on the address left in the
descriptor. After closing and reopening the interface, a receive could
therefore start partway through its buffer and overwrite memory beyond
the allocation.

Reload each RX descriptor from rx_db_inuse before enabling it, as the
receive completion path already does.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/amd/au1000_eth.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
index 6b817d18329b..8c2dbf72652e 100644
--- a/drivers/net/ethernet/amd/au1000_eth.c
+++ b/drivers/net/ethernet/amd/au1000_eth.c
@@ -722,7 +722,8 @@ static int au1000_init(struct net_device *dev)
 
 
 	for (i = 0; i < NUM_RX_DMA; i++)
-		aup->rx_dma_ring[i]->buff_stat |= RX_DMA_ENABLE;
+		writel(lower_32_bits(aup->rx_db_inuse[i]->dma_addr) |
+		       RX_DMA_ENABLE, &aup->rx_dma_ring[i]->buff_stat);
 
 	wmb(); /* drain writebuffer */
 
-- 
2.53.0

[PATCH net 10/11] net: au1000: keep the MAC stopped until the PHY link is ready

[Florian Fainelli]

phy_stop() powers down the PHY when the interface is closed. The driver
leaves MAC receive/transmit enabled on link-down and enables them again
during initialization before resuming the PHY. The MTX-1 can hang when
the MAC runs while its MII clocks are absent or still recovering.

Start the PHY before initializing the MAC, but leave receive/transmit
stopped while the PHY reports no link. The duplex update enables them
when the link comes up. Stop the MAC in the link-down callback before
phylib suspends the PHY, and stop the PHY if MAC initialization fails.

Fixes: be9dad1f9f26 ("net: phy: suspend phydev when going to HALTED")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/amd/au1000_eth.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
index 8c2dbf72652e..c616ce616541 100644
--- a/drivers/net/ethernet/amd/au1000_eth.c
+++ b/drivers/net/ethernet/amd/au1000_eth.c
@@ -458,7 +458,8 @@ au1000_adjust_link(struct net_device *dev)
 		/* link state changed */
 
 		if (!phydev->link) {
-			/* link went down */
+			/* Stop the MAC before the PHY can remove its clocks. */
+			au1000_hard_stop(dev);
 			aup->old_speed = 0;
 			aup->old_duplex = -1;
 		}
@@ -727,7 +728,7 @@ static int au1000_init(struct net_device *dev)
 
 	wmb(); /* drain writebuffer */
 
-	control = MAC_RX_ENABLE | MAC_TX_ENABLE;
+	control = 0;
 #ifndef CONFIG_CPU_LITTLE_ENDIAN
 	control |= MAC_BIG_ENDIAN;
 #endif
@@ -740,6 +741,10 @@ static int au1000_init(struct net_device *dev)
 		control |= MAC_FULL_DUPLEX;
 	}
 
+	/* The PHY may still be restoring its MII clocks after resume. */
+	if (!dev->phydev || dev->phydev->link)
+		control |= MAC_RX_ENABLE | MAC_TX_ENABLE;
+
 	writel(control, &aup->mac->control);
 	writel(0x8100, &aup->mac->vlan1_tag); /* activate vlan support */
 	wmb(); /* drain writebuffer */
@@ -928,16 +933,18 @@ static int au1000_open(struct net_device *dev)
 		return retval;
 	}
 
+	if (dev->phydev)
+		phy_start(dev->phydev);
+
 	retval = au1000_init(dev);
 	if (retval) {
 		netdev_err(dev, "error in au1000_init\n");
+		if (dev->phydev)
+			phy_stop(dev->phydev);
 		free_irq(dev->irq, dev);
 		return retval;
 	}
 
-	if (dev->phydev)
-		phy_start(dev->phydev);
-
 	netif_start_queue(dev);
 
 	netif_dbg(aup, drv, dev, "open: Initialization done.\n");
-- 
2.53.0

[PATCH net 11/11] net: phy: smsc: wait for LAN83C183 power-down recovery

[Florian Fainelli]

The LAN83C183 uses the generic PHY driver, which returns immediately
after clearing BMCR_PDOWN. Section 1.11 of the datasheet allows 500 ms
before normal operation is restored. Starting the Au1000 MAC before the
MII clocks recover can hang the MTX-1 when reopening its interface.

Add a LAN83C183 driver entry and resume callback. Wait for recovery only
when BMCR_PDOWN was actually cleared, and propagate MDIO errors without
waiting. Resuming an already awake PHY does not incur the delay.

Fixes: be9dad1f9f26 ("net: phy: suspend phydev when going to HALTED")
Link: https://pdf.dzsc.com/LAN/LAN83C183.pdf#page=44
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/Kconfig |  2 +-
 drivers/net/phy/smsc.c  | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 30e9d22c8d1a..10466b2574eb 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -421,7 +421,7 @@ config SMSC_PHY
 	tristate "SMSC PHYs"
 	select CRC16
 	help
-	  Currently supports the LAN83C185, LAN8187 and LAN8700 PHYs
+	  Currently supports the LAN83C183, LAN83C185, LAN8187 and LAN8700 PHYs
 
 config STE10XP
 	tristate "STMicroelectronics STe10Xp PHYs"
diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index 48487149c225..c3b51afd4b1e 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/mii.h>
@@ -684,8 +685,32 @@ int smsc_phy_probe(struct phy_device *phydev)
 }
 EXPORT_SYMBOL_GPL(smsc_phy_probe);
 
+static int lan83c183_resume(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_modify_changed(phydev, MII_BMCR, BMCR_PDOWN, 0);
+	if (ret < 0)
+		return ret;
+
+	/* The PHY needs up to 500 ms to restore operation after power-down. */
+	if (ret)
+		msleep(500);
+
+	return 0;
+}
+
 static struct phy_driver smsc_phy_driver[] = {
 {
+	.phy_id		= 0x0016f840,
+	.phy_id_mask	= 0xfffffff0,
+	.name		= "SMSC LAN83C183",
+
+	.get_features	= genphy_read_abilities,
+	.suspend	= genphy_suspend,
+	.resume		= lan83c183_resume,
+	.set_loopback	= genphy_loopback,
+}, {
 	.phy_id		= 0x0007c0a0, /* OUI=0x00800f, Model#=0x0a */
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "SMSC LAN83C185",
@@ -887,6 +912,7 @@ MODULE_AUTHOR("Herbert Valerio Riedel");
 MODULE_LICENSE("GPL");
 
 static const struct mdio_device_id __maybe_unused smsc_tbl[] = {
+	{ 0x0016f840, 0xfffffff0 },
 	{ 0x0007c0a0, 0xfffffff0 },
 	{ 0x0007c0b0, 0xfffffff0 },
 	{ 0x0007c0c0, 0xfffffff0 },
-- 
2.53.0

Re: [PATCH net 00/11] net: au1000: Bug fixes

[Jakub Kicinski]

On Tue, 27 Oct 2026 15:31:29 -0700 Florian Fainelli wrote:
> This patch series contains bug fixes that were found during code
> inspection and also during fixing the open/close/open issue that was met
> on the MTX-1 platform.

Are you trying to bypass the 15 patch limit by using 2 email addrs? 🤔️

Re: [PATCH net 00/11] net: au1000: Bug fixes

[Florian Fainelli]

On 10/27/2026 4:21 PM, Jakub Kicinski wrote:
> On Tue, 27 Oct 2026 15:31:29 -0700 Florian Fainelli wrote:
>> This patch series contains bug fixes that were found during code
>> inspection and also during fixing the open/close/open issue that was met
>> on the MTX-1 platform.
> 
> Are you trying to bypass the 15 patch limit by using 2 email addrs? 🤔️

That would be clever, would not it, not at all. There's what happens at 
work and then in my "spare" time. Things should slow down once patches 
get accepted.
-- 
Florian

Re: [PATCH net 03/11] net: au1000: disconnect the PHY before freeing the MDIO bus

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0 (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).

- [Medium] In drivers/net/ethernet/amd/au1000_eth.c, au1000_remove() now
  calls phy_disconnect() after unregister_netdev().

Pre-existing issues:
- [Low] The two new phy_disconnect() calls (au1000_remove() after
  unregister_netdev(), and the au1000_probe() err_out path) run without…

--

net: au1000: disconnect the PHY before freeing the MDIO bus

This patch adds phy_disconnect() calls to the au1000_probe() error path
and to au1000_remove(). This balances the phy_connect() done in
au1000_mii_probe(). In both paths the disconnect runs before the MDIO bus
is unregistered.

> diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
> index 58bc8e1b539d5..70319b432182f 100644
> --- a/drivers/net/ethernet/amd/au1000_eth.c
> +++ b/drivers/net/ethernet/amd/au1000_eth.c
> @@ -1287,6 +1287,8 @@ static int au1000_probe(struct platform_device *pdev)
>  	return 0;
>  
>  err_out:
> +	if (dev->phydev)
> +		phy_disconnect(dev->phydev);
>  	if (aup->mii_bus)
>  		mdiobus_unregister(aup->mii_bus);
>  

[ ... ]

> @@ -1334,6 +1336,8 @@ static void au1000_remove(struct platform_device *pdev)
>  	struct resource *base, *macen;
>  
>  	unregister_netdev(dev);
> +	if (dev->phydev)
> +		phy_disconnect(dev->phydev);

[Severity: Medium]
Can this leave the MAC clocked and out of reset after the driver unbinds?

By the time au1000_remove() reaches this point, the MAC is already in
reset with its clock gated, and aup->mac_enabled is 0. If the interface
was never opened, au1000_probe() did this by calling au1000_reset_mac()
just before register_netdev(). If the interface was opened,
au1000_close() did it.

If the interface was never opened, phydev->suspended is still false,
because phy_attach_direct() resumed the PHY. In that case phy_disconnect()
does MDIO accesses:

phy_disconnect()
  phy_detach()
    phy_detach_internal()
      phy_suspend()
        genphy_suspend()
          phy_set_bits(phydev, MII_BMCR, BMCR_PDOWN)

Each of these accesses goes through au1000_mdiobus_read() or
au1000_mdiobus_write(). Both call au1000_enable_mac(dev, 0) first:

	if (force_reset || (!aup->mac_enabled)) {
		writel(MAC_EN_CLOCK_ENABLE, aup->enable);
		...
		writel((MAC_EN_RESET0 | MAC_EN_RESET1 | MAC_EN_RESET2
				| MAC_EN_CLOCK_ENABLE), aup->enable);
		...
		aup->mac_enabled = 1;
	}

Nothing later in au1000_remove() calls au1000_reset_mac() again before
the registers are unmapped and the netdev is freed. Before this patch,
remove left the MAC in reset with its clock gated.

The err_out path in au1000_probe() does call au1000_reset_mac(dev) after
phy_disconnect() and mdiobus_unregister(). Should au1000_remove() also
call au1000_reset_mac(dev) after phy_disconnect(), to match it?

[Severity: Low]
This is a pre-existing issue, not one introduced by this patch, but both
new phy_disconnect() calls run without RTNL held. In au1000_remove(),
unregister_netdev() has already dropped RTNL. au1000_probe() never takes
it.

phy_disconnect()->phy_detach()->phy_detach_internal() does:

		hwprov = rtnl_dereference(dev->hwprov);

With CONFIG_PROVE_RCU enabled, could this emit a one-time "suspicious
rcu_dereference_protected() usage" warning? The read itself looks
harmless, since au1000 never installs a hwtstamp provider.

Other drivers already call phy_disconnect() after unregister_netdev()
without RTNL, for example r6040_remove_one(), et131x_pci_remove(),
bgmac_enet_remove(), mlxbf_gige_remove(), hisi_femac_drv_remove() and
ixp4xx_eth_remove(). So this looks like a problem with the phylib
annotation, not with this driver.

>  	mdiobus_unregister(aup->mii_bus);
>  	mdiobus_free(aup->mii_bus);
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027223140.3774570-1-f.fainelli%40gmail.com

Re: [PATCH net 04/11] net: au1000: report MDIO transaction timeouts

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new completion poll in au1000_mdio_write() reads MAC_MII_BUSY
  right after `writel(mii_control, mii_control_reg)`.

Pre-existing issues:
- [Low] On success, au1000_mdio_read() returns the raw 32-bit
  MAC_MII_DATA value (`return readl(mii_data_reg);`, au1000_eth.c:298)…

--

net: au1000: report MDIO transaction timeouts

This commit changes au1000_mdio_write() to return an int. Timeouts from
the busy wait before the transaction, and from a new completion poll after
the write, now go back to phylib through au1000_mdiobus_write(). The read
timeout paths in au1000_mdio_read() now return -ETIMEDOUT instead of -1.

> diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
> index 70319b432182f..a507a524370fd 100644
> --- a/drivers/net/ethernet/amd/au1000_eth.c
> +++ b/drivers/net/ethernet/amd/au1000_eth.c

[ ... ]

> @@ -292,14 +292,14 @@ static int au1000_mdio_read(struct net_device *dev, int phy_addr, int reg)
>  		mdelay(1);
>  		if (--timedout == 0) {
>  			netdev_err(dev, "mdio_read busy timeout!!\n");
> -			return -1;
> +			return -ETIMEDOUT;
>  		}
>  	}
>  	return readl(mii_data_reg);

[Severity: Low]
This isn't a bug introduced by this patch, but should au1000_mdio_read()
mask the data register value to 16 bits before returning it?

On success the raw 32-bit MAC_MII_DATA value goes back through
au1000_mdiobus_read() to phylib. __mdiobus_read() expects either a
register value or a negative errno, and it passes the value through
without masking:

drivers/net/phy/mdio_bus.c:__mdiobus_read() {
    ...
	if (bus->read)
		retval = bus->read(bus, addr, regnum);
    ...
}

If bits 31:16 of mii_data ever read back non-zero, phylib would get a
value wider than 16 bits. If bit 31 were set, it would read as an error.

Nothing in the tree shows whether those upper bits are ever non-zero on
this hardware. Would something like readl(mii_data_reg) & 0xffff be
safer, now that negative values on this path are proper errnos?

>  }

[ ... ]

> @@ -320,6 +320,17 @@ static void au1000_mdio_write(struct net_device *dev, int phy_addr,
>  
>  	writel(value, mii_data_reg);
>  	writel(mii_control, mii_control_reg);
> +
> +	timedout = 20;
> +	while (readl(mii_control_reg) & MAC_MII_BUSY) {
> +		mdelay(1);

[Severity: Low]
With this new completion poll, will every MDIO write now busy-wait for at
least 1 ms?

A clause 22 management frame is about 64 MDC cycles, which is roughly
26 us at 2.5 MHz. So the first readl() right after
writel(mii_control, mii_control_reg) will almost always see MAC_MII_BUSY
set, and mdelay(1) will run at least once.

The mii_bus->write callback runs in process context under
bus->mdio_lock, which is a mutex. Could this use readl_poll_timeout()
with a sleep of a few tens of microseconds and the same ~20 ms timeout?

To be fair, before this patch the access that followed a write already
paid this mdelay(1) in its own BUSY wait before starting. So for
back-to-back sequences the cost mostly moves into the write. The new cost
falls on the last write of a burst and on isolated writes. The existing
completion poll in au1000_mdio_read() has the same 1 ms granularity.

> +		if (--timedout == 0) {
> +			netdev_err(dev, "mdio_write busy timeout!!\n");
> +			return -ETIMEDOUT;
> +		}
> +	}
> +
> +	return 0;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027223140.3774570-1-f.fainelli%40gmail.com

Re: [PATCH net 05/11] net: au1000: serialize TX completion with packet submission

[netdev-bot+sashiko]

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

Critical: 0 (1) · High: 0 · Medium: 2 · 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).

- [Medium] au1000_tx() in drivers/net/ethernet/amd/au1000_eth.c now frees
  the skb with hardware IRQs disabled.
- [Medium] After this patch, au1000_tx()'s inline reclaim path leaves
  aup->tx_tail permanently out of step with the hardware.

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but
  au1000_tx() copies skb->len bytes into a fixed MAX_BUF_SIZE…

--

net: au1000: serialize TX completion with packet submission

This commit takes the existing IRQ-safe aup->lock in both au1000_tx_ack()
and au1000_tx(). The goal is to serialize TX descriptor reclamation in hard
IRQ context with packet submission in ndo_start_xmit. It also closes a
missed-wakeup window between netif_stop_queue() and setting tx_full.

> diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
> index a507a524370fd..90887dac26bc9 100644
> --- a/drivers/net/ethernet/amd/au1000_eth.c
> +++ b/drivers/net/ethernet/amd/au1000_eth.c

[ ... ]

> @@ -973,17 +977,20 @@ static netdev_tx_t au1000_tx(struct sk_buff *skb, struct net_device *dev)
>  	u32 buff_stat;
>  	struct db_dest *pDB;
>  	int i;
> +	unsigned long flags;
>  
>  	netif_dbg(aup, tx_queued, dev, "tx: aup %x len=%d, data=%p, head %d\n",
>  				(unsigned)aup, skb->len,
>  				skb->data, aup->tx_head);
>  
> +	spin_lock_irqsave(&aup->lock, flags);
>  	ptxd = aup->tx_dma_ring[aup->tx_head];
>  	buff_stat = ptxd->buff_stat;
>  	if (buff_stat & TX_DMA_ENABLE) {
>  		/* We've wrapped around and the transmitter is still busy */
>  		netif_stop_queue(dev);
>  		aup->tx_full = 1;
> +		spin_unlock_irqrestore(&aup->lock, flags);
>  		return NETDEV_TX_BUSY;
>  	} else if (buff_stat & TX_T_DONE) {
>  		au1000_update_tx_stats(dev, ptxd->status);

[Severity: Medium]

Now that the lock is held here, can this inline reclaim leave aup->tx_tail
permanently out of step with the hardware?

If desc[tx_head] has TX_T_DONE set at this point, the ring is full
(tx_head == tx_tail) and the oldest buffer has just completed. This branch
updates stats and zeroes len. Later in au1000_tx() the code re-arms the
descriptor, which clears the done bit, and advances tx_head:

	ptxd->buff_stat = lower_32_bits(pDB->dma_addr) | TX_DMA_ENABLE;
	...
	aup->tx_head = (aup->tx_head + 1) & (NUM_TX_DMA - 1);

tx_tail is never advanced on this path.

Before this patch, on the uniprocessor Au1x00, the pending MAC DMA
interrupt could run au1000_tx_ack() before the re-arm and move tx_tail
forward. Now the ISR is held off until spin_unlock_irqrestore(). At that
point au1000_tx_ack() finds desc[tx_tail] re-armed with no done bit:

	while (ptxd->buff_stat & TX_T_DONE) {

So it returns without moving tx_tail, which now points at the newest
submitted descriptor.

au1000_init() sets tx_head from the hardware's own buffer index, so the
hardware completes descriptors in index order and that newest descriptor
completes last. Wouldn't that leave the TX_T_DONE bits of older completed
descriptors unacknowledged until then?

It would also delay netif_wake_queue() in au1000_tx_ack() until the whole
4-entry ring drains, rather than after one completion. The offset would
then persist until au1000_init() resets head and tail.

The same thing can happen when au1000_tx() is called with IRQs already
disabled, for example from netpoll.

The MAC DMA interrupt is IRQ_TYPE_LEVEL_HIGH, and au1000_interrupt() always
returns IRQ_RETVAL(1). If unacknowledged done bits keep the line asserted,
could this become an interrupt storm? That part depends on hardware
behaviour that has not been confirmed.

[ ... ]

[Severity: Critical]

This is a pre-existing issue, not one introduced by this patch.
au1000_tx() copies skb->len bytes into a fixed MAX_BUF_SIZE (2048 byte)
bounce buffer without checking the length:

	pDB = aup->tx_db_inuse[aup->tx_head];
	skb_copy_from_linear_data(skb, (void *)pDB->vaddr, skb->len);

Can skb->len be larger than 2048 here? When po->vnet_hdr_sz is nonzero,
tpacket_snd() in net/packet/af_packet.c skips both MTU checks:

	if ((size_max > dev->mtu + reserve + VLAN_HLEN) && !vnet_hdr_sz)
		size_max = dev->mtu + reserve + VLAN_HLEN;
	...
		if (likely(tp_len >= 0) &&
		    tp_len > dev->mtu + reserve &&
		    !vnet_hdr_sz &&
		    !packet_extra_vlan_len_allowed(dev, skb))

An all-zero virtio_net_hdr (GSO_NONE, no NEEDS_CSUM) is accepted without
making the skb GSO. validate_xmit_skb() then only linearizes it, because
au1000 does not advertise NETIF_F_SG, and applies no MTU check.

With CAP_NET_RAW, this path could deliver a multi-kilobyte skb to
au1000_tx():

socket(AF_PACKET, SOCK_RAW)
  setsockopt(PACKET_VNET_HDR)
  setsockopt(PACKET_TX_RING) with tp_frame_size > 2048
  tpacket_snd()
    packet_xmit()
      validate_xmit_skb()
        au1000_tx()
          skb_copy_from_linear_data(..., skb->len)

au1000_probe() allocates a single 16 KiB region with dma_alloc_coherent().
The TX buffers are at offsets 0-8K and the RX buffers at 8-16K. Would an
oversized frame overwrite the neighbouring TX/RX DMA buffers? Beyond about
10 KiB, would it write past the end of the coherent allocation?

Should au1000_tx() drop frames larger than MAX_BUF_SIZE?

> @@ -1012,6 +1019,7 @@ static netdev_tx_t au1000_tx(struct sk_buff *skb, struct net_device *dev)
>  	wmb(); /* drain writebuffer */
>  	dev_kfree_skb(skb);
>  	aup->tx_head = (aup->tx_head + 1) & (NUM_TX_DMA - 1);
> +	spin_unlock_irqrestore(&aup->lock, flags);
>  	return NETDEV_TX_OK;
>  }

[Severity: Medium]

With the new spin_lock_irqsave() in au1000_tx(), dev_kfree_skb(skb) now
runs with hard IRQs disabled. dev_kfree_skb() is just consume_skb():

include/linux/skbuff.h:
#define dev_kfree_skb(a)	consume_skb(a)

include/linux/netdevice.h says:

 * It is not allowed to call kfree_skb() or consume_skb() from hardware
 * interrupt context or with hardware interrupts being disabled.
 * (in_hardirq() || irqs_disabled())

Does this mean every transmitted packet now runs its skb destructors with
IRQs off? That includes sock_wfree()/tcp_wfree(), zerocopy ubuf callbacks,
and dst and conntrack puts.

If any of those reach local_bh_enable(), lockdep_assert_irqs_enabled()
would fire. With CONFIG_TRACE_IRQFLAGS, __local_bh_enable_ip() could also
turn IRQs back on inside the aup->lock section, which would reopen the
race this patch is closing.

The data has already been copied into pDB->vaddr. Could the free move
after spin_unlock_irqrestore(), or use dev_consume_skb_any() instead?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027223140.3774570-1-f.fainelli%40gmail.com

Re: [PATCH net 06/11] net: au1000: assign a valid MAC address without platform data

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The code change is correct, but the commit message overstates its
  impact.

--

net: au1000: assign a valid MAC address without platform data

This commit adds a call to eth_hw_addr_random() in the no-platform-data
branch of au1000_probe(). Without it, dev_addr stays all zeroes and
eth_validate_addr() refuses to open the device.

> The supported no-platform-data path leaves dev_addr as all zeroes. The
> device can register but eth_validate_addr() prevents it from opening.

[ ... ]

> Fixes: 7718f2c27dbd ("au1000_eth: use eth_hw_addr_random() instead of random_ether_addr()")

[Severity: Low]
The code change itself looks fine. Is "supported" the right word for this
path, though, and does a Fixes: tag fit here?

As far as I can tell, no in-tree configuration reaches the !pd branch in
au1000_probe(). The only "au1000-eth" platform devices in the tree are in
arch/mips/alchemy/common/platform.c, and both set platform data
statically:

au1xxx_eth0_device:
	.platform_data          = &au1xxx_eth0_platform_data,

au1xxx_eth1_device:
	.platform_data          = &au1xxx_eth1_platform_data,

board-mtx1.c only copies replacement pdata into those static structs
through au1xxx_override_eth_cfg(). au1000_eth_driver has no
of_match_table either, so device tree can't create an instance without
pdata.

On in-tree boards, a missing or invalid PROM MAC leaves pd->mac zeroed.
That case already takes the existing else branch:

		if (is_valid_ether_addr(pd->mac)) {
			eth_hw_addr_set(dev, pd->mac);
		} else {
			/* Set a random MAC since no valid provided by platform_data. */
			eth_hw_addr_random(dev);
		}

So it looks like only out-of-tree board code that registers the device
without platform data can hit the zero MAC failure.

Could the commit message describe this as a latent fix for users without
pdata? Without in-tree users, would net-next without the Fixes: tag be a
better fit than a stable backport?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027223140.3774570-1-f.fainelli%40gmail.com

Re: [PATCH net 07/11] net: au1000: fix buffer descriptor free list insertion

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message and Fixes: tag present this as a functional
  bug fix, but the bug never affects runtime behaviour.

--

net: au1000: fix buffer descriptor free list insertion

The patch changes au1000_ReleaseDB() so the returned descriptor is pushed
onto the head of aup->pDBfree with its pnext pointing at the old head.
This matches au1000_GetFreeDB() and the list setup loop in au1000_probe().
It is tagged as fixing the initial git import.

> ReleaseDB links the old head to the returned descriptor and makes the
> returned descriptor the new head without updating its stale next
> pointer. This loses free descriptors and can create cycles. Link the
> returned descriptor to the old head instead, matching GetFreeDB and the
> list initialization.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

> diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
> index c0e38a507f892..f8976e324f0e7 100644
> --- a/drivers/net/ethernet/amd/au1000_eth.c
> +++ b/drivers/net/ethernet/amd/au1000_eth.c
> @@ -583,9 +583,7 @@ static struct db_dest *au1000_GetFreeDB(struct au1000_private *aup)
>
>  static void au1000_ReleaseDB(struct au1000_private *aup, struct db_dest *pDB)
>  {
> -	struct db_dest *pDBfree = aup->pDBfree;
> -	if (pDBfree)
> -		pDBfree->pnext = pDB;
> +	pDB->pnext = aup->pDBfree;
>  	aup->pDBfree = pDB;
>  }

[Severity: Low]
The new linking looks correct. Can the old behaviour actually be seen at
runtime, though?

au1000_GetFreeDB() is only called during the first ring fill in
au1000_probe(), before any au1000_ReleaseDB() call can happen.

au1000_ReleaseDB() is only called from two teardown paths. In both, the
next steps free the descriptor buffers and the au1000_private that holds
pDBfree and db[]. The first is the au1000_probe() error path:

au1000_probe() {
    ...
	for (i = 0; i < NUM_RX_DMA; i++) {
		if (aup->rx_db_inuse[i])
			au1000_ReleaseDB(aup, aup->rx_db_inuse[i]);
	}
	for (i = 0; i < NUM_TX_DMA; i++) {
		if (aup->tx_db_inuse[i])
			au1000_ReleaseDB(aup, aup->tx_db_inuse[i]);
	}
    ...
	dma_free_coherent(&pdev->dev, MAX_BUF_SIZE * (NUM_TX_BUFFS + NUM_RX_BUFFS),
			  aup->vaddr, aup->dma_addr);
err_vaddr:
	free_netdev(dev);
    ...
}

The second is au1000_remove(), which runs the same release loops and then
calls dma_free_coherent() and free_netdev(dev).

So nothing ever walks or pops the list that the old code corrupted. The
lost descriptors and cycles in the commit message would not cause a leak,
a hang or a wrong DMA buffer.

Could the commit message say that there is no runtime impact, and could
the Fixes: tag be dropped so this does not go to stable and the net tree as
a functional fix? Another option is to remove the au1000_ReleaseDB() calls
from the teardown paths, since they have no effect.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027223140.3774570-1-f.fainelli%40gmail.com

Re: [PATCH net 08/11] net: au1000: restore receive filtering after MAC initialization

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In au1000_init() (drivers/net/ethernet/amd/au1000_eth.c), the
  control register is written with MAC_RX_ENABLE | MAC_TX_ENABLE and no…

--

net: au1000: restore receive filtering after MAC initialization

This patch reapplies the current netdev receive mode at the end of
au1000_init(). Promiscuous and multicast settings are then kept after a
watchdog reset. It also takes aup->lock around the control register
read-modify-write in au1000_multicast_list().

> diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
> index f8976e324f0e7..6b817d18329b2 100644
> --- a/drivers/net/ethernet/amd/au1000_eth.c
> +++ b/drivers/net/ethernet/amd/au1000_eth.c

[ ... ]

> @@ -742,6 +744,9 @@ static int au1000_init(struct net_device *dev)
>  	wmb(); /* drain writebuffer */
>  
>  	spin_unlock_irqrestore(&aup->lock, flags);
> +	netif_addr_lock_bh(dev);
> +	au1000_multicast_list(dev);
> +	netif_addr_unlock_bh(dev);

[Severity: Low]
Is there a short window here where the receiver runs without the
requested filter?

A little earlier, au1000_init() enables RX and TX with none of the filter
bits set:

	if (!dev->phydev || dev->phydev->link)
		control |= MAC_RX_ENABLE | MAC_TX_ENABLE;

	writel(control, &aup->mac->control);

Then aup->lock is dropped. Only after that does au1000_multicast_list()
program multi_hash_high/low and set MAC_HASH_MODE, MAC_PROMISCUOUS or
MAC_PASS_ALL_MULTI, using a second read-modify-write.

This can happen on the au1000_tx_timeout()->au1000_reset_mac()->au1000_init()
path when the link is up, or on open when there is no PHY. In between the
two writes, frames for joined multicast groups, or non-local frames in
promiscuous mode, could be dropped.

The window is small next to the reset delays that come before it. On open
it is also shorter than before, since __dev_open() calls dev_set_rx_mode()
only after ndo_open. The window is still there at the end of the series in
"net: au1000: keep the MAC stopped until the PHY link is ready" when there
is no PHY or the link is up.

Would it be worth computing the filter bits and adding them to control
before the single writel()? Then enabling RX and programming the filter
would happen in one step.

>  	return 0;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027223140.3774570-1-f.fainelli%40gmail.com

Re: [PATCH net 10/11] net: au1000: keep the MAC stopped until the PHY link is ready

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · 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).

- [Medium] au1000_open() now calls phy_start() before au1000_init().
- [Medium] Reopening the interface after a PHY status read that changed
  phydev->link and then failed can leave MAC RX/TX disabled for good…
- [Low] The new au1000_hard_stop() call in the link-down branch of
  au1000_adjust_link() runs inside spin_lock_irqsave(&aup->lock).

--

net: au1000: keep the MAC stopped until the PHY link is ready

The patch moves phy_start() ahead of au1000_init() in au1000_open(). It
also leaves MAC RX/TX disabled in au1000_init() while the PHY reports no
link, and stops the MAC in the link-down branch of au1000_adjust_link().
If au1000_init() fails, the PHY is now stopped.

> diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
> index 8c2dbf72652e9..c616ce616541f 100644
> --- a/drivers/net/ethernet/amd/au1000_eth.c
> +++ b/drivers/net/ethernet/amd/au1000_eth.c
> @@ -458,7 +458,8 @@ au1000_adjust_link(struct net_device *dev)
>  		/* link state changed */
>  
>  		if (!phydev->link) {
> -			/* link went down */
> +			/* Stop the MAC before the PHY can remove its clocks. */
> +			au1000_hard_stop(dev);

[Severity: Low]

au1000_adjust_link() holds spin_lock_irqsave(&aup->lock) at this point,
and au1000_hard_stop() ends with:

	wmb(); /* drain writebuffer */
	mdelay(10);

So each link-down now busy-waits 10 ms with interrupts off.

On close, the sequence looks like this:

au1000_close()
  phy_stop()
    -> au1000_adjust_link() -> au1000_hard_stop()   /* 10 ms */
  spin_lock_irqsave(&aup->lock, flags)
  au1000_reset_mac_unlocked()
    au1000_hard_stop()                              /* 10 ms again */
    two mdelay(2) for the reset                     /* 4 ms */

That is roughly 24 ms per close with interrupts off. The duplex branch
already does something similar on link-up.

Is the second au1000_hard_stop() in au1000_reset_mac_unlocked() still
needed now that the link-down callback stops the MAC?

>  			aup->old_speed = 0;
>  			aup->old_duplex = -1;
>  		}

[ ... ]

> @@ -740,6 +741,10 @@ static int au1000_init(struct net_device *dev)
>  		control |= MAC_FULL_DUPLEX;
>  	}
>  
> +	/* The PHY may still be restoring its MII clocks after resume. */
> +	if (!dev->phydev || dev->phydev->link)
> +		control |= MAC_RX_ENABLE | MAC_TX_ENABLE;
> +

[Severity: Medium]

Can the MAC stay stopped after a reopen if old_link and old_duplex are
stale?

With this change, RX/TX get turned back on after open only through
au1000_adjust_link(). It calls au1000_enable_rx_tx() in just one place,
the duplex branch:

	if (phydev->link && (aup->old_duplex != phydev->duplex)) {
		...
		au1000_enable_rx_tx(dev);

Neither au1000_close() nor au1000_reset_mac_unlocked() resets old_link,
old_speed or old_duplex.

Here is one sequence that could leave them stale. Assume autoneg is
disabled and the link is up at 100/Full:

genphy_read_status()
  genphy_update_link()        /* BMSR says link down, phydev->link = 0 */
  genphy_read_status_fixed()  /* BMCR read fails with -ETIMEDOUT */
phy_check_link_status()
  returns the error before phy_link_down()

MDIO timeouts reach phylib since "net: au1000: report MDIO transaction
timeouts". The PHY_ERROR handling, and a later phy_stop() into
PHY_HALTED, call phy_link_down() only if phydev->link is still set.
au1000_adjust_link() therefore never sees the link drop, so old_link
stays 1 and old_duplex stays DUPLEX_FULL.

On the next open, au1000_init() leaves RX/TX disabled because
phydev->link is 0. When the link comes back at the same speed and
duplex, au1000_adjust_link() skips all three branches. The MAC stays
stopped while carrier is reported up.

Before this patch, au1000_init() always set MAC_RX_ENABLE |
MAC_TX_ENABLE, so this case recovered on reopen. Would resetting
old_link, old_speed and old_duplex in the open or close path avoid it?

>  	writel(control, &aup->mac->control);
>  	writel(0x8100, &aup->mac->vlan1_tag); /* activate vlan support */
>  	wmb(); /* drain writebuffer */
> @@ -928,16 +933,18 @@ static int au1000_open(struct net_device *dev)
>  		return retval;
>  	}
>  
> +	if (dev->phydev)
> +		phy_start(dev->phydev);
> +
>  	retval = au1000_init(dev);

[Severity: Medium]

Is it safe to start the PHY before the MAC is initialized?
Documentation/networking/phy.rst says:

  Lastly, once the controller is ready to handle network traffic, you call
  phy_start(phydev).

The commit message says "Start the PHY before initializing the MAC" but
doesn't explain why the order has to change.

phy_start() resumes the PHY over MDIO:

au1000_mdiobus_read() / au1000_mdiobus_write()
  au1000_enable_mac(dev, 0)

mac_enabled is 0 after probe or close, so this takes the MAC out of reset
before au1000_init() runs. That contradicts the comment above
au1000_init():

 * conserve power. Thus, whenever au1000_init() is called,
 * the device should already be in reset state.

phy_start() then queues the state machine with zero delay, and
au1000_init() does:

	/* bring the device out of reset */
	au1000_enable_mac(dev, 1);

With force_reset set, this holds the MAC block in reset for about 2 ms,
including mii_control and mii_data. It takes only aup->lock, not the
mii_bus lock. au1000_mdio_read() and au1000_mdio_write() program
mii_control, poll with mdelay(1) and read mii_data without any driver
lock.

Could that forced reset land in the middle of an MDIO transaction from
the phylib worker, for example on a preemptible UP kernel? A read could
then return a reset value, a BMCR or advertisement write could be lost,
or -ETIMEDOUT could put the PHY into PHY_ERROR.

If the worker runs first and already sees link up, au1000_adjust_link()
takes the duplex branch (old_duplex == -1) and calls
au1000_enable_rx_tx(). That happens before the MAC address and RX
descriptors are programmed. au1000_init() then force-resets a MAC with
RX/TX enabled.

There is also a problem with the new check in au1000_init(). It reads
phydev->link under aup->lock, while phylib updates it under
phydev->lock:

	if (!dev->phydev || dev->phydev->link)
		control |= MAC_RX_ENABLE | MAC_TX_ENABLE;

genphy_update_link() can set link to 1 for a moment and then clear it:

	phydev->link = status & BMSR_LSTATUS ? 1 : 0;
	...
	if (phydev->autoneg == AUTONEG_ENABLE && !phydev->autoneg_complete)
		phydev->link = 0;

If au1000_init() reads that brief 1, RX/TX get enabled. The link-down
callback never runs afterwards, because phydev->link still equals
old_link.

Wouldn't that leave the MAC running with no link, which is the state
this patch is meant to prevent?

>  	if (retval) {
>  		netdev_err(dev, "error in au1000_init\n");
> +		if (dev->phydev)
> +			phy_stop(dev->phydev);
>  		free_irq(dev->irq, dev);
>  		return retval;
>  	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027223140.3774570-1-f.fainelli%40gmail.com

Re: [PATCH net 03/11] net: au1000: disconnect the PHY before freeing the MDIO bus

[Florian Fainelli]

On 10/29/26 09:15, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0 (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).
> 
> - [Medium] In drivers/net/ethernet/amd/au1000_eth.c, au1000_remove() now
>    calls phy_disconnect() after unregister_netdev().
> 
> Pre-existing issues:
> - [Low] The two new phy_disconnect() calls (au1000_remove() after
>    unregister_netdev(), and the au1000_probe() err_out path) run without…
> 
> --
> 
> net: au1000: disconnect the PHY before freeing the MDIO bus
> 
> This patch adds phy_disconnect() calls to the au1000_probe() error path
> and to au1000_remove(). This balances the phy_connect() done in
> au1000_mii_probe(). In both paths the disconnect runs before the MDIO bus
> is unregistered.
> 
>> diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
>> index 58bc8e1b539d5..70319b432182f 100644
>> --- a/drivers/net/ethernet/amd/au1000_eth.c
>> +++ b/drivers/net/ethernet/amd/au1000_eth.c
>> @@ -1287,6 +1287,8 @@ static int au1000_probe(struct platform_device *pdev)
>>   	return 0;
>>   
>>   err_out:
>> +	if (dev->phydev)
>> +		phy_disconnect(dev->phydev);
>>   	if (aup->mii_bus)
>>   		mdiobus_unregister(aup->mii_bus);
>>   
> 
> [ ... ]
> 
>> @@ -1334,6 +1336,8 @@ static void au1000_remove(struct platform_device *pdev)
>>   	struct resource *base, *macen;
>>   
>>   	unregister_netdev(dev);
>> +	if (dev->phydev)
>> +		phy_disconnect(dev->phydev);
> 
> [Severity: Medium]
> Can this leave the MAC clocked and out of reset after the driver unbinds?
> 
> By the time au1000_remove() reaches this point, the MAC is already in
> reset with its clock gated, and aup->mac_enabled is 0. If the interface
> was never opened, au1000_probe() did this by calling au1000_reset_mac()
> just before register_netdev(). If the interface was opened,
> au1000_close() did it.
> 
> If the interface was never opened, phydev->suspended is still false,
> because phy_attach_direct() resumed the PHY. In that case phy_disconnect()
> does MDIO accesses:
> 
> phy_disconnect()
>    phy_detach()
>      phy_detach_internal()
>        phy_suspend()
>          genphy_suspend()
>            phy_set_bits(phydev, MII_BMCR, BMCR_PDOWN)
> 
> Each of these accesses goes through au1000_mdiobus_read() or
> au1000_mdiobus_write(). Both call au1000_enable_mac(dev, 0) first:
> 
> 	if (force_reset || (!aup->mac_enabled)) {
> 		writel(MAC_EN_CLOCK_ENABLE, aup->enable);
> 		...
> 		writel((MAC_EN_RESET0 | MAC_EN_RESET1 | MAC_EN_RESET2
> 				| MAC_EN_CLOCK_ENABLE), aup->enable);
> 		...
> 		aup->mac_enabled = 1;
> 	}
> 
> Nothing later in au1000_remove() calls au1000_reset_mac() again before
> the registers are unmapped and the netdev is freed. Before this patch,
> remove left the MAC in reset with its clock gated.
> 
> The err_out path in au1000_probe() does call au1000_reset_mac(dev) after
> phy_disconnect() and mdiobus_unregister(). Should au1000_remove() also
> call au1000_reset_mac(dev) after phy_disconnect(), to match it?
> 
> [Severity: Low]
> This is a pre-existing issue, not one introduced by this patch, but both
> new phy_disconnect() calls run without RTNL held. In au1000_remove(),
> unregister_netdev() has already dropped RTNL. au1000_probe() never takes
> it.
> 
> phy_disconnect()->phy_detach()->phy_detach_internal() does:
> 
> 		hwprov = rtnl_dereference(dev->hwprov);
> 
> With CONFIG_PROVE_RCU enabled, could this emit a one-time "suspicious
> rcu_dereference_protected() usage" warning? The read itself looks
> harmless, since au1000 never installs a hwtstamp provider.
> 
> Other drivers already call phy_disconnect() after unregister_netdev()
> without RTNL, for example r6040_remove_one(), et131x_pci_remove(),
> bgmac_enet_remove(), mlxbf_gige_remove(), hisi_femac_drv_remove() and
> ixp4xx_eth_remove(). So this looks like a problem with the phylib
> annotation, not with this driver.
> 
>>   	mdiobus_unregister(aup->mii_bus);
>>   	mdiobus_free(aup->mii_bus);
>>
> 

Agreed. PHY teardown can re-enable the MAC through MDIO accesses. I’ll 
update patch #3 to call au1000_reset_mac() after MDIO bus teardown, 
matching the probe error path.

The RTNL annotation concern appears to be a separate phylib issue shared 
by other drivers.

pw-bot: cr
-- 
Florian

Re: [PATCH net 04/11] net: au1000: report MDIO transaction timeouts

[Florian Fainelli]

On 10/29/26 09:15, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] The new completion poll in au1000_mdio_write() reads MAC_MII_BUSY
>    right after `writel(mii_control, mii_control_reg)`.
> 
> Pre-existing issues:
> - [Low] On success, au1000_mdio_read() returns the raw 32-bit
>    MAC_MII_DATA value (`return readl(mii_data_reg);`, au1000_eth.c:298)…
> 
> --
> 
> net: au1000: report MDIO transaction timeouts
> 
> This commit changes au1000_mdio_write() to return an int. Timeouts from
> the busy wait before the transaction, and from a new completion poll after
> the write, now go back to phylib through au1000_mdiobus_write(). The read
> timeout paths in au1000_mdio_read() now return -ETIMEDOUT instead of -1.
> 
>> diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
>> index 70319b432182f..a507a524370fd 100644
>> --- a/drivers/net/ethernet/amd/au1000_eth.c
>> +++ b/drivers/net/ethernet/amd/au1000_eth.c
> 
> [ ... ]
> 
>> @@ -292,14 +292,14 @@ static int au1000_mdio_read(struct net_device *dev, int phy_addr, int reg)
>>   		mdelay(1);
>>   		if (--timedout == 0) {
>>   			netdev_err(dev, "mdio_read busy timeout!!\n");
>> -			return -1;
>> +			return -ETIMEDOUT;
>>   		}
>>   	}
>>   	return readl(mii_data_reg);
> 
> [Severity: Low]
> This isn't a bug introduced by this patch, but should au1000_mdio_read()
> mask the data register value to 16 bits before returning it?
> 
> On success the raw 32-bit MAC_MII_DATA value goes back through
> au1000_mdiobus_read() to phylib. __mdiobus_read() expects either a
> register value or a negative errno, and it passes the value through
> without masking:
> 
> drivers/net/phy/mdio_bus.c:__mdiobus_read() {
>      ...
> 	if (bus->read)
> 		retval = bus->read(bus, addr, regnum);
>      ...
> }
> 
> If bits 31:16 of mii_data ever read back non-zero, phylib would get a
> value wider than 16 bits. If bit 31 were set, it would read as an error.
> 
> Nothing in the tree shows whether those upper bits are ever non-zero on
> this hardware. Would something like readl(mii_data_reg) & 0xffff be
> safer, now that negative values on this path are proper errnos?
> 
>>   }
> 
> [ ... ]
> 
>> @@ -320,6 +320,17 @@ static void au1000_mdio_write(struct net_device *dev, int phy_addr,
>>   
>>   	writel(value, mii_data_reg);
>>   	writel(mii_control, mii_control_reg);
>> +
>> +	timedout = 20;
>> +	while (readl(mii_control_reg) & MAC_MII_BUSY) {
>> +		mdelay(1);
> 
> [Severity: Low]
> With this new completion poll, will every MDIO write now busy-wait for at
> least 1 ms?
> 
> A clause 22 management frame is about 64 MDC cycles, which is roughly
> 26 us at 2.5 MHz. So the first readl() right after
> writel(mii_control, mii_control_reg) will almost always see MAC_MII_BUSY
> set, and mdelay(1) will run at least once.
> 
> The mii_bus->write callback runs in process context under
> bus->mdio_lock, which is a mutex. Could this use readl_poll_timeout()
> with a sleep of a few tens of microseconds and the same ~20 ms timeout?
> 
> To be fair, before this patch the access that followed a write already
> paid this mdelay(1) in its own BUSY wait before starting. So for
> back-to-back sequences the cost mostly moves into the write. The new cost
> falls on the last write of a burst and on isolated writes. The existing
> completion poll in au1000_mdio_read() has the same 1 ms granularity.

Agreed on the completion polling. I’ll use readl_poll_timeout() for the 
new write completion wait, retaining the 20 ms timeout.

The upper-bit concern is pre-existing and speculative without hardware 
evidence, so I’ll leave that unchanged in this series.

pw-bot: cr
-- 
Florian

Re: [PATCH net 05/11] net: au1000: serialize TX completion with packet submission

[Florian Fainelli]

On 10/29/26 09:15, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 (1) · High: 0 · Medium: 2 · 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).
> 
> - [Medium] au1000_tx() in drivers/net/ethernet/amd/au1000_eth.c now frees
>    the skb with hardware IRQs disabled.
> - [Medium] After this patch, au1000_tx()'s inline reclaim path leaves
>    aup->tx_tail permanently out of step with the hardware.
> 
> Pre-existing issues:
> - [Critical] This problem wasn't introduced by this patch, but
>    au1000_tx() copies skb->len bytes into a fixed MAX_BUF_SIZE…
> 
> --
> 
> net: au1000: serialize TX completion with packet submission
> 
> This commit takes the existing IRQ-safe aup->lock in both au1000_tx_ack()
> and au1000_tx(). The goal is to serialize TX descriptor reclamation in hard
> IRQ context with packet submission in ndo_start_xmit. It also closes a
> missed-wakeup window between netif_stop_queue() and setting tx_full.
> 
>> diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
>> index a507a524370fd..90887dac26bc9 100644
>> --- a/drivers/net/ethernet/amd/au1000_eth.c
>> +++ b/drivers/net/ethernet/amd/au1000_eth.c
> 
> [ ... ]
> 
>> @@ -973,17 +977,20 @@ static netdev_tx_t au1000_tx(struct sk_buff *skb, struct net_device *dev)
>>   	u32 buff_stat;
>>   	struct db_dest *pDB;
>>   	int i;
>> +	unsigned long flags;
>>   
>>   	netif_dbg(aup, tx_queued, dev, "tx: aup %x len=%d, data=%p, head %d\n",
>>   				(unsigned)aup, skb->len,
>>   				skb->data, aup->tx_head);
>>   
>> +	spin_lock_irqsave(&aup->lock, flags);
>>   	ptxd = aup->tx_dma_ring[aup->tx_head];
>>   	buff_stat = ptxd->buff_stat;
>>   	if (buff_stat & TX_DMA_ENABLE) {
>>   		/* We've wrapped around and the transmitter is still busy */
>>   		netif_stop_queue(dev);
>>   		aup->tx_full = 1;
>> +		spin_unlock_irqrestore(&aup->lock, flags);
>>   		return NETDEV_TX_BUSY;
>>   	} else if (buff_stat & TX_T_DONE) {
>>   		au1000_update_tx_stats(dev, ptxd->status);
> 
> [Severity: Medium]
> 
> Now that the lock is held here, can this inline reclaim leave aup->tx_tail
> permanently out of step with the hardware?
> 
> If desc[tx_head] has TX_T_DONE set at this point, the ring is full
> (tx_head == tx_tail) and the oldest buffer has just completed. This branch
> updates stats and zeroes len. Later in au1000_tx() the code re-arms the
> descriptor, which clears the done bit, and advances tx_head:
> 
> 	ptxd->buff_stat = lower_32_bits(pDB->dma_addr) | TX_DMA_ENABLE;
> 	...
> 	aup->tx_head = (aup->tx_head + 1) & (NUM_TX_DMA - 1);
> 
> tx_tail is never advanced on this path.
> 
> Before this patch, on the uniprocessor Au1x00, the pending MAC DMA
> interrupt could run au1000_tx_ack() before the re-arm and move tx_tail
> forward. Now the ISR is held off until spin_unlock_irqrestore(). At that
> point au1000_tx_ack() finds desc[tx_tail] re-armed with no done bit:
> 
> 	while (ptxd->buff_stat & TX_T_DONE) {
> 
> So it returns without moving tx_tail, which now points at the newest
> submitted descriptor.
> 
> au1000_init() sets tx_head from the hardware's own buffer index, so the
> hardware completes descriptors in index order and that newest descriptor
> completes last. Wouldn't that leave the TX_T_DONE bits of older completed
> descriptors unacknowledged until then?
> 
> It would also delay netif_wake_queue() in au1000_tx_ack() until the whole
> 4-entry ring drains, rather than after one completion. The offset would
> then persist until au1000_init() resets head and tail.
> 
> The same thing can happen when au1000_tx() is called with IRQs already
> disabled, for example from netpoll.
> 
> The MAC DMA interrupt is IRQ_TYPE_LEVEL_HIGH, and au1000_interrupt() always
> returns IRQ_RETVAL(1). If unacknowledged done bits keep the line asserted,
> could this become an interrupt storm? That part depends on hardware
> behaviour that has not been confirmed.
> 
> [ ... ]
> 
> [Severity: Critical]
> 
> This is a pre-existing issue, not one introduced by this patch.
> au1000_tx() copies skb->len bytes into a fixed MAX_BUF_SIZE (2048 byte)
> bounce buffer without checking the length:
> 
> 	pDB = aup->tx_db_inuse[aup->tx_head];
> 	skb_copy_from_linear_data(skb, (void *)pDB->vaddr, skb->len);
> 
> Can skb->len be larger than 2048 here? When po->vnet_hdr_sz is nonzero,
> tpacket_snd() in net/packet/af_packet.c skips both MTU checks:
> 
> 	if ((size_max > dev->mtu + reserve + VLAN_HLEN) && !vnet_hdr_sz)
> 		size_max = dev->mtu + reserve + VLAN_HLEN;
> 	...
> 		if (likely(tp_len >= 0) &&
> 		    tp_len > dev->mtu + reserve &&
> 		    !vnet_hdr_sz &&
> 		    !packet_extra_vlan_len_allowed(dev, skb))
> 
> An all-zero virtio_net_hdr (GSO_NONE, no NEEDS_CSUM) is accepted without
> making the skb GSO. validate_xmit_skb() then only linearizes it, because
> au1000 does not advertise NETIF_F_SG, and applies no MTU check.
> 
> With CAP_NET_RAW, this path could deliver a multi-kilobyte skb to
> au1000_tx():
> 
> socket(AF_PACKET, SOCK_RAW)
>    setsockopt(PACKET_VNET_HDR)
>    setsockopt(PACKET_TX_RING) with tp_frame_size > 2048
>    tpacket_snd()
>      packet_xmit()
>        validate_xmit_skb()
>          au1000_tx()
>            skb_copy_from_linear_data(..., skb->len)
> 
> au1000_probe() allocates a single 16 KiB region with dma_alloc_coherent().
> The TX buffers are at offsets 0-8K and the RX buffers at 8-16K. Would an
> oversized frame overwrite the neighbouring TX/RX DMA buffers? Beyond about
> 10 KiB, would it write past the end of the coherent allocation?
> 
> Should au1000_tx() drop frames larger than MAX_BUF_SIZE?
> 
>> @@ -1012,6 +1019,7 @@ static netdev_tx_t au1000_tx(struct sk_buff *skb, struct net_device *dev)
>>   	wmb(); /* drain writebuffer */
>>   	dev_kfree_skb(skb);
>>   	aup->tx_head = (aup->tx_head + 1) & (NUM_TX_DMA - 1);
>> +	spin_unlock_irqrestore(&aup->lock, flags);
>>   	return NETDEV_TX_OK;
>>   }
> 
> [Severity: Medium]
> 
> With the new spin_lock_irqsave() in au1000_tx(), dev_kfree_skb(skb) now
> runs with hard IRQs disabled. dev_kfree_skb() is just consume_skb():
> 
> include/linux/skbuff.h:
> #define dev_kfree_skb(a)	consume_skb(a)
> 
> include/linux/netdevice.h says:
> 
>   * It is not allowed to call kfree_skb() or consume_skb() from hardware
>   * interrupt context or with hardware interrupts being disabled.
>   * (in_hardirq() || irqs_disabled())
> 
> Does this mean every transmitted packet now runs its skb destructors with
> IRQs off? That includes sock_wfree()/tcp_wfree(), zerocopy ubuf callbacks,
> and dst and conntrack puts.
> 
> If any of those reach local_bh_enable(), lockdep_assert_irqs_enabled()
> would fire. With CONFIG_TRACE_IRQFLAGS, __local_bh_enable_ip() could also
> turn IRQs back on inside the aup->lock section, which would reopen the
> race this patch is closing.
> 
> The data has already been copied into pDB->vaddr. Could the free move
> after spin_unlock_irqrestore(), or use dev_consume_skb_any() instead?
> 

Agreed on both medium findings. I’ll advance tx_tail during inline 
reclaim and move SKB consumption after unlocking, using 
dev_consume_skb_any() for callers entering with IRQs disabled.

I’ll also add a separate fix rejecting oversized frames before the 
bounce-buffer copy. The possible interrupt storm remains unconfirmed.

pw-bot: cr
-- 
Florian

Re: [PATCH net 06/11] net: au1000: assign a valid MAC address without platform data

[Florian Fainelli]

On 10/29/26 09:15, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] The code change is correct, but the commit message overstates its
>    impact.
> 
> --
> 
> net: au1000: assign a valid MAC address without platform data
> 
> This commit adds a call to eth_hw_addr_random() in the no-platform-data
> branch of au1000_probe(). Without it, dev_addr stays all zeroes and
> eth_validate_addr() refuses to open the device.
> 
>> The supported no-platform-data path leaves dev_addr as all zeroes. The
>> device can register but eth_validate_addr() prevents it from opening.
> 
> [ ... ]
> 
>> Fixes: 7718f2c27dbd ("au1000_eth: use eth_hw_addr_random() instead of random_ether_addr()")
> 
> [Severity: Low]
> The code change itself looks fine. Is "supported" the right word for this
> path, though, and does a Fixes: tag fit here?
> 
> As far as I can tell, no in-tree configuration reaches the !pd branch in
> au1000_probe(). The only "au1000-eth" platform devices in the tree are in
> arch/mips/alchemy/common/platform.c, and both set platform data
> statically:
> 
> au1xxx_eth0_device:
> 	.platform_data          = &au1xxx_eth0_platform_data,
> 
> au1xxx_eth1_device:
> 	.platform_data          = &au1xxx_eth1_platform_data,
> 
> board-mtx1.c only copies replacement pdata into those static structs
> through au1xxx_override_eth_cfg(). au1000_eth_driver has no
> of_match_table either, so device tree can't create an instance without
> pdata.
> 
> On in-tree boards, a missing or invalid PROM MAC leaves pd->mac zeroed.
> That case already takes the existing else branch:
> 
> 		if (is_valid_ether_addr(pd->mac)) {
> 			eth_hw_addr_set(dev, pd->mac);
> 		} else {
> 			/* Set a random MAC since no valid provided by platform_data. */
> 			eth_hw_addr_random(dev);
> 		}
> 
> So it looks like only out-of-tree board code that registers the device
> without platform data can hit the zero MAC failure.
> 
> Could the commit message describe this as a latent fix for users without
> pdata? Without in-tree users, would net-next without the Fixes: tag be a
> better fit than a stable backport?
> 

Agreed that “supported” overstates the in-tree impact. I’ll describe 
this as a latent regression in the no-platform-data fallback.

I’ll retain the Fixes tag because that commit removed the unconditional 
random-address initialization, introducing this behavior.
-- 
Florian

Re: [PATCH net 07/11] net: au1000: fix buffer descriptor free list insertion

[Florian Fainelli]

On 10/29/26 09:15, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] The commit message and Fixes: tag present this as a functional
>    bug fix, but the bug never affects runtime behaviour.
> 
> --
> 
> net: au1000: fix buffer descriptor free list insertion
> 
> The patch changes au1000_ReleaseDB() so the returned descriptor is pushed
> onto the head of aup->pDBfree with its pnext pointing at the old head.
> This matches au1000_GetFreeDB() and the list setup loop in au1000_probe().
> It is tagged as fixing the initial git import.
> 
>> ReleaseDB links the old head to the returned descriptor and makes the
>> returned descriptor the new head without updating its stale next
>> pointer. This loses free descriptors and can create cycles. Link the
>> returned descriptor to the old head instead, matching GetFreeDB and the
>> list initialization.
>>
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> 
>> diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
>> index c0e38a507f892..f8976e324f0e7 100644
>> --- a/drivers/net/ethernet/amd/au1000_eth.c
>> +++ b/drivers/net/ethernet/amd/au1000_eth.c
>> @@ -583,9 +583,7 @@ static struct db_dest *au1000_GetFreeDB(struct au1000_private *aup)
>>
>>   static void au1000_ReleaseDB(struct au1000_private *aup, struct db_dest *pDB)
>>   {
>> -	struct db_dest *pDBfree = aup->pDBfree;
>> -	if (pDBfree)
>> -		pDBfree->pnext = pDB;
>> +	pDB->pnext = aup->pDBfree;
>>   	aup->pDBfree = pDB;
>>   }
> 
> [Severity: Low]
> The new linking looks correct. Can the old behaviour actually be seen at
> runtime, though?
> 
> au1000_GetFreeDB() is only called during the first ring fill in
> au1000_probe(), before any au1000_ReleaseDB() call can happen.
> 
> au1000_ReleaseDB() is only called from two teardown paths. In both, the
> next steps free the descriptor buffers and the au1000_private that holds
> pDBfree and db[]. The first is the au1000_probe() error path:
> 
> au1000_probe() {
>      ...
> 	for (i = 0; i < NUM_RX_DMA; i++) {
> 		if (aup->rx_db_inuse[i])
> 			au1000_ReleaseDB(aup, aup->rx_db_inuse[i]);
> 	}
> 	for (i = 0; i < NUM_TX_DMA; i++) {
> 		if (aup->tx_db_inuse[i])
> 			au1000_ReleaseDB(aup, aup->tx_db_inuse[i]);
> 	}
>      ...
> 	dma_free_coherent(&pdev->dev, MAX_BUF_SIZE * (NUM_TX_BUFFS + NUM_RX_BUFFS),
> 			  aup->vaddr, aup->dma_addr);
> err_vaddr:
> 	free_netdev(dev);
>      ...
> }
> 
> The second is au1000_remove(), which runs the same release loops and then
> calls dma_free_coherent() and free_netdev(dev).
> 
> So nothing ever walks or pops the list that the old code corrupted. The
> lost descriptors and cycles in the commit message would not cause a leak,
> a hang or a wrong DMA buffer.
> 
> Could the commit message say that there is no runtime impact, and could
> the Fixes: tag be dropped so this does not go to stable and the net tree as
> a functional fix? Another option is to remove the au1000_ReleaseDB() calls
> from the teardown paths, since they have no effect.
> 

Agreed. The free list is never consumed after au1000_ReleaseDB() runs, 
so the incorrect linking has no runtime impact. I’ll drop this patch 
from the fixes series and leave removal of the redundant teardown 
bookkeeping for separate cleanup.

pw-bot: cr
-- 
Florian

Re: [PATCH net 08/11] net: au1000: restore receive filtering after MAC initialization

[Florian Fainelli]

On 10/29/26 09:15, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] In au1000_init() (drivers/net/ethernet/amd/au1000_eth.c), the
>    control register is written with MAC_RX_ENABLE | MAC_TX_ENABLE and no…
> 
> --
> 
> net: au1000: restore receive filtering after MAC initialization
> 
> This patch reapplies the current netdev receive mode at the end of
> au1000_init(). Promiscuous and multicast settings are then kept after a
> watchdog reset. It also takes aup->lock around the control register
> read-modify-write in au1000_multicast_list().
> 
>> diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
>> index f8976e324f0e7..6b817d18329b2 100644
>> --- a/drivers/net/ethernet/amd/au1000_eth.c
>> +++ b/drivers/net/ethernet/amd/au1000_eth.c
> 
> [ ... ]
> 
>> @@ -742,6 +744,9 @@ static int au1000_init(struct net_device *dev)
>>   	wmb(); /* drain writebuffer */
>>   
>>   	spin_unlock_irqrestore(&aup->lock, flags);
>> +	netif_addr_lock_bh(dev);
>> +	au1000_multicast_list(dev);
>> +	netif_addr_unlock_bh(dev);
> 
> [Severity: Low]
> Is there a short window here where the receiver runs without the
> requested filter?
> 
> A little earlier, au1000_init() enables RX and TX with none of the filter
> bits set:
> 
> 	if (!dev->phydev || dev->phydev->link)
> 		control |= MAC_RX_ENABLE | MAC_TX_ENABLE;
> 
> 	writel(control, &aup->mac->control);
> 
> Then aup->lock is dropped. Only after that does au1000_multicast_list()
> program multi_hash_high/low and set MAC_HASH_MODE, MAC_PROMISCUOUS or
> MAC_PASS_ALL_MULTI, using a second read-modify-write.
> 
> This can happen on the au1000_tx_timeout()->au1000_reset_mac()->au1000_init()
> path when the link is up, or on open when there is no PHY. In between the
> two writes, frames for joined multicast groups, or non-local frames in
> promiscuous mode, could be dropped.
> 
> The window is small next to the reset delays that come before it. On open
> it is also shorter than before, since __dev_open() calls dev_set_rx_mode()
> only after ndo_open. The window is still there at the end of the series in
> "net: au1000: keep the MAC stopped until the PHY link is ready" when there
> is no PHY or the link is up.
> 
> Would it be worth computing the filter bits and adding them to control
> before the single writel()? Then enabling RX and programming the filter
> would happen in one step.

Agreed. I’ll share the receive-mode programming logic with 
initialization, program the hash registers before enabling RX, and 
include the filter bits in the same control-register write as RX_ENABLE.

pw-bot: cr
-- 
Florian

Re: [PATCH net 10/11] net: au1000: keep the MAC stopped until the PHY link is ready

[Florian Fainelli]

On 10/29/26 09:15, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · 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).
> 
> - [Medium] au1000_open() now calls phy_start() before au1000_init().
> - [Medium] Reopening the interface after a PHY status read that changed
>    phydev->link and then failed can leave MAC RX/TX disabled for good…
> - [Low] The new au1000_hard_stop() call in the link-down branch of
>    au1000_adjust_link() runs inside spin_lock_irqsave(&aup->lock).
> 
> --
> 
> net: au1000: keep the MAC stopped until the PHY link is ready
> 
> The patch moves phy_start() ahead of au1000_init() in au1000_open(). It
> also leaves MAC RX/TX disabled in au1000_init() while the PHY reports no
> link, and stops the MAC in the link-down branch of au1000_adjust_link().
> If au1000_init() fails, the PHY is now stopped.
> 
>> diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
>> index 8c2dbf72652e9..c616ce616541f 100644
>> --- a/drivers/net/ethernet/amd/au1000_eth.c
>> +++ b/drivers/net/ethernet/amd/au1000_eth.c
>> @@ -458,7 +458,8 @@ au1000_adjust_link(struct net_device *dev)
>>   		/* link state changed */
>>   
>>   		if (!phydev->link) {
>> -			/* link went down */
>> +			/* Stop the MAC before the PHY can remove its clocks. */
>> +			au1000_hard_stop(dev);
> 
> [Severity: Low]
> 
> au1000_adjust_link() holds spin_lock_irqsave(&aup->lock) at this point,
> and au1000_hard_stop() ends with:
> 
> 	wmb(); /* drain writebuffer */
> 	mdelay(10);
> 
> So each link-down now busy-waits 10 ms with interrupts off.
> 
> On close, the sequence looks like this:
> 
> au1000_close()
>    phy_stop()
>      -> au1000_adjust_link() -> au1000_hard_stop()   /* 10 ms */
>    spin_lock_irqsave(&aup->lock, flags)
>    au1000_reset_mac_unlocked()
>      au1000_hard_stop()                              /* 10 ms again */
>      two mdelay(2) for the reset                     /* 4 ms */
> 
> That is roughly 24 ms per close with interrupts off. The duplex branch
> already does something similar on link-up.
> 
> Is the second au1000_hard_stop() in au1000_reset_mac_unlocked() still
> needed now that the link-down callback stops the MAC?
> 
>>   			aup->old_speed = 0;
>>   			aup->old_duplex = -1;
>>   		}
> 
> [ ... ]
> 
>> @@ -740,6 +741,10 @@ static int au1000_init(struct net_device *dev)
>>   		control |= MAC_FULL_DUPLEX;
>>   	}
>>   
>> +	/* The PHY may still be restoring its MII clocks after resume. */
>> +	if (!dev->phydev || dev->phydev->link)
>> +		control |= MAC_RX_ENABLE | MAC_TX_ENABLE;
>> +
> 
> [Severity: Medium]
> 
> Can the MAC stay stopped after a reopen if old_link and old_duplex are
> stale?
> 
> With this change, RX/TX get turned back on after open only through
> au1000_adjust_link(). It calls au1000_enable_rx_tx() in just one place,
> the duplex branch:
> 
> 	if (phydev->link && (aup->old_duplex != phydev->duplex)) {
> 		...
> 		au1000_enable_rx_tx(dev);
> 
> Neither au1000_close() nor au1000_reset_mac_unlocked() resets old_link,
> old_speed or old_duplex.
> 
> Here is one sequence that could leave them stale. Assume autoneg is
> disabled and the link is up at 100/Full:
> 
> genphy_read_status()
>    genphy_update_link()        /* BMSR says link down, phydev->link = 0 */
>    genphy_read_status_fixed()  /* BMCR read fails with -ETIMEDOUT */
> phy_check_link_status()
>    returns the error before phy_link_down()
> 
> MDIO timeouts reach phylib since "net: au1000: report MDIO transaction
> timeouts". The PHY_ERROR handling, and a later phy_stop() into
> PHY_HALTED, call phy_link_down() only if phydev->link is still set.
> au1000_adjust_link() therefore never sees the link drop, so old_link
> stays 1 and old_duplex stays DUPLEX_FULL.
> 
> On the next open, au1000_init() leaves RX/TX disabled because
> phydev->link is 0. When the link comes back at the same speed and
> duplex, au1000_adjust_link() skips all three branches. The MAC stays
> stopped while carrier is reported up.
> 
> Before this patch, au1000_init() always set MAC_RX_ENABLE |
> MAC_TX_ENABLE, so this case recovered on reopen. Would resetting
> old_link, old_speed and old_duplex in the open or close path avoid it?
> 
>>   	writel(control, &aup->mac->control);
>>   	writel(0x8100, &aup->mac->vlan1_tag); /* activate vlan support */
>>   	wmb(); /* drain writebuffer */
>> @@ -928,16 +933,18 @@ static int au1000_open(struct net_device *dev)
>>   		return retval;
>>   	}
>>   
>> +	if (dev->phydev)
>> +		phy_start(dev->phydev);
>> +
>>   	retval = au1000_init(dev);
> 
> [Severity: Medium]
> 
> Is it safe to start the PHY before the MAC is initialized?
> Documentation/networking/phy.rst says:
> 
>    Lastly, once the controller is ready to handle network traffic, you call
>    phy_start(phydev).
> 
> The commit message says "Start the PHY before initializing the MAC" but
> doesn't explain why the order has to change.
> 
> phy_start() resumes the PHY over MDIO:
> 
> au1000_mdiobus_read() / au1000_mdiobus_write()
>    au1000_enable_mac(dev, 0)
> 
> mac_enabled is 0 after probe or close, so this takes the MAC out of reset
> before au1000_init() runs. That contradicts the comment above
> au1000_init():
> 
>   * conserve power. Thus, whenever au1000_init() is called,
>   * the device should already be in reset state.
> 
> phy_start() then queues the state machine with zero delay, and
> au1000_init() does:
> 
> 	/* bring the device out of reset */
> 	au1000_enable_mac(dev, 1);
> 
> With force_reset set, this holds the MAC block in reset for about 2 ms,
> including mii_control and mii_data. It takes only aup->lock, not the
> mii_bus lock. au1000_mdio_read() and au1000_mdio_write() program
> mii_control, poll with mdelay(1) and read mii_data without any driver
> lock.
> 
> Could that forced reset land in the middle of an MDIO transaction from
> the phylib worker, for example on a preemptible UP kernel? A read could
> then return a reset value, a BMCR or advertisement write could be lost,
> or -ETIMEDOUT could put the PHY into PHY_ERROR.
> 
> If the worker runs first and already sees link up, au1000_adjust_link()
> takes the duplex branch (old_duplex == -1) and calls
> au1000_enable_rx_tx(). That happens before the MAC address and RX
> descriptors are programmed. au1000_init() then force-resets a MAC with
> RX/TX enabled.
> 
> There is also a problem with the new check in au1000_init(). It reads
> phydev->link under aup->lock, while phylib updates it under
> phydev->lock:
> 
> 	if (!dev->phydev || dev->phydev->link)
> 		control |= MAC_RX_ENABLE | MAC_TX_ENABLE;
> 
> genphy_update_link() can set link to 1 for a moment and then clear it:
> 
> 	phydev->link = status & BMSR_LSTATUS ? 1 : 0;
> 	...
> 	if (phydev->autoneg == AUTONEG_ENABLE && !phydev->autoneg_complete)
> 		phydev->link = 0;
> 
> If au1000_init() reads that brief 1, RX/TX get enabled. The link-down
> callback never runs afterwards, because phydev->link still equals
> old_link.
> 
> Wouldn't that leave the MAC running with no link, which is the state
> this patch is meant to prevent?
> 
>>   	if (retval) {
>>   		netdev_err(dev, "error in au1000_init\n");
>> +		if (dev->phydev)
>> +			phy_stop(dev->phydev);
>>   		free_irq(dev->irq, dev);
>>   		return retval;
>>   	}
> 
> [ ... ]
> 

Agreed on both medium findings. I’ll initialize the MAC before starting 
the PHY, clear the cached link state on open, and use the driver’s 
acknowledged link state during initialization.

I’ll retain the reset-time hard stop because watchdog recovery and other 
reset paths do not guarantee a preceding link-down callback. Reducing 
the IRQ-off delays needs separate work.

pw-bot: cr
-- 
Florian