[PATCH net-next 0/7] Polling be gone on LAN95xx

[PATCH net-next 0/7] Polling be gone on LAN95xx

[Lukas Wunner]

Do away with link status polling on LAN95XX USB Ethernet
and rely on interrupts instead, thereby reducing bus traffic,
CPU overhead and improving interface bringup latency.

The meat of the series is in patch [5/7].  The preceding and
following patches are various cleanups to prepare for and
adjust to interrupt-driven link state detection.

Please review and test.  Thanks!

Lukas Wunner (7):
  usbnet: Run unregister_netdev() before unbind() again
  usbnet: smsc95xx: Don't clear read-only PHY interrupt
  usbnet: smsc95xx: Don't reset PHY behind PHY driver's back
  usbnet: smsc95xx: Avoid link settings race on interrupt reception
  usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid
    polling
  net: phy: smsc: Cache interrupt mask
  net: phy: smsc: Cope with hot-removal in interrupt handler

 drivers/net/phy/smsc.c         |  28 +++---
 drivers/net/usb/asix_devices.c |   6 +-
 drivers/net/usb/smsc95xx.c     | 155 ++++++++++++++++-----------------
 drivers/net/usb/usbnet.c       |   6 +-
 4 files changed, 91 insertions(+), 104 deletions(-)

-- 
2.35.2

[PATCH net-next 1/7] usbnet: Run unregister_netdev() before unbind() again

[Lukas Wunner]

Commit 2c9d6c2b871d ("usbnet: run unbind() before unregister_netdev()")
sought to fix a use-after-free on disconnect of USB Ethernet adapters.

It turns out that a different fix is necessary to address the issue:
https://lore.kernel.org/netdev/18b3541e5372bc9b9fc733d422f4e698c089077c.1650177997.git.lukas@wunner.de/

So the commit was not necessary.

The commit made binding and unbinding of USB Ethernet asymmetrical:
Before, usbnet_probe() first invoked the ->bind() callback and then
register_netdev().  usbnet_disconnect() mirrored that by first invoking
unregister_netdev() and then ->unbind().

Since the commit, the order in usbnet_disconnect() is reversed and no
longer mirrors usbnet_probe().

One consequence is that a PHY disconnected (and stopped) in ->unbind()
is afterwards stopped once more by unregister_netdev() as it closes the
netdev before unregistering.  That necessitates a contortion in ->stop()
because the PHY may only be stopped if it hasn't already been
disconnected.

Reverting the commit allows making the call to phy_stop() unconditional
in ->stop().

Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Cc: Martyn Welch <martyn.welch@collabora.com>
Cc: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/usb/asix_devices.c | 6 +-----
 drivers/net/usb/smsc95xx.c     | 3 +--
 drivers/net/usb/usbnet.c       | 6 +++---
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 38e47a93fb83..5b5eb630c4b7 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -795,11 +795,7 @@ static int ax88772_stop(struct usbnet *dev)
 {
 	struct asix_common_private *priv = dev->driver_priv;
 
-	/* On unplugged USB, we will get MDIO communication errors and the
-	 * PHY will be set in to PHY_HALTED state.
-	 */
-	if (priv->phydev->state != PHY_HALTED)
-		phy_stop(priv->phydev);
+	phy_stop(priv->phydev);
 
 	return 0;
 }
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 4ef61f6b85df..edf0492ad489 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1243,8 +1243,7 @@ static int smsc95xx_start_phy(struct usbnet *dev)
 
 static int smsc95xx_stop(struct usbnet *dev)
 {
-	if (dev->net->phydev)
-		phy_stop(dev->net->phydev);
+	phy_stop(dev->net->phydev);
 
 	return 0;
 }
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 9a6450f796dc..36b24ec11650 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1616,9 +1616,6 @@ void usbnet_disconnect (struct usb_interface *intf)
 		   xdev->bus->bus_name, xdev->devpath,
 		   dev->driver_info->description);
 
-	if (dev->driver_info->unbind)
-		dev->driver_info->unbind(dev, intf);
-
 	net = dev->net;
 	unregister_netdev (net);
 
@@ -1626,6 +1623,9 @@ void usbnet_disconnect (struct usb_interface *intf)
 
 	usb_scuttle_anchored_urbs(&dev->deferred);
 
+	if (dev->driver_info->unbind)
+		dev->driver_info->unbind(dev, intf);
+
 	usb_kill_urb(dev->interrupt);
 	usb_free_urb(dev->interrupt);
 	kfree(dev->padding_pkt);
-- 
2.35.2

Re: [PATCH net-next 1/7] usbnet: Run unregister_netdev() before unbind() again

[Oliver Neukum]

On 27.04.22 07:48, Lukas Wunner wrote:
> Commit 2c9d6c2b871d ("usbnet: run unbind() before unregister_netdev()")
> sought to fix a use-after-free on disconnect of USB Ethernet adapters.
>
> It turns out that a different fix is necessary to address the issue:
> https://lore.kernel.org/netdev/18b3541e5372bc9b9fc733d422f4e698c089077c.1650177997.git.lukas@wunner.de/
>
> So the commit was not necessary.
>
> The commit made binding and unbinding of USB Ethernet asymmetrical:
> Before, usbnet_probe() first invoked the ->bind() callback and then
> register_netdev().  usbnet_disconnect() mirrored that by first invoking
> unregister_netdev() and then ->unbind().
>
> Since the commit, the order in usbnet_disconnect() is reversed and no
> longer mirrors usbnet_probe().
>
> One consequence is that a PHY disconnected (and stopped) in ->unbind()
> is afterwards stopped once more by unregister_netdev() as it closes the
> netdev before unregistering.  That necessitates a contortion in ->stop()
> because the PHY may only be stopped if it hasn't already been
> disconnected.
>
> Reverting the commit allows making the call to phy_stop() unconditional
> in ->stop().
>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>
> Cc: Martyn Welch <martyn.welch@collabora.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
>
Acked-by: Oliver Neukum <oneukum@suse.com>

Re: [PATCH net-next 1/7] usbnet: Run unregister_netdev() before unbind() again

[Oliver Neukum]

On 27.04.22 07:48, Lukas Wunner wrote:
> Commit 2c9d6c2b871d ("usbnet: run unbind() before unregister_netdev()")
> sought to fix a use-after-free on disconnect of USB Ethernet adapters.
>
>
Hi,

thank you for doing this. We have finally come at least to a partial
solution for this issue.

    Regards
        Oliver

[PATCH net-next 2/7] usbnet: smsc95xx: Don't clear read-only PHY interrupt

[Lukas Wunner]

Upon receiving data from the Interrupt Endpoint, the SMSC LAN95xx driver
attempts to clear the signaled interrupts by writing "all ones" to the
Interrupt Status Register.

However the driver only ever enables a single type of interrupt, namely
the PHY Interrupt.  And according to page 119 of the LAN950x datasheet,
its bit in the Interrupt Status Register is read-only.  There's no other
way to clear it than in a separate PHY register:

https://www.microchip.com/content/dam/mchp/documents/UNG/ProductDocuments/DataSheets/LAN950x-Data-Sheet-DS00001875D.pdf

Consequently, writing "all ones" to the Interrupt Status Register is
pointless and can be dropped.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/net/usb/smsc95xx.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index edf0492ad489..2cb44d65bbc3 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -572,10 +572,6 @@ static int smsc95xx_link_reset(struct usbnet *dev)
 	unsigned long flags;
 	int ret;
 
-	ret = smsc95xx_write_reg(dev, INT_STS, INT_STS_CLEAR_ALL_);
-	if (ret < 0)
-		return ret;
-
 	spin_lock_irqsave(&pdata->mac_cr_lock, flags);
 	if (pdata->phydev->duplex != DUPLEX_FULL) {
 		pdata->mac_cr &= ~MAC_CR_FDPX_;
-- 
2.35.2

[PATCH net-next 3/7] usbnet: smsc95xx: Don't reset PHY behind PHY driver's back

[Lukas Wunner]

smsc95xx_reset() resets the PHY behind the PHY driver's back, which
seems like a bad idea generally.  Remove that portion of the function.

We're about to use PHY interrupts instead of polling to detect link
changes on SMSC LAN95xx chips.  Because smsc95xx_reset() is called from
usbnet_open(), PHY interrupt settings are lost whenever the net_device
is brought up.

There are two other callers of smsc95xx_reset(), namely smsc95xx_bind()
and smsc95xx_reset_resume(), and both may indeed benefit from a PHY
reset.  However they already perform one through their calls to
phy_connect_direct() and phy_init_hw().

Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Martyn Welch <martyn.welch@collabora.com>
Cc: Gabriel Hojda <ghojda@yo2urs.ro>
---
 drivers/net/usb/smsc95xx.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 2cb44d65bbc3..6c37c7adde1b 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -887,24 +887,6 @@ static int smsc95xx_reset(struct usbnet *dev)
 		return ret;
 	}
 
-	ret = smsc95xx_write_reg(dev, PM_CTRL, PM_CTL_PHY_RST_);
-	if (ret < 0)
-		return ret;
-
-	timeout = 0;
-	do {
-		msleep(10);
-		ret = smsc95xx_read_reg(dev, PM_CTRL, &read_buf);
-		if (ret < 0)
-			return ret;
-		timeout++;
-	} while ((read_buf & PM_CTL_PHY_RST_) && (timeout < 100));
-
-	if (timeout >= 100) {
-		netdev_warn(dev->net, "timeout waiting for PHY Reset\n");
-		return ret;
-	}
-
 	ret = smsc95xx_set_mac_address(dev);
 	if (ret < 0)
 		return ret;
-- 
2.35.2

[PATCH net-next 4/7] usbnet: smsc95xx: Avoid link settings race on interrupt reception

[Lukas Wunner]

When a PHY interrupt is signaled, the SMSC LAN95xx driver updates the
MAC full duplex mode and PHY flow control registers based on cached data
in struct phy_device:

  smsc95xx_status()                 # raises EVENT_LINK_RESET
    usbnet_deferred_kevent()
      smsc95xx_link_reset()         # uses cached data in phydev

Simultaneously, phylib polls link status once per second and updates
that cached data:

  phy_state_machine()
    phy_check_link_status()
      phy_read_status()
        lan87xx_read_status()
          genphy_read_status()      # updates cached data in phydev

If smsc95xx_link_reset() wins the race against genphy_read_status(),
the registers may be updated based on stale data.

E.g. if the link was previously down, phydev->duplex is set to
DUPLEX_UNKNOWN and that's what smsc95xx_link_reset() will use, even
though genphy_read_status() may update it to DUPLEX_FULL afterwards.

PHY interrupts are currently only enabled on suspend to trigger wakeup,
so the impact of the race is limited, but we're about to enable them
perpetually.

Avoid the race by delaying execution of smsc95xx_link_reset() until
phy_state_machine() has done its job and calls back via
smsc95xx_handle_link_change().

Signaling EVENT_LINK_RESET on wakeup is not necessary because phylib
picks up link status changes through polling.  So drop the declaration
of a ->link_reset() callback.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/net/usb/smsc95xx.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 6c37c7adde1b..36abfaeae3c6 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -566,7 +566,7 @@ static int smsc95xx_phy_update_flowcontrol(struct usbnet *dev)
 	return smsc95xx_write_reg(dev, AFC_CFG, afc_cfg);
 }
 
-static int smsc95xx_link_reset(struct usbnet *dev)
+static void smsc95xx_mac_update_fullduplex(struct usbnet *dev)
 {
 	struct smsc95xx_priv *pdata = dev->driver_priv;
 	unsigned long flags;
@@ -583,14 +583,14 @@ static int smsc95xx_link_reset(struct usbnet *dev)
 	spin_unlock_irqrestore(&pdata->mac_cr_lock, flags);
 
 	ret = smsc95xx_write_reg(dev, MAC_CR, pdata->mac_cr);
-	if (ret < 0)
-		return ret;
+	if (ret < 0) {
+		netdev_warn(dev->net, "Error updating MAC full duplex mode\n");
+		return;
+	}
 
 	ret = smsc95xx_phy_update_flowcontrol(dev);
 	if (ret < 0)
 		netdev_warn(dev->net, "Error updating PHY flow control\n");
-
-	return ret;
 }
 
 static void smsc95xx_status(struct usbnet *dev, struct urb *urb)
@@ -607,7 +607,7 @@ static void smsc95xx_status(struct usbnet *dev, struct urb *urb)
 	netif_dbg(dev, link, dev->net, "intdata: 0x%08X\n", intdata);
 
 	if (intdata & INT_ENP_PHY_INT_)
-		usbnet_defer_kevent(dev, EVENT_LINK_RESET);
+		;
 	else
 		netdev_warn(dev->net, "unexpected interrupt, intdata=0x%08X\n",
 			    intdata);
@@ -1070,6 +1070,7 @@ static void smsc95xx_handle_link_change(struct net_device *net)
 	struct usbnet *dev = netdev_priv(net);
 
 	phy_print_status(net->phydev);
+	smsc95xx_mac_update_fullduplex(dev);
 	usbnet_defer_kevent(dev, EVENT_LINK_CHANGE);
 }
 
@@ -1975,7 +1976,6 @@ static const struct driver_info smsc95xx_info = {
 	.description	= "smsc95xx USB 2.0 Ethernet",
 	.bind		= smsc95xx_bind,
 	.unbind		= smsc95xx_unbind,
-	.link_reset	= smsc95xx_link_reset,
 	.reset		= smsc95xx_reset,
 	.check_connect	= smsc95xx_start_phy,
 	.stop		= smsc95xx_stop,
-- 
2.35.2

Re: [PATCH net-next 4/7] usbnet: smsc95xx: Avoid link settings race on interrupt reception

[Andrew Lunn]

>  static void smsc95xx_status(struct usbnet *dev, struct urb *urb)
> @@ -607,7 +607,7 @@ static void smsc95xx_status(struct usbnet *dev, struct urb *urb)
>  	netif_dbg(dev, link, dev->net, "intdata: 0x%08X\n", intdata);
>  
>  	if (intdata & INT_ENP_PHY_INT_)
> -		usbnet_defer_kevent(dev, EVENT_LINK_RESET);
> +		;
>  	else
>  		netdev_warn(dev->net, "unexpected interrupt, intdata=0x%08X\n",
>  			    intdata);

Hi Lukas

It would probably make sense to invert the condition and remove the ;

   Andrew

[PATCH net-next 5/7] usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid polling

[Lukas Wunner]

Link status of SMSC LAN95xx chips is polled once per second, even though
they're capable of signaling PHY interrupts through the MAC layer.

Forward those interrupts to the PHY driver to avoid polling.  Benefits
are reduced bus traffic, reduced CPU overhead and quicker interface
bringup.

Polling was introduced in 2016 by commit d69d16949346 ("usbnet:
smsc95xx: fix link detection for disabled autonegotiation").
Back then, the LAN95xx driver neglected to enable the ENERGYON interrupt,
hence couldn't detect link-up events when auto-negotiation was disabled.
The proper solution would have been to enable the ENERGYON interrupt
instead of polling.

Since then, PHY handling was moved from the LAN95xx driver to the SMSC
PHY driver with commit 05b35e7eb9a1 ("smsc95xx: add phylib support").
That PHY driver is capable of link detection with auto-negotiation
disabled because it enables the ENERGYON interrupt.

Note that signaling interrupts through the MAC layer not only works with
the integrated PHY, but also with an external PHY, provided its
interrupt pin is attached to LAN95xx's nPHY_INT pin.

In the unlikely event that the interrupt pin of an external PHY is
attached to a GPIO of the SoC (or not connected at all), the driver can
be amended to retrieve the irq from the PHY's of_node.

To forward PHY interrupts to phylib, it is not sufficient to call
phy_mac_interrupt().  Instead, the PHY's interrupt handler needs to run
so that PHY interrupts are cleared.  That's because according to page
119 of the LAN950x datasheet, "The source of this interrupt is a level.
The interrupt persists until it is cleared in the PHY."

https://www.microchip.com/content/dam/mchp/documents/UNG/ProductDocuments/DataSheets/LAN950x-Data-Sheet-DS00001875D.pdf

Therefore, create an IRQ domain with a single IRQ for the PHY.  In the
future, the IRQ domain may be extended to support the 11 GPIOs on the
LAN95xx.

Normally the PHY interrupt should be masked until the PHY driver has
cleared it.  However masking requires a (sleeping) USB transaction and
interrupts are received in (non-sleepable) softirq context.  I decided
not to mask the interrupt at all (by using the dummy_irq_chip's noop
->irq_mask() callback):  The USB interrupt endpoint is polled in 1 msec
intervals and normally that's sufficient to wake the PHY driver's IRQ
thread and have it clear the interrupt.  If it does take longer, worst
thing that can happen is the IRQ thread is woken again.  No big deal.

Because PHY interrupts are now perpetually enabled, there's no need to
selectively enable them on suspend.  So remove all invocations of
smsc95xx_enable_phy_wakeup_interrupts().

Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Andre Edich <andre.edich@microchip.com>
---
 drivers/net/usb/smsc95xx.c | 118 +++++++++++++++++++++----------------
 1 file changed, 66 insertions(+), 52 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 36abfaeae3c6..1d376bad13e5 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -18,6 +18,8 @@
 #include <linux/usb/usbnet.h>
 #include <linux/slab.h>
 #include <linux/of_net.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
 #include <linux/mdio.h>
 #include <linux/phy.h>
 #include <net/selftests.h>
@@ -53,6 +55,9 @@
 #define SUSPEND_ALLMODES		(SUSPEND_SUSPEND0 | SUSPEND_SUSPEND1 | \
 					 SUSPEND_SUSPEND2 | SUSPEND_SUSPEND3)
 
+#define SMSC95XX_NR_IRQS		(1) /* raise to 12 for GPIOs */
+#define PHY_HWIRQ 			(SMSC95XX_NR_IRQS - 1)
+
 struct smsc95xx_priv {
 	u32 mac_cr;
 	u32 hash_hi;
@@ -61,6 +66,9 @@ struct smsc95xx_priv {
 	spinlock_t mac_cr_lock;
 	u8 features;
 	u8 suspend_flags;
+	struct irq_chip irqchip;
+	struct irq_domain *irqdomain;
+	struct fwnode_handle *irqfwnode;
 	struct mii_bus *mdiobus;
 	struct phy_device *phydev;
 };
@@ -595,6 +603,8 @@ static void smsc95xx_mac_update_fullduplex(struct usbnet *dev)
 
 static void smsc95xx_status(struct usbnet *dev, struct urb *urb)
 {
+	struct smsc95xx_priv *pdata = dev->driver_priv;
+	unsigned long flags;
 	u32 intdata;
 
 	if (urb->actual_length != 4) {
@@ -606,11 +616,20 @@ static void smsc95xx_status(struct usbnet *dev, struct urb *urb)
 	intdata = get_unaligned_le32(urb->transfer_buffer);
 	netif_dbg(dev, link, dev->net, "intdata: 0x%08X\n", intdata);
 
+	/* USB interrupts are received in softirq (tasklet) context.
+	 * Switch to hardirq context to make genirq code happy.
+	 */
+	local_irq_save(flags);
+	__irq_enter_raw();
+
 	if (intdata & INT_ENP_PHY_INT_)
-		;
+		generic_handle_domain_irq(pdata->irqdomain, PHY_HWIRQ);
 	else
 		netdev_warn(dev->net, "unexpected interrupt, intdata=0x%08X\n",
 			    intdata);
+
+	__irq_exit_raw();
+	local_irq_restore(flags);
 }
 
 /* Enable or disable Tx & Rx checksum offload engines */
@@ -1078,8 +1097,9 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
 {
 	struct smsc95xx_priv *pdata;
 	bool is_internal_phy;
+	char usb_path[64];
+	int ret, phy_irq;
 	u32 val;
-	int ret;
 
 	printk(KERN_INFO SMSC_CHIPNAME " v" SMSC_DRIVER_VERSION "\n");
 
@@ -1119,10 +1139,38 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
 	if (ret)
 		goto free_pdata;
 
+	/* create irq domain for use by PHY driver and GPIO consumers */
+	usb_make_path(dev->udev, usb_path, sizeof(usb_path));
+	pdata->irqfwnode = irq_domain_alloc_named_fwnode(usb_path);
+	if (!pdata->irqfwnode) {
+		ret = -ENOMEM;
+		goto free_pdata;
+	}
+
+	pdata->irqdomain = irq_domain_create_linear(pdata->irqfwnode,
+						    SMSC95XX_NR_IRQS,
+						    &irq_domain_simple_ops,
+						    pdata);
+	if (!pdata->irqdomain) {
+		ret = -ENOMEM;
+		goto free_irqfwnode;
+	}
+
+	phy_irq = irq_create_mapping(pdata->irqdomain, PHY_HWIRQ);
+	if (!phy_irq) {
+		ret = -ENOENT;
+		goto remove_irqdomain;
+	}
+
+	pdata->irqchip = dummy_irq_chip;
+	pdata->irqchip.name = SMSC_CHIPNAME;
+	irq_set_chip_and_handler_name(phy_irq, &pdata->irqchip,
+				      handle_simple_irq, "phy");
+
 	pdata->mdiobus = mdiobus_alloc();
 	if (!pdata->mdiobus) {
 		ret = -ENOMEM;
-		goto free_pdata;
+		goto dispose_irq;
 	}
 
 	ret = smsc95xx_read_reg(dev, HW_CFG, &val);
@@ -1155,6 +1203,7 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
 		goto unregister_mdio;
 	}
 
+	pdata->phydev->irq = phy_irq;
 	pdata->phydev->is_internal = is_internal_phy;
 
 	/* detect device revision as different features may be available */
@@ -1197,6 +1246,15 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
 free_mdio:
 	mdiobus_free(pdata->mdiobus);
 
+dispose_irq:
+	irq_dispose_mapping(phy_irq);
+
+remove_irqdomain:
+	irq_domain_remove(pdata->irqdomain);
+
+free_irqfwnode:
+	irq_domain_free_fwnode(pdata->irqfwnode);
+
 free_pdata:
 	kfree(pdata);
 	return ret;
@@ -1209,6 +1267,9 @@ static void smsc95xx_unbind(struct usbnet *dev, struct usb_interface *intf)
 	phy_disconnect(dev->net->phydev);
 	mdiobus_unregister(pdata->mdiobus);
 	mdiobus_free(pdata->mdiobus);
+	irq_dispose_mapping(irq_find_mapping(pdata->irqdomain, PHY_HWIRQ));
+	irq_domain_remove(pdata->irqdomain);
+	irq_domain_free_fwnode(pdata->irqfwnode);
 	netif_dbg(dev, ifdown, dev->net, "free pdata\n");
 	kfree(pdata);
 }
@@ -1233,29 +1294,6 @@ static u32 smsc_crc(const u8 *buffer, size_t len, int filter)
 	return crc << ((filter % 2) * 16);
 }
 
-static int smsc95xx_enable_phy_wakeup_interrupts(struct usbnet *dev, u16 mask)
-{
-	int ret;
-
-	netdev_dbg(dev->net, "enabling PHY wakeup interrupts\n");
-
-	/* read to clear */
-	ret = smsc95xx_mdio_read_nopm(dev, PHY_INT_SRC);
-	if (ret < 0)
-		return ret;
-
-	/* enable interrupt source */
-	ret = smsc95xx_mdio_read_nopm(dev, PHY_INT_MASK);
-	if (ret < 0)
-		return ret;
-
-	ret |= mask;
-
-	smsc95xx_mdio_write_nopm(dev, PHY_INT_MASK, ret);
-
-	return 0;
-}
-
 static int smsc95xx_link_ok_nopm(struct usbnet *dev)
 {
 	int ret;
@@ -1422,7 +1460,6 @@ static int smsc95xx_enter_suspend3(struct usbnet *dev)
 static int smsc95xx_autosuspend(struct usbnet *dev, u32 link_up)
 {
 	struct smsc95xx_priv *pdata = dev->driver_priv;
-	int ret;
 
 	if (!netif_running(dev->net)) {
 		/* interface is ifconfig down so fully power down hw */
@@ -1441,27 +1478,10 @@ static int smsc95xx_autosuspend(struct usbnet *dev, u32 link_up)
 		}
 
 		netdev_dbg(dev->net, "autosuspend entering SUSPEND1\n");
-
-		/* enable PHY wakeup events for if cable is attached */
-		ret = smsc95xx_enable_phy_wakeup_interrupts(dev,
-			PHY_INT_MASK_ANEG_COMP_);
-		if (ret < 0) {
-			netdev_warn(dev->net, "error enabling PHY wakeup ints\n");
-			return ret;
-		}
-
 		netdev_info(dev->net, "entering SUSPEND1 mode\n");
 		return smsc95xx_enter_suspend1(dev);
 	}
 
-	/* enable PHY wakeup events so we remote wakeup if cable is pulled */
-	ret = smsc95xx_enable_phy_wakeup_interrupts(dev,
-		PHY_INT_MASK_LINK_DOWN_);
-	if (ret < 0) {
-		netdev_warn(dev->net, "error enabling PHY wakeup ints\n");
-		return ret;
-	}
-
 	netdev_dbg(dev->net, "autosuspend entering SUSPEND3\n");
 	return smsc95xx_enter_suspend3(dev);
 }
@@ -1527,13 +1547,6 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
 	}
 
 	if (pdata->wolopts & WAKE_PHY) {
-		ret = smsc95xx_enable_phy_wakeup_interrupts(dev,
-			(PHY_INT_MASK_ANEG_COMP_ | PHY_INT_MASK_LINK_DOWN_));
-		if (ret < 0) {
-			netdev_warn(dev->net, "error enabling PHY wakeup ints\n");
-			goto done;
-		}
-
 		/* if link is down then configure EDPD and enter SUSPEND1,
 		 * otherwise enter SUSPEND0 below
 		 */
@@ -1767,11 +1780,12 @@ static int smsc95xx_resume(struct usb_interface *intf)
 			return ret;
 	}
 
+	phy_init_hw(pdata->phydev);
+
 	ret = usbnet_resume(intf);
 	if (ret < 0)
 		netdev_warn(dev->net, "usbnet_resume error\n");
 
-	phy_init_hw(pdata->phydev);
 	return ret;
 }
 
-- 
2.35.2

Re: [PATCH net-next 5/7] usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid polling

[Andrew Lunn]

> @@ -606,11 +616,20 @@ static void smsc95xx_status(struct usbnet *dev, struct urb *urb)
>  	intdata = get_unaligned_le32(urb->transfer_buffer);
>  	netif_dbg(dev, link, dev->net, "intdata: 0x%08X\n", intdata);
>  
> +	/* USB interrupts are received in softirq (tasklet) context.
> +	 * Switch to hardirq context to make genirq code happy.
> +	 */
> +	local_irq_save(flags);
> +	__irq_enter_raw();
> +
>  	if (intdata & INT_ENP_PHY_INT_)
> -		;
> +		generic_handle_domain_irq(pdata->irqdomain, PHY_HWIRQ);
>  	else
>  		netdev_warn(dev->net, "unexpected interrupt, intdata=0x%08X\n",
>  			    intdata);

Ah, O.K, forget my previous comment. Maybe add something to the commit
message that the ; will soon be replaced by a call to actually handle
the interrupt.

	Andrew

Re: [PATCH net-next 5/7] usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid polling

[Andrew Lunn]

On Wed, Apr 27, 2022 at 07:48:05AM +0200, Lukas Wunner wrote:
> Link status of SMSC LAN95xx chips is polled once per second, even though
> they're capable of signaling PHY interrupts through the MAC layer.
> 
> Forward those interrupts to the PHY driver to avoid polling.  Benefits
> are reduced bus traffic, reduced CPU overhead and quicker interface
> bringup.
> 
> Polling was introduced in 2016 by commit d69d16949346 ("usbnet:
> smsc95xx: fix link detection for disabled autonegotiation").
> Back then, the LAN95xx driver neglected to enable the ENERGYON interrupt,
> hence couldn't detect link-up events when auto-negotiation was disabled.
> The proper solution would have been to enable the ENERGYON interrupt
> instead of polling.
> 
> Since then, PHY handling was moved from the LAN95xx driver to the SMSC
> PHY driver with commit 05b35e7eb9a1 ("smsc95xx: add phylib support").
> That PHY driver is capable of link detection with auto-negotiation
> disabled because it enables the ENERGYON interrupt.
> 
> Note that signaling interrupts through the MAC layer not only works with
> the integrated PHY, but also with an external PHY, provided its
> interrupt pin is attached to LAN95xx's nPHY_INT pin.
> 
> In the unlikely event that the interrupt pin of an external PHY is
> attached to a GPIO of the SoC (or not connected at all), the driver can
> be amended to retrieve the irq from the PHY's of_node.
> 
> To forward PHY interrupts to phylib, it is not sufficient to call
> phy_mac_interrupt().  Instead, the PHY's interrupt handler needs to run
> so that PHY interrupts are cleared.  That's because according to page
> 119 of the LAN950x datasheet, "The source of this interrupt is a level.
> The interrupt persists until it is cleared in the PHY."
> 
> https://www.microchip.com/content/dam/mchp/documents/UNG/ProductDocuments/DataSheets/LAN950x-Data-Sheet-DS00001875D.pdf
> 
> Therefore, create an IRQ domain with a single IRQ for the PHY.  In the
> future, the IRQ domain may be extended to support the 11 GPIOs on the
> LAN95xx.
> 
> Normally the PHY interrupt should be masked until the PHY driver has
> cleared it.  However masking requires a (sleeping) USB transaction and
> interrupts are received in (non-sleepable) softirq context.  I decided
> not to mask the interrupt at all (by using the dummy_irq_chip's noop
> ->irq_mask() callback):  The USB interrupt endpoint is polled in 1 msec
> intervals and normally that's sufficient to wake the PHY driver's IRQ
> thread and have it clear the interrupt.  If it does take longer, worst
> thing that can happen is the IRQ thread is woken again.  No big deal.
> 
> Because PHY interrupts are now perpetually enabled, there's no need to
> selectively enable them on suspend.  So remove all invocations of
> smsc95xx_enable_phy_wakeup_interrupts().
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Andre Edich <andre.edich@microchip.com>

This looks reasonable from a PHY perspective. I cannot say much about
USB though.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 6/7] net: phy: smsc: Cache interrupt mask

[Lukas Wunner]

Cache the interrupt mask to avoid re-reading it from the PHY upon every
interrupt.  The PHY may be located on a USB device, so the additional
read may unnecessarily increase interrupt overhead and latency.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/net/phy/smsc.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index d8cac02a79b9..a521d48b22a7 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -44,6 +44,7 @@ static struct smsc_hw_stat smsc_hw_stats[] = {
 };
 
 struct smsc_phy_priv {
+	u16 intmask;
 	bool energy_enable;
 	struct clk *refclk;
 };
@@ -58,7 +59,6 @@ static int smsc_phy_ack_interrupt(struct phy_device *phydev)
 static int smsc_phy_config_intr(struct phy_device *phydev)
 {
 	struct smsc_phy_priv *priv = phydev->priv;
-	u16 intmask = 0;
 	int rc;
 
 	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
@@ -66,12 +66,15 @@ static int smsc_phy_config_intr(struct phy_device *phydev)
 		if (rc)
 			return rc;
 
-		intmask = MII_LAN83C185_ISF_INT4 | MII_LAN83C185_ISF_INT6;
+		priv->intmask = MII_LAN83C185_ISF_INT4 | MII_LAN83C185_ISF_INT6;
 		if (priv->energy_enable)
-			intmask |= MII_LAN83C185_ISF_INT7;
-		rc = phy_write(phydev, MII_LAN83C185_IM, intmask);
+			priv->intmask |= MII_LAN83C185_ISF_INT7;
+
+		rc = phy_write(phydev, MII_LAN83C185_IM, priv->intmask);
 	} else {
-		rc = phy_write(phydev, MII_LAN83C185_IM, intmask);
+		priv->intmask = 0;
+
+		rc = phy_write(phydev, MII_LAN83C185_IM, 0);
 		if (rc)
 			return rc;
 
@@ -83,13 +86,8 @@ static int smsc_phy_config_intr(struct phy_device *phydev)
 
 static irqreturn_t smsc_phy_handle_interrupt(struct phy_device *phydev)
 {
-	int irq_status, irq_enabled;
-
-	irq_enabled = phy_read(phydev, MII_LAN83C185_IM);
-	if (irq_enabled < 0) {
-		phy_error(phydev);
-		return IRQ_NONE;
-	}
+	struct smsc_phy_priv *priv = phydev->priv;
+	int irq_status;
 
 	irq_status = phy_read(phydev, MII_LAN83C185_ISF);
 	if (irq_status < 0) {
@@ -97,7 +95,7 @@ static irqreturn_t smsc_phy_handle_interrupt(struct phy_device *phydev)
 		return IRQ_NONE;
 	}
 
-	if (!(irq_status & irq_enabled))
+	if (!(irq_status & priv->intmask))
 		return IRQ_NONE;
 
 	phy_trigger_machine(phydev);
-- 
2.35.2

Re: [PATCH net-next 6/7] net: phy: smsc: Cache interrupt mask

[Andrew Lunn]

On Wed, Apr 27, 2022 at 07:48:06AM +0200, Lukas Wunner wrote:
> Cache the interrupt mask to avoid re-reading it from the PHY upon every
> interrupt.  The PHY may be located on a USB device, so the additional
> read may unnecessarily increase interrupt overhead and latency.

I don't think your justification is valid. The MDIO bus is clocked at
2.5MHz. So even if you are using USB 1.1 at 12MHz, the USB overheads
are not particularly large. At 480Mbps they are pretty insignificant.

In general, we consider PHYs as slow devices, they take over 1 second
to negotiate a link and declare it up. So we don't do this sort of
micro optimization.

What i think is relevant here is that you could have an interrupt
storm going on because you don't mask interrupts? It is not a true
storm, due to the way USB works, more of a light shower. Do you have
any statistics to show this code actually reduces the amount of rain
in a significant way?

	Andrew

Re: [PATCH net-next 6/7] net: phy: smsc: Cache interrupt mask

[Lukas Wunner]

On Wed, Apr 27, 2022 at 02:14:22PM +0200, Andrew Lunn wrote:
> On Wed, Apr 27, 2022 at 07:48:06AM +0200, Lukas Wunner wrote:
> > Cache the interrupt mask to avoid re-reading it from the PHY upon every
> > interrupt.  The PHY may be located on a USB device, so the additional
> > read may unnecessarily increase interrupt overhead and latency.
> 
> I don't think your justification is valid. The MDIO bus is clocked at
> 2.5MHz. So even if you are using USB 1.1 at 12MHz, the USB overheads
> are not particularly large. At 480Mbps they are pretty insignificant.
> 
> In general, we consider PHYs as slow devices, they take over 1 second
> to negotiate a link and declare it up. So we don't do this sort of
> micro optimization.
> 
> What i think is relevant here is that you could have an interrupt
> storm going on because you don't mask interrupts? It is not a true
> storm, due to the way USB works, more of a light shower. Do you have
> any statistics to show this code actually reduces the amount of rain
> in a significant way?

TBH the primary motivation for this change is that it simplifies the
succeeding commit ("Cope with hot-removal in interrupt handler").

Additionally it seemed silly to me to re-read the interrupt mask
every time for no reason at all.  To test and debug this series
I logged every MDIO read/write and these nonsensical transactions
are very visible and very annoying in the log output.

So yeah, maybe the latency argument isn't very strong, but there
are other arguments which I didn't deem necessary mentioning in
the commit message as they seemed somewhat egotistical. :)

Thanks,

Lukas

[PATCH net-next 7/7] net: phy: smsc: Cope with hot-removal in interrupt handler

[Lukas Wunner]

If reading the Interrupt Source Flag register fails with -ENODEV, then
the PHY has been hot-removed and the correct response is to bail out
instead of throwing a WARN splat and attempting to suspend the PHY.
The PHY should be stopped in due course anyway as the kernel
asynchronously tears down the device.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/net/phy/smsc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index a521d48b22a7..35bff7fd234c 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -91,7 +91,9 @@ static irqreturn_t smsc_phy_handle_interrupt(struct phy_device *phydev)
 
 	irq_status = phy_read(phydev, MII_LAN83C185_ISF);
 	if (irq_status < 0) {
-		phy_error(phydev);
+		if (irq_status != -ENODEV)
+			phy_error(phydev);
+
 		return IRQ_NONE;
 	}
 
-- 
2.35.2

Re: [PATCH net-next 7/7] net: phy: smsc: Cope with hot-removal in interrupt handler

[Oliver Neukum]

On 27.04.22 07:48, Lukas Wunner wrote:
> If reading the Interrupt Source Flag register fails with -ENODEV, then
> the PHY has been hot-removed and the correct response is to bail out
> instead of throwing a WARN splat and attempting to suspend the PHY.
> The PHY should be stopped in due course anyway as the kernel
> asynchronously tears down the device.
>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/net/phy/smsc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> index a521d48b22a7..35bff7fd234c 100644
> --- a/drivers/net/phy/smsc.c
> +++ b/drivers/net/phy/smsc.c
> @@ -91,7 +91,9 @@ static irqreturn_t smsc_phy_handle_interrupt(struct phy_device *phydev)
>  
>  	irq_status = phy_read(phydev, MII_LAN83C185_ISF);
>  	if (irq_status < 0) {
> -		phy_error(phydev);
> +		if (irq_status != -ENODEV)
> +			phy_error(phydev);
> +
>  		return IRQ_NONE;
>  	}
>  
Hi,

picking a small nit, if you get ENODEV, you have no idea whether the irq
was caused
by the phy. Strictly speaking you should return IRQ_HANDLED in that case.

    Regards
        Oliver

Re: [PATCH net-next 0/7] Polling be gone on LAN95xx

[Oleksij Rempel]

On Wed, Apr 27, 2022 at 07:48:00AM +0200, Lukas Wunner wrote:
> Do away with link status polling on LAN95XX USB Ethernet
> and rely on interrupts instead, thereby reducing bus traffic,
> CPU overhead and improving interface bringup latency.
> 
> The meat of the series is in patch [5/7].  The preceding and
> following patches are various cleanups to prepare for and
> adjust to interrupt-driven link state detection.
> 
> Please review and test.  Thanks!

Tested-by: Oleksij Rempel <o.rempel@pengutronix.de>

Tested on:
- LAN9514 (RPi2)
- LAN9512 (EVB)
- LAN9500 (EVB)

On USB unplug i get some not usable kernel message, but it is not the
show stopper for me:
smsc95xx 1-1.4.1:1.0 eth1: Error updating MAC full duplex mode
smsc95xx 1-1.4.1:1.0 eth1: hardware isn't capable of remote wakeup

Thank you!

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next 0/7] Polling be gone on LAN95xx

[Lukas Wunner]

On Wed, Apr 27, 2022 at 02:37:15PM +0200, Oleksij Rempel wrote:
> On Wed, Apr 27, 2022 at 07:48:00AM +0200, Lukas Wunner wrote:
> > Do away with link status polling on LAN95XX USB Ethernet
> > and rely on interrupts instead, thereby reducing bus traffic,
> > CPU overhead and improving interface bringup latency.
> > 
> > The meat of the series is in patch [5/7].  The preceding and
> > following patches are various cleanups to prepare for and
> > adjust to interrupt-driven link state detection.
> 
> Tested-by: Oleksij Rempel <o.rempel@pengutronix.de>
> 
> Tested on:
> - LAN9514 (RPi2)
> - LAN9512 (EVB)
> - LAN9500 (EVB)

Thanks a lot for testing, this helps greatly.

> On USB unplug i get some not usable kernel message, but it is not the
> show stopper for me:
> smsc95xx 1-1.4.1:1.0 eth1: Error updating MAC full duplex mode
> smsc95xx 1-1.4.1:1.0 eth1: hardware isn't capable of remote wakeup

Okay, I've amended patch [4/7] to silence the first of these messages
if the error is caused by hot-removal.

The second message isn't related to this series, I'll address that
separately at a later point in time.

Thanks,

Lukas

Re: [PATCH net-next 0/7] Polling be gone on LAN95xx

[Ferry Toth]

Hi,
Op 27-04-2022 om 07:48 schreef Lukas Wunner:
> Do away with link status polling on LAN95XX USB Ethernet
> and rely on interrupts instead, thereby reducing bus traffic,
> CPU overhead and improving interface bringup latency.
> 
> The meat of the series is in patch [5/7].  The preceding and
> following patches are various cleanups to prepare for and
> adjust to interrupt-driven link state detection.
> 
> Please review and test.  Thanks!
> 
> Lukas Wunner (7):
>    usbnet: Run unregister_netdev() before unbind() again
>    usbnet: smsc95xx: Don't clear read-only PHY interrupt
>    usbnet: smsc95xx: Don't reset PHY behind PHY driver's back
>    usbnet: smsc95xx: Avoid link settings race on interrupt reception
>    usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid
>      polling
>    net: phy: smsc: Cache interrupt mask
>    net: phy: smsc: Cope with hot-removal in interrupt handler
> 
>   drivers/net/phy/smsc.c         |  28 +++---
>   drivers/net/usb/asix_devices.c |   6 +-
>   drivers/net/usb/smsc95xx.c     | 155 ++++++++++++++++-----------------
>   drivers/net/usb/usbnet.c       |   6 +-
>   4 files changed, 91 insertions(+), 104 deletions(-)
> 

Tested-by: Ferry Toth <fntoth@gmail.com> (Intel Edison-Arduino)

(linux v5.17 + the series + usbnet: smsc95xx: Fix deadlock on runtime 
resume)

While testing I noted another problem. I have "DMA-API: debugging 
enabled by kernel config" and this (I guess) shows me before and after 
the patches:

------------[ cut here ]------------
DMA-API: xhci-hcd xhci-hcd.1.auto: cacheline tracking EEXIST, 
overlapping mappings aren't supported
WARNING: CPU: 0 PID: 24 at kernel/dma/debug.c:570 add_dma_entry+0x1d9/0x270
Modules linked in: mmc_block extcon_intel_mrfld sdhci_pci cqhci sdhci 
led_class mmc_core intel_soc_pmic_mrfld btrfs libcrc32c xor zlib_deflate 
raid6_pq zstd_c>
CPU: 0 PID: 24 Comm: kworker/0:1 Not tainted 5.17.0-edison-acpi-standard #1
Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS 542 
2015.01.21:18.19.48
Workqueue: usb_hub_wq hub_event
RIP: 0010:add_dma_entry+0x1d9/0x270
Code: ff 0f 84 97 00 00 00 4c 8b 67 50 4d 85 e4 75 03 4c 8b 27 e8 29 a4 
52 00 48 89 c6 4c 89 e2 48 c7 c7 78 9d 7e 98 e8 b3 f7 b6 00 <0f> 0b 48 
85 ed 0f 85 d3 >
RSP: 0018:ffffb2ce000d7ad0 EFLAGS: 00010286
RAX: 0000000000000000 RBX: 00000000ffffffff RCX: 0000000000000000
RDX: 0000000000000001 RSI: 00000000ffffffea RDI: 00000000ffffffff
RBP: ffff9dea81239a00 R08: ffffffff98b356c8 R09: 00000000ffffdfff
R10: ffffffff98a556e0 R11: ffffffff98a556e0 R12: ffff9dea87397180
R13: 0000000000000001 R14: 0000000000000206 R15: 0000000000046c59
FS:  0000000000000000(0000) GS:ffff9deabe200000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00005595807522e0 CR3: 0000000006be2000 CR4: 00000000001006f0
Call Trace:
  <TASK>
  dma_map_page_attrs+0xfc/0x240
  ? usb_hcd_link_urb_to_ep+0x6e/0xa0
  ? preempt_count_add+0x63/0x90
  usb_hcd_map_urb_for_dma+0x3f3/0x580
  usb_hcd_submit_urb+0x93/0xb90
  ? _raw_spin_lock+0xe/0x30
  ? _raw_spin_unlock+0xd/0x20
  ? usb_hcd_link_urb_to_ep+0x6e/0xa0
  ? prepare_transfer+0xff/0x140
  usb_start_wait_urb+0x60/0x160
  usb_control_msg+0xdc/0x140
  hub_ext_port_status+0x82/0x100
  hub_event+0x1b2/0x1880
  ? hub_activate+0x59c/0x880
  process_one_work+0x1d3/0x3a0
  worker_thread+0x48/0x3c0
  ? rescuer_thread+0x380/0x380
  kthread+0xe2/0x110
  ? kthread_complete_and_exit+0x20/0x20
  ret_from_fork+0x22/0x30
  </TASK>
---[ end trace 0000000000000000 ]---
DMA-API: Mapped at:
  debug_dma_map_page+0x60/0xf0
  dma_map_page_attrs+0xfc/0x240
  usb_hcd_map_urb_for_dma+0x3f3/0x580
  usb_hcd_submit_urb+0x93/0xb90
  usb_start_wait_urb+0x60/0x160
usb 1-1.1: new high-speed USB device number 3 using xhci-hcd
usb 1-1.1: New USB device found, idVendor=0424, idProduct=ec00, 
bcdDevice= 2.00
usb 1-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
usb 1-1.1: Product: LAN9514
usb 1-1.1: Manufacturer: SMSC
usb 1-1.1: SerialNumber: 00951d0d

Any ideas on this?

Re: [PATCH net-next 0/7] Polling be gone on LAN95xx

[Lukas Wunner]

On Mon, May 02, 2022 at 10:33:06PM +0200, Ferry Toth wrote:
> Op 27-04-2022 om 07:48 schreef Lukas Wunner:
> > Do away with link status polling on LAN95XX USB Ethernet
> > and rely on interrupts instead, thereby reducing bus traffic,
> > CPU overhead and improving interface bringup latency.
> 
> Tested-by: Ferry Toth <fntoth@gmail.com> (Intel Edison-Arduino)

Thank you!

> While testing I noted another problem. I have "DMA-API: debugging enabled by
> kernel config" and this (I guess) shows me before and after the patches:
> 
> ------------[ cut here ]------------
> DMA-API: xhci-hcd xhci-hcd.1.auto: cacheline tracking EEXIST, overlapping
> mappings aren't supported

That is under investigation here:
https://bugzilla.kernel.org/show_bug.cgi?id=215740

It's apparently a long-standing bug in the USB core which was exposed
by a new WARN() check introduced in 5.14.

Thanks,

Lukas

Re: [PATCH net-next 0/7] Polling be gone on LAN95xx

[Ferry Toth]

Hi

On 03-05-2022 10:26, Lukas Wunner wrote:
> On Mon, May 02, 2022 at 10:33:06PM +0200, Ferry Toth wrote:
>> Op 27-04-2022 om 07:48 schreef Lukas Wunner:
>>> Do away with link status polling on LAN95XX USB Ethernet
>>> and rely on interrupts instead, thereby reducing bus traffic,
>>> CPU overhead and improving interface bringup latency.
>> Tested-by: Ferry Toth <fntoth@gmail.com> (Intel Edison-Arduino)
> Thank you!
>
>> While testing I noted another problem. I have "DMA-API: debugging enabled by
>> kernel config" and this (I guess) shows me before and after the patches:
>>
>> ------------[ cut here ]------------
>> DMA-API: xhci-hcd xhci-hcd.1.auto: cacheline tracking EEXIST, overlapping
>> mappings aren't supported
> That is under investigation here:
> https://bugzilla.kernel.org/show_bug.cgi?id=215740
>
> It's apparently a long-standing bug in the USB core which was exposed
> by a new WARN() check introduced in 5.14.

I'm not sure this is correct. The issue happens for me only when 
connecting the SMSC9414.

Other usb devices I have (memory sticks, wifi, bluetooth) do not trigger 
this.

I think we need to consider it might be a valid warning. It seems to be 
originating from the same "Workqueue: usb_hub_wq hub_event" though.

> Thanks,
>
> Lukas

Re: [PATCH net-next 0/7] Polling be gone on LAN95xx

[Lukas Wunner]

On Tue, May 03, 2022 at 04:26:58PM +0200, Ferry Toth wrote:
> On 03-05-2022 10:26, Lukas Wunner wrote:
> > On Mon, May 02, 2022 at 10:33:06PM +0200, Ferry Toth wrote:
> > > I have "DMA-API: debugging enabled by kernel config"
> > > and this (I guess) shows me before and after the patches:
> > > 
> > > ------------[ cut here ]------------
> > > DMA-API: xhci-hcd xhci-hcd.1.auto: cacheline tracking EEXIST, overlapping
> > > mappings aren't supported
> > 
> > That is under investigation here:
> > https://bugzilla.kernel.org/show_bug.cgi?id=215740
> > 
> > It's apparently a long-standing bug in the USB core which was exposed
> > by a new WARN() check introduced in 5.14.
> 
> I'm not sure this is correct. The issue happens for me only when connecting
> the SMSC9414.
> 
> Other usb devices I have (memory sticks, wifi, bluetooth) do not trigger
> this.

Hm, I've taken the liberty to add that information to the bugzilla.

Thanks,

Lukas