Polling be gone on LAN95xx

Acked-by Oliver Neukum <[email protected]>

Cc Martyn Welch <[email protected]>

Cc Andrew Lunn <[email protected]>

---
 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(-)

drivers/net/usb/asix_devices.c +1 -5

index 38e47a93fb83..5b5eb630c4b7 100644
@@ -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;
 }

drivers/net/usb/smsc95xx.c +1 -2

index 4ef61f6b85df..edf0492ad489 100644
@@ -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;
 }

drivers/net/usb/usbnet.c +3 -3

index 9a6450f796dc..36b24ec11650 100644
@@ -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);

Signature · 1 line

2.35.2

Lukas Wunner

author

2022-05-03 13:21 UTC #2 lore raw

PATCH v2 2/7 usbnet: smsc95xx: Don't clear read-only PHY interrupt

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

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:

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

Tested-by Oleksij Rempel <[email protected]> # LAN9514/9512/9500

---
 drivers/net/usb/smsc95xx.c | 4 ----
 1 file changed, 4 deletions(-)

drivers/net/usb/smsc95xx.c +0 -4

index edf0492ad489..2cb44d65bbc3 100644
@@ -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_;

Signature · 1 line

2.35.2

Andrew Lunn

maintainer Reviewed-by on v2 2/7

2022-05-03 22:20 UTC #3 lore raw

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

Quoted 9 lines · 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:

Quoted 8 lines

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.
Tested-by Oleksij Rempel <[email protected]> # LAN9514/9512/9500
Tested-by Ferry Toth <[email protected]> # LAN9514
Signed-off-by Lukas Wunner <[email protected]>

Andrew

Lukas Wunner

author

2022-05-03 13:22 UTC #4 lore raw

PATCH v2 3/7 usbnet: smsc95xx: Don't reset PHY behind PHY driver's back

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().

Tested-by Oleksij Rempel <[email protected]> # LAN9514/9512/9500

Cc Martyn Welch <[email protected]>

Cc Gabriel Hojda <[email protected]>

---
 drivers/net/usb/smsc95xx.c | 18 ------------------
 1 file changed, 18 deletions(-)

drivers/net/usb/smsc95xx.c +0 ~~-18~~

index 2cb44d65bbc3..6c37c7adde1b 100644
@@ -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;

Signature · 1 line

2.35.2

Andrew Lunn

maintainer Reviewed-by on v2 3/7

2022-05-03 22:21 UTC #5 lore raw

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

Quoted 13 lines · 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().

Quoted 5 lines

Tested-by Oleksij Rempel <[email protected]> # LAN9514/9512/9500
Tested-by Ferry Toth <[email protected]> # LAN9514
Signed-off-by Lukas Wunner <[email protected]>
Cc Martyn Welch <[email protected]>
Cc Gabriel Hojda <[email protected]>

Andrew

Lukas Wunner

author

2022-05-03 13:23 UTC #6 lore raw

PATCH v2 4/7 usbnet: smsc95xx: Avoid link settings race on interrupt reception

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.

Note that the semicolon on a line by itself added in smsc95xx_status() is a placeholder for a function call which will be added in a subsequent commit. That function call will actually handle the INT_ENP_PHY_INT_ interrupt.

Tested-by Oleksij Rempel <[email protected]> # LAN9514/9512/9500

---
Changes since v1:
* Silence "Error updating MAC full duplex mode" warning if it's caused by hot-removal. (Oleksij Rempel)
* Explain in commit message why a semicolon on a line by itself is temporarily added in smsc95xx_status(). (Andrew Lunn)

 drivers/net/usb/smsc95xx.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

drivers/net/usb/smsc95xx.c +9 -7

index 6c37c7adde1b..6309ff8e75de 100644
@@ -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,16 @@ 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) {
+		if (ret != -ENODEV)
+			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 +609,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 +1072,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 +1978,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,

Signature · 1 line

2.35.2

Andrew Lunn

maintainer Reviewed-by on v2 4/7

2022-05-03 22:23 UTC #7 lore raw

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

Quoted 36 lines · 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.

Quoted 8 lines

Note that the semicolon on a line by itself added in smsc95xx_status() is a placeholder for a function call which will be added in a subsequent commit. That function call will actually handle the INT_ENP_PHY_INT_ interrupt.
Tested-by Oleksij Rempel <[email protected]> # LAN9514/9512/9500
Tested-by Ferry Toth <[email protected]> # LAN9514
Signed-off-by Lukas Wunner <[email protected]>

Andrew

Lukas Wunner

author

2022-05-03 13:25 UTC #8 lore raw

PATCH v2 5/7 usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid polling

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

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."

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().

In smsc95xx_resume(), move the call of phy_init_hw() before usbnet_resume() (which restarts the status URB) to ensure that the PHY is fully initialized when an interrupt is handled.

Tested-by Oleksij Rempel <[email protected]> # LAN9514/9512/9500

https://lore.kernel.org/all/c6b7f4e4a17913d2f2bc4fe722df0804c2d6fea7.1651574194.git.lukas@wunner.de/

Reviewed-by Andrew Lunn <[email protected]> # from a PHY perspective

Cc Andre Edich <[email protected]>

---
Changes since v1:
* Eliminate a checkpatch "no space before tabs" warning.
* Explain in commit message why the call to phy_init_hw() is moved in smsc95xx_resume().

 drivers/net/usb/smsc95xx.c | 118 +++++++++++++++++++++----------------
 1 file changed, 66 insertions(+), 52 deletions(-)

drivers/net/usb/smsc95xx.c +66 ~~-52~~

index 6309ff8e75de..9278d4e24d79 100644
@@ -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;
 };

@@ -597,6 +605,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) {

@@ -608,11 +618,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 */

@@ -1080,8 +1099,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");

@@ -1121,10 +1141,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);

@@ -1157,6 +1205,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 */

@@ -1199,6 +1248,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;

@@ -1211,6 +1269,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);
 }

@@ -1235,29 +1296,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;

@@ -1424,7 +1462,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 */

@@ -1443,27 +1480,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);
 }

@@ -1529,13 +1549,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
 		 */

@@ -1769,11 +1782,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;
 }

Signature · 1 line

2.35.2

Jakub Kicinski

maintainer

2022-05-05 18:39 UTC #9 lore raw

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

Quoted 9 lines · On Tue, 3 May 2022 15:15:05 +0200 Lukas Wunner wrote:

drivers/net/usb/smsc95xx.c +5 -0

@@ -608,11 +618,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();

drivers/net/usb/smsc95xx.c static void smsc95xx_status(struct usbnet *dev, struct urb *urb) · line 634

Quoted 10 lines

drivers/net/usb/smsc95xx.c +5 -1

+
 	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);

IRQ maintainers could you cast your eyes over this?

Full patch:

Lukas Wunner

author

2022-05-05 18:53 UTC #10 lore raw

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

Quoted 23 lines · Jakub Kicinski

Quoted 9 lines · On Tue, 3 May 2022 15:15:05 +0200 Lukas Wunner wrote:

drivers/net/usb/smsc95xx.c +5 -0

@@ -608,11 +618,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();

drivers/net/usb/smsc95xx.c static void smsc95xx_status(struct usbnet *dev, struct urb *urb) · line 634

Quoted 10 lines

drivers/net/usb/smsc95xx.c +5 -1

+
 	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);

IRQ maintainers could you cast your eyes over this?

Quoted 3 lines

Full patch:
https://lore.kernel.org/all/c6b7f4e4a17913d2f2bc4fe722df0804c2d6fea7.1651574194.git.lukas@wunner.de/

This is basically identical to what drivers/net/usb/lan78xx.c does in lan78xx_status(), except I'm passing the hw irq instead of the linux irq to genirq code, thereby avoiding the overhead of a radix_tree_lookup().

generic_handle_domain_irq() warns unconditionally on !in_irq(), unlike handle_irq_desc(), which constrains the warning to handle_enforce_irqctx() (i.e. x86 APIC, arm GIC/GICv3). Perhaps that's an oversight in generic_handle_domain_irq(), unless __irq_resolve_mapping() becomes unsafe outside in_irq() for some reason...

In any case the unconditional in_irq() necessitates __irq_enter_raw() here.

And there's no _safe variant() of generic_handle_domain_irq() (unlike generic_handle_irq_safe() which was recently added by 509853f9e1e7), hence the necessity of an explicit local_irq_save().

Thanks,

Lukas

Marc Zyngier

2022-05-06 10:58 UTC #11 lore raw

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

Quoted 41 lines · Lukas Wunner

Quoted 23 lines · Jakub Kicinski
Quoted 9 lines · On Tue, 3 May 2022 15:15:05 +0200 Lukas Wunner wrote:
drivers/net/usb/smsc95xx.c +5 -0
@@ -608,11 +618,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();
drivers/net/usb/smsc95xx.c static void smsc95xx_status(struct usbnet *dev, struct urb *urb) · line 634
Quoted 10 lines
drivers/net/usb/smsc95xx.c +5 -1
+
 	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);
IRQ maintainers could you cast your eyes over this?
Quoted 3 lines
Full patch:
https://lore.kernel.org/all/c6b7f4e4a17913d2f2bc4fe722df0804c2d6fea7.1651574194.git.lukas@wunner.de/
This is basically identical to what drivers/net/usb/lan78xx.c does in lan78xx_status(), except I'm passing the hw irq instead of the linux irq to genirq code, thereby avoiding the overhead of a radix_tree_lookup().
generic_handle_domain_irq() warns unconditionally on !in_irq(), unlike handle_irq_desc(), which constrains the warning to handle_enforce_irqctx() (i.e. x86 APIC, arm GIC/GICv3). Perhaps that's an oversight in generic_handle_domain_irq(), unless __irq_resolve_mapping() becomes unsafe outside in_irq() for some reason...

Quoted 6 lines

In any case the unconditional in_irq() necessitates __irq_enter_raw() here.
And there's no _safe variant() of generic_handle_domain_irq() (unlike generic_handle_irq_safe() which was recently added by 509853f9e1e7), hence the necessity of an explicit local_irq_save().

Please don't directly use __irq_enter_raw() and similar things directly in driver code (it doesn't do anything related to RCU, for example, which could be problematic if used in arbitrary contexts). Given how infrequent this interrupt is, I'd rather you use something similar to what lan78xx is doing, and be done with it.

And since this is a construct that seems to be often repeated, why don't you simply make the phy interrupt handling available over a smp_call_function() interface, which would always put you in the correct context and avoid faking things up?

Thanks,

M.

Signature · 1 line

Without deviation from the norm, progress is not possible.

Lukas Wunner

author

2022-05-06 20:17 UTC #12 lore raw

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

Quoted 52 lines · Marc Zyngier

Quoted 38 lines · Lukas Wunner
Quoted 23 lines · Jakub Kicinski
Quoted 9 lines · On Tue, 3 May 2022 15:15:05 +0200 Lukas Wunner wrote:
drivers/net/usb/smsc95xx.c +5 -0
@@ -608,11 +618,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();
drivers/net/usb/smsc95xx.c static void smsc95xx_status(struct usbnet *dev, struct urb *urb) · line 634
Quoted 10 lines
drivers/net/usb/smsc95xx.c +5 -1
+
 	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);
Full patch:
Quoted 1 line
https://lore.kernel.org/all/c6b7f4e4a17913d2f2bc4fe722df0804c2d6fea7.1651574194.git.lukas@wunner.de/
This is basically identical to what drivers/net/usb/lan78xx.c does in lan78xx_status(), except I'm passing the hw irq instead of the linux irq to genirq code, thereby avoiding the overhead of a radix_tree_lookup().
generic_handle_domain_irq() warns unconditionally on !in_irq(), unlike handle_irq_desc(), which constrains the warning to handle_enforce_irqctx() (i.e. x86 APIC, arm GIC/GICv3). Perhaps that's an oversight in generic_handle_domain_irq(), unless __irq_resolve_mapping() becomes unsafe outside in_irq() for some reason...
Quoted 6 lines
In any case the unconditional in_irq() necessitates __irq_enter_raw() here.
And there's no _safe variant() of generic_handle_domain_irq() (unlike generic_handle_irq_safe() which was recently added by 509853f9e1e7), hence the necessity of an explicit local_irq_save().
Please don't directly use __irq_enter_raw() and similar things directly in driver code (it doesn't do anything related to RCU, for example, which could be problematic if used in arbitrary contexts). Given how infrequent this interrupt is, I'd rather you use something similar to what lan78xx is doing, and be done with it.

Quoted 4 lines

And since this is a construct that seems to be often repeated, why don't you simply make the phy interrupt handling available over a smp_call_function() interface, which would always put you in the correct context and avoid faking things up?

As I've explained in the commit message (linked above), LAN95xx chips have 11 GPIOs which can be an interrupt source. This patch adds PHY interrupt support in such a way that GPIO interrupts can easily be supported by a subsequent commit. To this end, LAN95xx needs to be represented as a proper irqchip.

The crucial thing to understand is that a USB interrupt is not received as a hardirq. USB gadgets are incapable of directly signaling an interrupt because they cannot initiate a bus transaction by themselves. All communication on the bus is initiated by the host controller, which polls a gadget's Interrupt Endpoint in regular intervals. If an interrupt is pending, that information is passed up the stack in softirq context. Hence there's no other way than "faking things up", to borrow your language.

Another USB driver in the tree, drivers/gpio/gpio-dln2.c, likewise represents the USB gadget as an irqchip to signal GPIO interrupts. This shows that LAN95xx is not an isolated case. gpio-dln2.c does not invoke __irq_enter_raw(), so I think users will now see a WARN splat with that driver since Mark Rutland's 0953fb263714 (+cc).

As I've pointed out above, it seems like an oversight that Mark didn't make the WARN_ON_ONCE() conditional on handle_enforce_irqctx() (as handle_irq_desc() does). Sadly you did not respond to that observation. Please clarify whether that is indeed erroneous. Once handle_enforce_irqctx() is added to generic_handle_domain_irq(), there's no need for me to call __irq_enter_raw(). Problem solved.

Should there be a valid reason for the missing handle_enforce_irqctx(), then I propose adding a generic_handle_domain_irq_safe() function which calls __irq_enter_raw() (or probably __irq_enter() to get accounting), thereby resolving your objection to calling __irq_enter_raw() from a driver.

Thanks,

Lukas

Marc Zyngier

2022-05-09 09:25 UTC #13 lore raw

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

Quoted 80 lines · Lukas Wunner

Quoted 52 lines · Marc Zyngier
Quoted 38 lines · Lukas Wunner
Quoted 23 lines · Jakub Kicinski
Quoted 9 lines · On Tue, 3 May 2022 15:15:05 +0200 Lukas Wunner wrote:
drivers/net/usb/smsc95xx.c +5 -0
@@ -608,11 +618,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();
drivers/net/usb/smsc95xx.c static void smsc95xx_status(struct usbnet *dev, struct urb *urb) · line 634
Quoted 10 lines
drivers/net/usb/smsc95xx.c +5 -1
+
 	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);
Full patch:
Quoted 1 line
https://lore.kernel.org/all/c6b7f4e4a17913d2f2bc4fe722df0804c2d6fea7.1651574194.git.lukas@wunner.de/
This is basically identical to what drivers/net/usb/lan78xx.c does in lan78xx_status(), except I'm passing the hw irq instead of the linux irq to genirq code, thereby avoiding the overhead of a radix_tree_lookup().
generic_handle_domain_irq() warns unconditionally on !in_irq(), unlike handle_irq_desc(), which constrains the warning to handle_enforce_irqctx() (i.e. x86 APIC, arm GIC/GICv3). Perhaps that's an oversight in generic_handle_domain_irq(), unless __irq_resolve_mapping() becomes unsafe outside in_irq() for some reason...
Quoted 6 lines
In any case the unconditional in_irq() necessitates __irq_enter_raw() here.
And there's no _safe variant() of generic_handle_domain_irq() (unlike generic_handle_irq_safe() which was recently added by 509853f9e1e7), hence the necessity of an explicit local_irq_save().
Please don't directly use __irq_enter_raw() and similar things directly in driver code (it doesn't do anything related to RCU, for example, which could be problematic if used in arbitrary contexts). Given how infrequent this interrupt is, I'd rather you use something similar to what lan78xx is doing, and be done with it.
Quoted 4 lines
And since this is a construct that seems to be often repeated, why don't you simply make the phy interrupt handling available over a smp_call_function() interface, which would always put you in the correct context and avoid faking things up?
As I've explained in the commit message (linked above), LAN95xx chips have 11 GPIOs which can be an interrupt source. This patch adds PHY interrupt support in such a way that GPIO interrupts can easily be supported by a subsequent commit. To this end, LAN95xx needs to be represented as a proper irqchip.
The crucial thing to understand is that a USB interrupt is not received as a hardirq. USB gadgets are incapable of directly signaling an interrupt because they cannot initiate a bus transaction by themselves. All communication on the bus is initiated by the host controller, which polls a gadget's Interrupt Endpoint in regular intervals. If an interrupt is pending, that information is passed up the stack in softirq context. Hence there's no other way than "faking things up", to borrow your language.
Another USB driver in the tree, drivers/gpio/gpio-dln2.c, likewise represents the USB gadget as an irqchip to signal GPIO interrupts. This shows that LAN95xx is not an isolated case. gpio-dln2.c does not invoke __irq_enter_raw(), so I think users will now see a WARN splat with that driver since Mark Rutland's 0953fb263714 (+cc).

Quoted 4 lines

As I've pointed out above, it seems like an oversight that Mark didn't make the WARN_ON_ONCE() conditional on handle_enforce_irqctx() (as handle_irq_desc() does). Sadly you did not respond to that observation.

When did you make that observation? I can only see an email from you being sent *after* the one I am replying to.

Quoted 3 lines

Please clarify whether that is indeed erroneous.
Once handle_enforce_irqctx() is added to generic_handle_domain_irq(), there's no need for me to call __irq_enter_raw(). Problem solved.

I don't see it as an oversight. Drivers shouldn't rely on architectural quirks, and it is much clearer to simply forbid something that cannot be guaranteed across the board, specially for something that is as generic as USB.

Quoted 5 lines

Should there be a valid reason for the missing handle_enforce_irqctx(), then I propose adding a generic_handle_domain_irq_safe() function which calls __irq_enter_raw() (or probably __irq_enter() to get accounting), thereby resolving your objection to calling __irq_enter_raw() from a driver.

Feel free to submit a patch.

Thanks,

M.

Signature · 1 line

Without deviation from the norm, progress is not possible.

Lukas Wunner

author

2022-05-10 08:19 UTC #14 lore raw

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

Quoted 17 lines · Marc Zyngier

Quoted 16 lines · Lukas Wunner
Quoted 14 lines · Marc Zyngier
Quoted 9 lines · Lukas Wunner
generic_handle_domain_irq() warns unconditionally on !in_irq(), unlike handle_irq_desc(), which constrains the warning to handle_enforce_irqctx() (i.e. x86 APIC, arm GIC/GICv3). Perhaps that's an oversight in generic_handle_domain_irq(), unless __irq_resolve_mapping() becomes unsafe outside in_irq() for some reason...
In any case the unconditional in_irq() necessitates __irq_enter_raw() here.
Please don't directly use __irq_enter_raw() and similar things directly in driver code (it doesn't do anything related to RCU, for example, which could be problematic if used in arbitrary contexts).

Quoted 7 lines

Quoted 4 lines
As I've pointed out above, it seems like an oversight that Mark didn't make the WARN_ON_ONCE() conditional on handle_enforce_irqctx() (as handle_irq_desc() does). Sadly you did not respond to that observation.
When did you make that observation? I can only see an email from you being sent *after* the one I am replying to.

I was referring to the above-quoted sentence:

"generic_handle_domain_irq() warns unconditionally on !in_irq(),
unlike handle_irq_desc(), which constrains the warning to handle_enforce_irqctx() (i.e. x86 APIC, arm GIC/GICv3). Perhaps that's an oversight in generic_handle_domain_irq(), unless __irq_resolve_mapping() becomes unsafe outside in_irq() for some reason..."

Never mind, let's focus on the problem at hand.
It's secondary who said what when.

Quoted 8 lines

Quoted 3 lines
Please clarify whether that is indeed erroneous.
Once handle_enforce_irqctx() is added to generic_handle_domain_irq(), there's no need for me to call __irq_enter_raw(). Problem solved.
I don't see it as an oversight. Drivers shouldn't rely on architectural quirks, and it is much clearer to simply forbid something that cannot be guaranteed across the board, specially for something that is as generic as USB.

Whether a warning is warranted is not dependent on the architecture, but on the irqchip from which an interrupt normally originates:

* Interrupt normally originates from x86 APIC or arm GIC/GICv3, but is synthesized in non-hardirq context: Warning is warranted.

* Interrupt normally originates from any other top-level irqchip, such as irq-bcm2836.c, but is synthesized in non-hardirq context:
Warning is a false positive!

* Interrupt is always synthesized in non-hardirq context by a USB irqchip: Warning is a false positive, regardless whether the top-level irqchip is x86 APIC, arm GIC/GICv3 or anything else!

Quoted 7 lines

Quoted 5 lines
Should there be a valid reason for the missing handle_enforce_irqctx(), then I propose adding a generic_handle_domain_irq_safe() function which calls __irq_enter_raw() (or probably __irq_enter() to get accounting), thereby resolving your objection to calling __irq_enter_raw() from a driver.
Feel free to submit a patch.

Done:

https://lore.kernel.org/lkml/c3caf60bfa78e5fdbdf483096b7174da65d1813a.1652168866.git.lukas@wunner.de/

I'm focussing on eliminating the false-positive warning for now. Introducing a generic_handle_domain_irq_safe() wrapper which alleviates drivers from calling local_irq_save() can be done in a separate step.

Thanks,

Lukas

tip-bot2 for Lukas Wunner

merge bot Applied

2022-05-11 00:27 UTC #15 lore raw

[tip: irq/urgent] genirq: Remove WARN_ON_ONCE() in generic_handle_domain_irq()

In reply to Jakub Kicinski

The following commit has been merged into the irq/urgent branch of tip:

Commit-ID:     792ea6a074ae7ea5ab6f1b8b31f76bb0297de66c
Gitweb:        https://git.kernel.org/tip/792ea6a074ae7ea5ab6f1b8b31f76bb0297de66c
Author:        Lukas Wunner <[email protected]>
AuthorDate:    Tue, 10 May 2022 09:56:05 +02:00
Committer:     Thomas Gleixner <[email protected]>
CommitterDate: Wed, 11 May 2022 02:22:52 +02:00

genirq: Remove WARN_ON_ONCE() in generic_handle_domain_irq()

Since commit 0953fb263714 ("irq: remove handle_domain_{irq,nmi}()"), generic_handle_domain_irq() warns if called outside hardirq context, even though the function calls down to handle_irq_desc(), which warns about the same, but conditionally on handle_enforce_irqctx().

The newly added warning is a false positive if the interrupt originates from any other irqchip than x86 APIC or ARM GIC/GICv3. Those are the only ones for which handle_enforce_irqctx() returns true. Per commit c16816acd086 ("genirq: Add protection against unsafe usage of generic_handle_irq()"):

"In general calling generic_handle_irq() with interrupts disabled from non
interrupt context is harmless. For some interrupt controllers like the x86 trainwrecks this is outright dangerous as it might corrupt state if an interrupt affinity change is pending."

Examples for interrupt chips where the warning is a false positive are USB-attached GPIO controllers such as drivers/gpio/gpio-dln2.c:

USB gadgets are incapable of directly signaling an interrupt because they cannot initiate a bus transaction by themselves. All communication on the bus is initiated by the host controller, which polls a gadget's Interrupt Endpoint in regular intervals. If an interrupt is pending, that information is passed up the stack in softirq context, from which a hardirq is synthesized via generic_handle_domain_irq().

Remove the warning to eliminate such false positives.

Fixes 0953fb263714 ("irq: remove handle_domain_{irq,nmi}()")

Link https://lore.kernel.org/r/c3caf60bfa78e5fdbdf483096b7174da65d1813a.1652168866.git.lukas@wunner.de open here

Signed-off-by Thomas Gleixner <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Jakub Kicinski <[email protected]>

CC Linus Walleij <[email protected]>

Cc Bartosz Golaszewski <[email protected]>

Cc Octavian Purdila <[email protected]>

Cc [email protected]

Link https://lore.kernel.org/r/[email protected] open here

---
 kernel/irq/irqdesc.c | 1 -
 1 file changed, 1 deletion(-)

kernel/irq/irqdesc.c +0 -1

index 0099b87..d323b18 100644
@@ -701,7 +701,6 @@ EXPORT_SYMBOL_GPL(generic_handle_irq_safe);
  */
 int generic_handle_domain_irq(struct irq_domain *domain, unsigned int hwirq)
 {
-	WARN_ON_ONCE(!in_hardirq());
 	return handle_irq_desc(irq_resolve_mapping(domain, hwirq));
 }
 EXPORT_SYMBOL_GPL(generic_handle_domain_irq);

Lukas Wunner

author

2022-05-11 09:26 UTC #16 lore raw

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

In reply to Jakub Kicinski

Hi Jakub,

Quoted 14 lines · Jakub Kicinski

Quoted 13 lines · On Tue, 3 May 2022 15:15:05 +0200 Lukas Wunner wrote:

drivers/net/usb/smsc95xx.c +7 -1

@@ -608,11 +618,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);

Quoted 8 lines

drivers/net/usb/smsc95xx.c static void smsc95xx_status(struct usbnet *dev, struct urb *urb) · line 634
Quoted 6 lines
drivers/net/usb/smsc95xx.c +3 -0
 	else
 		netdev_warn(dev->net, "unexpected interrupt, intdata=0x%08X\n",
 			    intdata);
+
+	__irq_exit_raw();
+	local_irq_restore(flags);
IRQ maintainers could you cast your eyes over this?

Thomas applied 792ea6a074ae ("genirq: Remove WARN_ON_ONCE() in generic_handle_domain_irq()") tonight:

http://git.kernel.org/tip/tip/c/792ea6a074ae

That allows me to drop the controversial __irq_enter_raw().

Jakub, do you want me to resend the full series (all 7 patches) or should I send only patch [5/7] in-reply-to this one here? Or do you prefer applying all patches except [5/7] and have me resend that single patch?

Let me know what your preferred modus operandi is.

Thanks,

Lukas

Jakub Kicinski

maintainer

2022-05-11 16:15 UTC #17 lore raw

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

Quoted 6 lines · On Wed, 11 May 2022 11:26:16 +0200 Lukas Wunner wrote:

Quoted 1 line
IRQ maintainers could you cast your eyes over this?
Thomas applied 792ea6a074ae ("genirq: Remove WARN_ON_ONCE() in generic_handle_domain_irq()") tonight:
http://git.kernel.org/tip/tip/c/792ea6a074ae

Perfect!

Quoted 8 lines

That allows me to drop the controversial __irq_enter_raw().
Jakub, do you want me to resend the full series (all 7 patches) or should I send only patch [5/7] in-reply-to this one here? Or do you prefer applying all patches except [5/7] and have me resend that single patch?
Let me know what your preferred modus operandi is.

Resending all patches would be the easiest for us and has the lowest chance of screw up on our side, so resend all please & thanks!

Lukas Wunner

author

2022-05-03 13:26 UTC #18 lore raw

PATCH v2 6/7 net: phy: smsc: Cache interrupt mask

Cache the interrupt mask to avoid re-reading it from the PHY upon every interrupt.

This will simplify a subsequent commit which detects hot-removal in the interrupt handler and bails out.

Analyzing and debugging PHY transactions also becomes simpler if such redundant reads are avoided.

Last not least, interrupt overhead and latency is slightly improved.

Tested-by Oleksij Rempel <[email protected]> # LAN9514/9512/9500

---
Changes since v1:
* Expand commit message to explain in greater detail why caching the interrupt mask is beneficial. (Andrew Lunn)

 drivers/net/phy/smsc.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

drivers/net/phy/smsc.c +11 ~~-13~~

index d8cac02a79b9..a521d48b22a7 100644
@@ -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);

Signature · 1 line

2.35.2

Andrew Lunn

maintainer Reviewed-by on v2 6/7

2022-05-03 22:26 UTC #19 lore raw

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

Quoted 6 lines · Lukas Wunner

Cache the interrupt mask to avoid re-reading it from the PHY upon every interrupt.
This will simplify a subsequent commit which detects hot-removal in the interrupt handler and bails out.

Quoted 8 lines

Analyzing and debugging PHY transactions also becomes simpler if such redundant reads are avoided.
Last not least, interrupt overhead and latency is slightly improved.
Tested-by Oleksij Rempel <[email protected]> # LAN9514/9512/9500
Tested-by Ferry Toth <[email protected]> # LAN9514
Signed-off-by Lukas Wunner <[email protected]>

Andrew

Lukas Wunner

author

2022-05-03 13:28 UTC #20 lore raw

PATCH v2 7/7 net: phy: smsc: Cope with hot-removal in interrupt handler

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.

Tested-by Oleksij Rempel <[email protected]> # LAN9514/9512/9500

---
 drivers/net/phy/smsc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/net/phy/smsc.c +3 -1

index a521d48b22a7..35bff7fd234c 100644
@@ -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;
 	}

Signature · 1 line

2.35.2

Andrew Lunn

maintainer Reviewed-by on v2 7/7

2022-05-03 22:26 UTC #21 lore raw

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

Quoted 9 lines · 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.
Tested-by Oleksij Rempel <[email protected]> # LAN9514/9512/9500
Tested-by Ferry Toth <[email protected]> # LAN9514
Signed-off-by Lukas Wunner <[email protected]>