[PATCH net-next v2 0/3] Deadlock no more in LAN95xx

netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH net-next v2 0/3] Deadlock no more in LAN95xx
@ 2022-07-01 20:47 Lukas Wunner
  2022-07-01 20:47 ` [PATCH net-next v2 1/3] usbnet: smsc95xx: Fix deadlock on runtime resume Lukas Wunner
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Lukas Wunner @ 2022-07-01 20:47 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet
  Cc: netdev, linux-usb, Steve Glendinning, UNGLinuxDriver,
	Oliver Neukum, Andre Edich, Oleksij Rempel, Oleksij Rempel,
	Martyn Welch, Gabriel Hojda, Christoph Fritz, Lino Sanfilippo,
	Philipp Rosenberger, Marek Szyprowski, Ferry Toth, Andrew Lunn,
	Alan Stern

Second attempt at fixing a runtime resume deadlock in the LAN95xx USB driver:

In short, the driver isn't using the "nopm" register accessors in portions
of its runtime resume path, causing a deadlock.  I'm fixing that by
auto-detecting whether nopm accessors shall be used, instead of
having to explicitly call them wherever it's necessary.
As a byproduct, code size shrinks significantly (see diffstat below).

Back in April I submitted a first attempt which was rejected by Alan Stern:
https://lore.kernel.org/all/6710d8c18ff54139cdc538763ba544187c5a0cee.1651041411.git.lukas@wunner.de/

That approach only detected whether a PM callback is running concurrently,
not whether the access is performed by the PM callback.  I've come up with
a different approach which should resolve the objection (see patch [1/3]).

Thanks!

Lukas Wunner (3):
  usbnet: smsc95xx: Fix deadlock on runtime resume
  usbnet: smsc95xx: Clean up nopm handling
  usbnet: smsc95xx: Clean up unnecessary BUG_ON() upon register access

 drivers/net/usb/smsc95xx.c | 202 ++++++++++++++++---------------------
 1 file changed, 86 insertions(+), 116 deletions(-)

-- 
2.36.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH net-next v2 1/3] usbnet: smsc95xx: Fix deadlock on runtime resume
  2022-07-01 20:47 [PATCH net-next v2 0/3] Deadlock no more in LAN95xx Lukas Wunner
@ 2022-07-01 20:47 ` Lukas Wunner
  2022-07-01 20:47 ` [PATCH net-next v2 2/3] usbnet: smsc95xx: Clean up nopm handling Lukas Wunner
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Lukas Wunner @ 2022-07-01 20:47 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet
  Cc: netdev, linux-usb, Steve Glendinning, UNGLinuxDriver,
	Oliver Neukum, Andre Edich, Oleksij Rempel, Oleksij Rempel,
	Martyn Welch, Gabriel Hojda, Christoph Fritz, Lino Sanfilippo,
	Philipp Rosenberger, Marek Szyprowski, Ferry Toth, Andrew Lunn,
	Alan Stern

Commit 05b35e7eb9a1 ("smsc95xx: add phylib support") amended
smsc95xx_resume() to call phy_init_hw().  That function waits for the
device to runtime resume even though it is placed in the runtime resume
path, causing a deadlock.

The problem is that phy_init_hw() calls down to smsc95xx_mdiobus_read(),
which never uses the _nopm variant of usbnet_read_cmd().

Commit b4df480f68ae ("usbnet: smsc95xx: add reset_resume function with
reset operation") causes a similar deadlock on resume if the device was
already runtime suspended when entering system sleep:

That's because the commit introduced smsc95xx_reset_resume(), which
calls down to smsc95xx_reset(), which neglects to use _nopm accessors.

Fix by auto-detecting whether a device access is performed by the
suspend/resume task_struct and use the _nopm variant if so.  This works
because the PM core guarantees that suspend/resume callbacks are run in
task context.

Stacktrace for posterity:

  INFO: task kworker/2:1:49 blocked for more than 122 seconds.
  Workqueue: usb_hub_wq hub_event
  schedule
  rpm_resume
  __pm_runtime_resume
  usb_autopm_get_interface
  usbnet_read_cmd
  __smsc95xx_read_reg
  __smsc95xx_phy_wait_not_busy
  __smsc95xx_mdio_read
  smsc95xx_mdiobus_read
  __mdiobus_read
  mdiobus_read
  smsc_phy_reset
  phy_init_hw
  smsc95xx_resume
  usb_resume_interface
  usb_resume_both
  usb_runtime_resume
  __rpm_callback
  rpm_callback
  rpm_resume
  __pm_runtime_resume
  usb_autoresume_device
  hub_event
  process_one_work

Fixes: b4df480f68ae ("usbnet: smsc95xx: add reset_resume function with reset operation")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: stable@vger.kernel.org # v3.16+
Cc: Andre Edich <andre.edich@microchip.com>
---
 drivers/net/usb/smsc95xx.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 35110814ba22..0316c80c3fc4 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -71,6 +71,7 @@ struct smsc95xx_priv {
 	struct fwnode_handle *irqfwnode;
 	struct mii_bus *mdiobus;
 	struct phy_device *phydev;
+	struct task_struct *pm_task;
 };
 
 static bool turbo_mode = true;
@@ -80,13 +81,14 @@ MODULE_PARM_DESC(turbo_mode, "Enable multiple frames per Rx transaction");
 static int __must_check __smsc95xx_read_reg(struct usbnet *dev, u32 index,
 					    u32 *data, int in_pm)
 {
+	struct smsc95xx_priv *pdata = dev->driver_priv;
 	u32 buf;
 	int ret;
 	int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16);
 
 	BUG_ON(!dev);
 
-	if (!in_pm)
+	if (current != pdata->pm_task)
 		fn = usbnet_read_cmd;
 	else
 		fn = usbnet_read_cmd_nopm;
@@ -110,13 +112,14 @@ static int __must_check __smsc95xx_read_reg(struct usbnet *dev, u32 index,
 static int __must_check __smsc95xx_write_reg(struct usbnet *dev, u32 index,
 					     u32 data, int in_pm)
 {
+	struct smsc95xx_priv *pdata = dev->driver_priv;
 	u32 buf;
 	int ret;
 	int (*fn)(struct usbnet *, u8, u8, u16, u16, const void *, u16);
 
 	BUG_ON(!dev);
 
-	if (!in_pm)
+	if (current != pdata->pm_task)
 		fn = usbnet_write_cmd;
 	else
 		fn = usbnet_write_cmd_nopm;
@@ -1490,9 +1493,12 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
 	u32 val, link_up;
 	int ret;
 
+	pdata->pm_task = current;
+
 	ret = usbnet_suspend(intf, message);
 	if (ret < 0) {
 		netdev_warn(dev->net, "usbnet_suspend error\n");
+		pdata->pm_task = NULL;
 		return ret;
 	}
 
@@ -1732,6 +1738,7 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
 	if (ret && PMSG_IS_AUTO(message))
 		usbnet_resume(intf);
 
+	pdata->pm_task = NULL;
 	return ret;
 }
 
@@ -1752,29 +1759,31 @@ static int smsc95xx_resume(struct usb_interface *intf)
 	/* do this first to ensure it's cleared even in error case */
 	pdata->suspend_flags = 0;
 
+	pdata->pm_task = current;
+
 	if (suspend_flags & SUSPEND_ALLMODES) {
 		/* clear wake-up sources */
 		ret = smsc95xx_read_reg_nopm(dev, WUCSR, &val);
 		if (ret < 0)
-			return ret;
+			goto done;
 
 		val &= ~(WUCSR_WAKE_EN_ | WUCSR_MPEN_);
 
 		ret = smsc95xx_write_reg_nopm(dev, WUCSR, val);
 		if (ret < 0)
-			return ret;
+			goto done;
 
 		/* clear wake-up status */
 		ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
 		if (ret < 0)
-			return ret;
+			goto done;
 
 		val &= ~PM_CTL_WOL_EN_;
 		val |= PM_CTL_WUPS_;
 
 		ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
 		if (ret < 0)
-			return ret;
+			goto done;
 	}
 
 	phy_init_hw(pdata->phydev);
@@ -1783,15 +1792,20 @@ static int smsc95xx_resume(struct usb_interface *intf)
 	if (ret < 0)
 		netdev_warn(dev->net, "usbnet_resume error\n");
 
+done:
+	pdata->pm_task = NULL;
 	return ret;
 }
 
 static int smsc95xx_reset_resume(struct usb_interface *intf)
 {
 	struct usbnet *dev = usb_get_intfdata(intf);
+	struct smsc95xx_priv *pdata = dev->driver_priv;
 	int ret;
 
+	pdata->pm_task = current;
 	ret = smsc95xx_reset(dev);
+	pdata->pm_task = NULL;
 	if (ret < 0)
 		return ret;
 
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH net-next v2 2/3] usbnet: smsc95xx: Clean up nopm handling
  2022-07-01 20:47 [PATCH net-next v2 0/3] Deadlock no more in LAN95xx Lukas Wunner
  2022-07-01 20:47 ` [PATCH net-next v2 1/3] usbnet: smsc95xx: Fix deadlock on runtime resume Lukas Wunner
@ 2022-07-01 20:47 ` Lukas Wunner
  2022-07-01 20:47 ` [PATCH net-next v2 3/3] usbnet: smsc95xx: Clean up unnecessary BUG_ON() upon register access Lukas Wunner
  2022-07-04  9:50 ` [PATCH net-next v2 0/3] Deadlock no more in LAN95xx patchwork-bot+netdevbpf
  3 siblings, 0 replies; 5+ messages in thread
From: Lukas Wunner @ 2022-07-01 20:47 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet
  Cc: netdev, linux-usb, Steve Glendinning, UNGLinuxDriver,
	Oliver Neukum, Andre Edich, Oleksij Rempel, Oleksij Rempel,
	Martyn Welch, Gabriel Hojda, Christoph Fritz, Lino Sanfilippo,
	Philipp Rosenberger, Marek Szyprowski, Ferry Toth, Andrew Lunn,
	Alan Stern

The LAN95xx driver has just been amended to auto-detect whether the
_nopm variant of usbnet_read_cmd() / usbnet_write_cmd() shall be used.

Drop all the now unnecessary open coding of that distinction.

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

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 0316c80c3fc4..5458629cf694 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -78,8 +78,8 @@ static bool turbo_mode = true;
 module_param(turbo_mode, bool, 0644);
 MODULE_PARM_DESC(turbo_mode, "Enable multiple frames per Rx transaction");
 
-static int __must_check __smsc95xx_read_reg(struct usbnet *dev, u32 index,
-					    u32 *data, int in_pm)
+static int __must_check smsc95xx_read_reg(struct usbnet *dev, u32 index,
+					  u32 *data)
 {
 	struct smsc95xx_priv *pdata = dev->driver_priv;
 	u32 buf;
@@ -109,8 +109,8 @@ static int __must_check __smsc95xx_read_reg(struct usbnet *dev, u32 index,
 	return ret;
 }
 
-static int __must_check __smsc95xx_write_reg(struct usbnet *dev, u32 index,
-					     u32 data, int in_pm)
+static int __must_check smsc95xx_write_reg(struct usbnet *dev, u32 index,
+					   u32 data)
 {
 	struct smsc95xx_priv *pdata = dev->driver_priv;
 	u32 buf;
@@ -137,41 +137,16 @@ static int __must_check __smsc95xx_write_reg(struct usbnet *dev, u32 index,
 	return ret;
 }
 
-static int __must_check smsc95xx_read_reg_nopm(struct usbnet *dev, u32 index,
-					       u32 *data)
-{
-	return __smsc95xx_read_reg(dev, index, data, 1);
-}
-
-static int __must_check smsc95xx_write_reg_nopm(struct usbnet *dev, u32 index,
-						u32 data)
-{
-	return __smsc95xx_write_reg(dev, index, data, 1);
-}
-
-static int __must_check smsc95xx_read_reg(struct usbnet *dev, u32 index,
-					  u32 *data)
-{
-	return __smsc95xx_read_reg(dev, index, data, 0);
-}
-
-static int __must_check smsc95xx_write_reg(struct usbnet *dev, u32 index,
-					   u32 data)
-{
-	return __smsc95xx_write_reg(dev, index, data, 0);
-}
-
 /* Loop until the read is completed with timeout
  * called with phy_mutex held */
-static int __must_check __smsc95xx_phy_wait_not_busy(struct usbnet *dev,
-						     int in_pm)
+static int __must_check smsc95xx_phy_wait_not_busy(struct usbnet *dev)
 {
 	unsigned long start_time = jiffies;
 	u32 val;
 	int ret;
 
 	do {
-		ret = __smsc95xx_read_reg(dev, MII_ADDR, &val, in_pm);
+		ret = smsc95xx_read_reg(dev, MII_ADDR, &val);
 		if (ret < 0) {
 			/* Ignore -ENODEV error during disconnect() */
 			if (ret == -ENODEV)
@@ -192,8 +167,7 @@ static u32 mii_address_cmd(int phy_id, int idx, u16 op)
 	return (phy_id & 0x1f) << 11 | (idx & 0x1f) << 6 | op;
 }
 
-static int __smsc95xx_mdio_read(struct usbnet *dev, int phy_id, int idx,
-				int in_pm)
+static int smsc95xx_mdio_read(struct usbnet *dev, int phy_id, int idx)
 {
 	u32 val, addr;
 	int ret;
@@ -201,7 +175,7 @@ static int __smsc95xx_mdio_read(struct usbnet *dev, int phy_id, int idx,
 	mutex_lock(&dev->phy_mutex);
 
 	/* confirm MII not busy */
-	ret = __smsc95xx_phy_wait_not_busy(dev, in_pm);
+	ret = smsc95xx_phy_wait_not_busy(dev);
 	if (ret < 0) {
 		netdev_warn(dev->net, "%s: MII is busy\n", __func__);
 		goto done;
@@ -209,20 +183,20 @@ static int __smsc95xx_mdio_read(struct usbnet *dev, int phy_id, int idx,
 
 	/* set the address, index & direction (read from PHY) */
 	addr = mii_address_cmd(phy_id, idx, MII_READ_ | MII_BUSY_);
-	ret = __smsc95xx_write_reg(dev, MII_ADDR, addr, in_pm);
+	ret = smsc95xx_write_reg(dev, MII_ADDR, addr);
 	if (ret < 0) {
 		if (ret != -ENODEV)
 			netdev_warn(dev->net, "Error writing MII_ADDR\n");
 		goto done;
 	}
 
-	ret = __smsc95xx_phy_wait_not_busy(dev, in_pm);
+	ret = smsc95xx_phy_wait_not_busy(dev);
 	if (ret < 0) {
 		netdev_warn(dev->net, "Timed out reading MII reg %02X\n", idx);
 		goto done;
 	}
 
-	ret = __smsc95xx_read_reg(dev, MII_DATA, &val, in_pm);
+	ret = smsc95xx_read_reg(dev, MII_DATA, &val);
 	if (ret < 0) {
 		if (ret != -ENODEV)
 			netdev_warn(dev->net, "Error reading MII_DATA\n");
@@ -240,8 +214,8 @@ static int __smsc95xx_mdio_read(struct usbnet *dev, int phy_id, int idx,
 	return ret;
 }
 
-static void __smsc95xx_mdio_write(struct usbnet *dev, int phy_id,
-				  int idx, int regval, int in_pm)
+static void smsc95xx_mdio_write(struct usbnet *dev, int phy_id, int idx,
+				int regval)
 {
 	u32 val, addr;
 	int ret;
@@ -249,14 +223,14 @@ static void __smsc95xx_mdio_write(struct usbnet *dev, int phy_id,
 	mutex_lock(&dev->phy_mutex);
 
 	/* confirm MII not busy */
-	ret = __smsc95xx_phy_wait_not_busy(dev, in_pm);
+	ret = smsc95xx_phy_wait_not_busy(dev);
 	if (ret < 0) {
 		netdev_warn(dev->net, "%s: MII is busy\n", __func__);
 		goto done;
 	}
 
 	val = regval;
-	ret = __smsc95xx_write_reg(dev, MII_DATA, val, in_pm);
+	ret = smsc95xx_write_reg(dev, MII_DATA, val);
 	if (ret < 0) {
 		if (ret != -ENODEV)
 			netdev_warn(dev->net, "Error writing MII_DATA\n");
@@ -265,14 +239,14 @@ static void __smsc95xx_mdio_write(struct usbnet *dev, int phy_id,
 
 	/* set the address, index & direction (write to PHY) */
 	addr = mii_address_cmd(phy_id, idx, MII_WRITE_ | MII_BUSY_);
-	ret = __smsc95xx_write_reg(dev, MII_ADDR, addr, in_pm);
+	ret = smsc95xx_write_reg(dev, MII_ADDR, addr);
 	if (ret < 0) {
 		if (ret != -ENODEV)
 			netdev_warn(dev->net, "Error writing MII_ADDR\n");
 		goto done;
 	}
 
-	ret = __smsc95xx_phy_wait_not_busy(dev, in_pm);
+	ret = smsc95xx_phy_wait_not_busy(dev);
 	if (ret < 0) {
 		netdev_warn(dev->net, "Timed out writing MII reg %02X\n", idx);
 		goto done;
@@ -282,25 +256,11 @@ static void __smsc95xx_mdio_write(struct usbnet *dev, int phy_id,
 	mutex_unlock(&dev->phy_mutex);
 }
 
-static int smsc95xx_mdio_read_nopm(struct usbnet *dev, int idx)
-{
-	struct smsc95xx_priv *pdata = dev->driver_priv;
-
-	return __smsc95xx_mdio_read(dev, pdata->phydev->mdio.addr, idx, 1);
-}
-
-static void smsc95xx_mdio_write_nopm(struct usbnet *dev, int idx, int regval)
-{
-	struct smsc95xx_priv *pdata = dev->driver_priv;
-
-	__smsc95xx_mdio_write(dev, pdata->phydev->mdio.addr, idx, regval, 1);
-}
-
 static int smsc95xx_mdiobus_read(struct mii_bus *bus, int phy_id, int idx)
 {
 	struct usbnet *dev = bus->priv;
 
-	return __smsc95xx_mdio_read(dev, phy_id, idx, 0);
+	return smsc95xx_mdio_read(dev, phy_id, idx);
 }
 
 static int smsc95xx_mdiobus_write(struct mii_bus *bus, int phy_id, int idx,
@@ -308,7 +268,7 @@ static int smsc95xx_mdiobus_write(struct mii_bus *bus, int phy_id, int idx,
 {
 	struct usbnet *dev = bus->priv;
 
-	__smsc95xx_mdio_write(dev, phy_id, idx, regval, 0);
+	smsc95xx_mdio_write(dev, phy_id, idx, regval);
 	return 0;
 }
 
@@ -868,7 +828,7 @@ static int smsc95xx_start_tx_path(struct usbnet *dev)
 }
 
 /* Starts the Receive path */
-static int smsc95xx_start_rx_path(struct usbnet *dev, int in_pm)
+static int smsc95xx_start_rx_path(struct usbnet *dev)
 {
 	struct smsc95xx_priv *pdata = dev->driver_priv;
 	unsigned long flags;
@@ -877,7 +837,7 @@ static int smsc95xx_start_rx_path(struct usbnet *dev, int in_pm)
 	pdata->mac_cr |= MAC_CR_RXEN_;
 	spin_unlock_irqrestore(&pdata->mac_cr_lock, flags);
 
-	return __smsc95xx_write_reg(dev, MAC_CR, pdata->mac_cr, in_pm);
+	return smsc95xx_write_reg(dev, MAC_CR, pdata->mac_cr);
 }
 
 static int smsc95xx_reset(struct usbnet *dev)
@@ -1060,7 +1020,7 @@ static int smsc95xx_reset(struct usbnet *dev)
 		return ret;
 	}
 
-	ret = smsc95xx_start_rx_path(dev, 0);
+	ret = smsc95xx_start_rx_path(dev);
 	if (ret < 0) {
 		netdev_warn(dev->net, "Failed to start RX path\n");
 		return ret;
@@ -1294,16 +1254,17 @@ static u32 smsc_crc(const u8 *buffer, size_t len, int filter)
 	return crc << ((filter % 2) * 16);
 }
 
-static int smsc95xx_link_ok_nopm(struct usbnet *dev)
+static int smsc95xx_link_ok(struct usbnet *dev)
 {
+	struct smsc95xx_priv *pdata = dev->driver_priv;
 	int ret;
 
 	/* first, a dummy read, needed to latch some MII phys */
-	ret = smsc95xx_mdio_read_nopm(dev, MII_BMSR);
+	ret = smsc95xx_mdio_read(dev, pdata->phydev->mdio.addr, MII_BMSR);
 	if (ret < 0)
 		return ret;
 
-	ret = smsc95xx_mdio_read_nopm(dev, MII_BMSR);
+	ret = smsc95xx_mdio_read(dev, pdata->phydev->mdio.addr, MII_BMSR);
 	if (ret < 0)
 		return ret;
 
@@ -1316,14 +1277,14 @@ static int smsc95xx_enter_suspend0(struct usbnet *dev)
 	u32 val;
 	int ret;
 
-	ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
+	ret = smsc95xx_read_reg(dev, PM_CTRL, &val);
 	if (ret < 0)
 		return ret;
 
 	val &= (~(PM_CTL_SUS_MODE_ | PM_CTL_WUPS_ | PM_CTL_PHY_RST_));
 	val |= PM_CTL_SUS_MODE_0;
 
-	ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
+	ret = smsc95xx_write_reg(dev, PM_CTRL, val);
 	if (ret < 0)
 		return ret;
 
@@ -1335,12 +1296,12 @@ static int smsc95xx_enter_suspend0(struct usbnet *dev)
 	if (pdata->wolopts & WAKE_PHY)
 		val |= PM_CTL_WUPS_ED_;
 
-	ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
+	ret = smsc95xx_write_reg(dev, PM_CTRL, val);
 	if (ret < 0)
 		return ret;
 
 	/* read back PM_CTRL */
-	ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
+	ret = smsc95xx_read_reg(dev, PM_CTRL, &val);
 	if (ret < 0)
 		return ret;
 
@@ -1352,34 +1313,34 @@ static int smsc95xx_enter_suspend0(struct usbnet *dev)
 static int smsc95xx_enter_suspend1(struct usbnet *dev)
 {
 	struct smsc95xx_priv *pdata = dev->driver_priv;
+	int ret, phy_id = pdata->phydev->mdio.addr;
 	u32 val;
-	int ret;
 
 	/* reconfigure link pulse detection timing for
 	 * compatibility with non-standard link partners
 	 */
 	if (pdata->features & FEATURE_PHY_NLP_CROSSOVER)
-		smsc95xx_mdio_write_nopm(dev, PHY_EDPD_CONFIG,
-					 PHY_EDPD_CONFIG_DEFAULT);
+		smsc95xx_mdio_write(dev, phy_id, PHY_EDPD_CONFIG,
+				    PHY_EDPD_CONFIG_DEFAULT);
 
 	/* enable energy detect power-down mode */
-	ret = smsc95xx_mdio_read_nopm(dev, PHY_MODE_CTRL_STS);
+	ret = smsc95xx_mdio_read(dev, phy_id, PHY_MODE_CTRL_STS);
 	if (ret < 0)
 		return ret;
 
 	ret |= MODE_CTRL_STS_EDPWRDOWN_;
 
-	smsc95xx_mdio_write_nopm(dev, PHY_MODE_CTRL_STS, ret);
+	smsc95xx_mdio_write(dev, phy_id, PHY_MODE_CTRL_STS, ret);
 
 	/* enter SUSPEND1 mode */
-	ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
+	ret = smsc95xx_read_reg(dev, PM_CTRL, &val);
 	if (ret < 0)
 		return ret;
 
 	val &= ~(PM_CTL_SUS_MODE_ | PM_CTL_WUPS_ | PM_CTL_PHY_RST_);
 	val |= PM_CTL_SUS_MODE_1;
 
-	ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
+	ret = smsc95xx_write_reg(dev, PM_CTRL, val);
 	if (ret < 0)
 		return ret;
 
@@ -1387,7 +1348,7 @@ static int smsc95xx_enter_suspend1(struct usbnet *dev)
 	val &= ~PM_CTL_WUPS_;
 	val |= (PM_CTL_WUPS_ED_ | PM_CTL_ED_EN_);
 
-	ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
+	ret = smsc95xx_write_reg(dev, PM_CTRL, val);
 	if (ret < 0)
 		return ret;
 
@@ -1402,14 +1363,14 @@ static int smsc95xx_enter_suspend2(struct usbnet *dev)
 	u32 val;
 	int ret;
 
-	ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
+	ret = smsc95xx_read_reg(dev, PM_CTRL, &val);
 	if (ret < 0)
 		return ret;
 
 	val &= ~(PM_CTL_SUS_MODE_ | PM_CTL_WUPS_ | PM_CTL_PHY_RST_);
 	val |= PM_CTL_SUS_MODE_2;
 
-	ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
+	ret = smsc95xx_write_reg(dev, PM_CTRL, val);
 	if (ret < 0)
 		return ret;
 
@@ -1424,7 +1385,7 @@ static int smsc95xx_enter_suspend3(struct usbnet *dev)
 	u32 val;
 	int ret;
 
-	ret = smsc95xx_read_reg_nopm(dev, RX_FIFO_INF, &val);
+	ret = smsc95xx_read_reg(dev, RX_FIFO_INF, &val);
 	if (ret < 0)
 		return ret;
 
@@ -1433,14 +1394,14 @@ static int smsc95xx_enter_suspend3(struct usbnet *dev)
 		return -EBUSY;
 	}
 
-	ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
+	ret = smsc95xx_read_reg(dev, PM_CTRL, &val);
 	if (ret < 0)
 		return ret;
 
 	val &= ~(PM_CTL_SUS_MODE_ | PM_CTL_WUPS_ | PM_CTL_PHY_RST_);
 	val |= PM_CTL_SUS_MODE_3 | PM_CTL_RES_CLR_WKP_STS;
 
-	ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
+	ret = smsc95xx_write_reg(dev, PM_CTRL, val);
 	if (ret < 0)
 		return ret;
 
@@ -1448,7 +1409,7 @@ static int smsc95xx_enter_suspend3(struct usbnet *dev)
 	val &= ~PM_CTL_WUPS_;
 	val |= PM_CTL_WUPS_WOL_;
 
-	ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
+	ret = smsc95xx_write_reg(dev, PM_CTRL, val);
 	if (ret < 0)
 		return ret;
 
@@ -1507,8 +1468,7 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
 		pdata->suspend_flags = 0;
 	}
 
-	/* determine if link is up using only _nopm functions */
-	link_up = smsc95xx_link_ok_nopm(dev);
+	link_up = smsc95xx_link_ok(dev);
 
 	if (message.event == PM_EVENT_AUTO_SUSPEND &&
 	    (pdata->features & FEATURE_REMOTE_WAKEUP)) {
@@ -1525,23 +1485,23 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
 		netdev_info(dev->net, "entering SUSPEND2 mode\n");
 
 		/* disable energy detect (link up) & wake up events */
-		ret = smsc95xx_read_reg_nopm(dev, WUCSR, &val);
+		ret = smsc95xx_read_reg(dev, WUCSR, &val);
 		if (ret < 0)
 			goto done;
 
 		val &= ~(WUCSR_MPEN_ | WUCSR_WAKE_EN_);
 
-		ret = smsc95xx_write_reg_nopm(dev, WUCSR, val);
+		ret = smsc95xx_write_reg(dev, WUCSR, val);
 		if (ret < 0)
 			goto done;
 
-		ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
+		ret = smsc95xx_read_reg(dev, PM_CTRL, &val);
 		if (ret < 0)
 			goto done;
 
 		val &= ~(PM_CTL_ED_EN_ | PM_CTL_WOL_EN_);
 
-		ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
+		ret = smsc95xx_write_reg(dev, PM_CTRL, val);
 		if (ret < 0)
 			goto done;
 
@@ -1632,7 +1592,7 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
 		}
 
 		for (i = 0; i < (wuff_filter_count * 4); i++) {
-			ret = smsc95xx_write_reg_nopm(dev, WUFF, filter_mask[i]);
+			ret = smsc95xx_write_reg(dev, WUFF, filter_mask[i]);
 			if (ret < 0) {
 				kfree(filter_mask);
 				goto done;
@@ -1641,50 +1601,50 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
 		kfree(filter_mask);
 
 		for (i = 0; i < (wuff_filter_count / 4); i++) {
-			ret = smsc95xx_write_reg_nopm(dev, WUFF, command[i]);
+			ret = smsc95xx_write_reg(dev, WUFF, command[i]);
 			if (ret < 0)
 				goto done;
 		}
 
 		for (i = 0; i < (wuff_filter_count / 4); i++) {
-			ret = smsc95xx_write_reg_nopm(dev, WUFF, offset[i]);
+			ret = smsc95xx_write_reg(dev, WUFF, offset[i]);
 			if (ret < 0)
 				goto done;
 		}
 
 		for (i = 0; i < (wuff_filter_count / 2); i++) {
-			ret = smsc95xx_write_reg_nopm(dev, WUFF, crc[i]);
+			ret = smsc95xx_write_reg(dev, WUFF, crc[i]);
 			if (ret < 0)
 				goto done;
 		}
 
 		/* clear any pending pattern match packet status */
-		ret = smsc95xx_read_reg_nopm(dev, WUCSR, &val);
+		ret = smsc95xx_read_reg(dev, WUCSR, &val);
 		if (ret < 0)
 			goto done;
 
 		val |= WUCSR_WUFR_;
 
-		ret = smsc95xx_write_reg_nopm(dev, WUCSR, val);
+		ret = smsc95xx_write_reg(dev, WUCSR, val);
 		if (ret < 0)
 			goto done;
 	}
 
 	if (pdata->wolopts & WAKE_MAGIC) {
 		/* clear any pending magic packet status */
-		ret = smsc95xx_read_reg_nopm(dev, WUCSR, &val);
+		ret = smsc95xx_read_reg(dev, WUCSR, &val);
 		if (ret < 0)
 			goto done;
 
 		val |= WUCSR_MPR_;
 
-		ret = smsc95xx_write_reg_nopm(dev, WUCSR, val);
+		ret = smsc95xx_write_reg(dev, WUCSR, val);
 		if (ret < 0)
 			goto done;
 	}
 
 	/* enable/disable wakeup sources */
-	ret = smsc95xx_read_reg_nopm(dev, WUCSR, &val);
+	ret = smsc95xx_read_reg(dev, WUCSR, &val);
 	if (ret < 0)
 		goto done;
 
@@ -1704,12 +1664,12 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
 		val &= ~WUCSR_MPEN_;
 	}
 
-	ret = smsc95xx_write_reg_nopm(dev, WUCSR, val);
+	ret = smsc95xx_write_reg(dev, WUCSR, val);
 	if (ret < 0)
 		goto done;
 
 	/* enable wol wakeup source */
-	ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
+	ret = smsc95xx_read_reg(dev, PM_CTRL, &val);
 	if (ret < 0)
 		goto done;
 
@@ -1719,12 +1679,12 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
 	if (pdata->wolopts & WAKE_PHY)
 		val |= PM_CTL_ED_EN_;
 
-	ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
+	ret = smsc95xx_write_reg(dev, PM_CTRL, val);
 	if (ret < 0)
 		goto done;
 
 	/* enable receiver to enable frame reception */
-	smsc95xx_start_rx_path(dev, 1);
+	smsc95xx_start_rx_path(dev);
 
 	/* some wol options are enabled, so enter SUSPEND0 */
 	netdev_info(dev->net, "entering SUSPEND0 mode\n");
@@ -1763,25 +1723,25 @@ static int smsc95xx_resume(struct usb_interface *intf)
 
 	if (suspend_flags & SUSPEND_ALLMODES) {
 		/* clear wake-up sources */
-		ret = smsc95xx_read_reg_nopm(dev, WUCSR, &val);
+		ret = smsc95xx_read_reg(dev, WUCSR, &val);
 		if (ret < 0)
 			goto done;
 
 		val &= ~(WUCSR_WAKE_EN_ | WUCSR_MPEN_);
 
-		ret = smsc95xx_write_reg_nopm(dev, WUCSR, val);
+		ret = smsc95xx_write_reg(dev, WUCSR, val);
 		if (ret < 0)
 			goto done;
 
 		/* clear wake-up status */
-		ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
+		ret = smsc95xx_read_reg(dev, PM_CTRL, &val);
 		if (ret < 0)
 			goto done;
 
 		val &= ~PM_CTL_WOL_EN_;
 		val |= PM_CTL_WUPS_;
 
-		ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
+		ret = smsc95xx_write_reg(dev, PM_CTRL, val);
 		if (ret < 0)
 			goto done;
 	}
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH net-next v2 3/3] usbnet: smsc95xx: Clean up unnecessary BUG_ON() upon register access
  2022-07-01 20:47 [PATCH net-next v2 0/3] Deadlock no more in LAN95xx Lukas Wunner
  2022-07-01 20:47 ` [PATCH net-next v2 1/3] usbnet: smsc95xx: Fix deadlock on runtime resume Lukas Wunner
  2022-07-01 20:47 ` [PATCH net-next v2 2/3] usbnet: smsc95xx: Clean up nopm handling Lukas Wunner
@ 2022-07-01 20:47 ` Lukas Wunner
  2022-07-04  9:50 ` [PATCH net-next v2 0/3] Deadlock no more in LAN95xx patchwork-bot+netdevbpf
  3 siblings, 0 replies; 5+ messages in thread
From: Lukas Wunner @ 2022-07-01 20:47 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet
  Cc: netdev, linux-usb, Steve Glendinning, UNGLinuxDriver,
	Oliver Neukum, Andre Edich, Oleksij Rempel, Oleksij Rempel,
	Martyn Welch, Gabriel Hojda, Christoph Fritz, Lino Sanfilippo,
	Philipp Rosenberger, Marek Szyprowski, Ferry Toth, Andrew Lunn,
	Alan Stern

smsc95xx_read_reg() and smsc95xx_write_reg() call BUG_ON() if the
struct usbnet pointer passed in is NULL.

The functions have just been amended to dereference the pointer on
entry.  So the kernel now oopses if the pointer is NULL, eliminating
the need for an explicit BUG_ON().

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 5458629cf694..bfb58c91db04 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -86,8 +86,6 @@ static int __must_check smsc95xx_read_reg(struct usbnet *dev, u32 index,
 	int ret;
 	int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16);
 
-	BUG_ON(!dev);
-
 	if (current != pdata->pm_task)
 		fn = usbnet_read_cmd;
 	else
@@ -117,8 +115,6 @@ static int __must_check smsc95xx_write_reg(struct usbnet *dev, u32 index,
 	int ret;
 	int (*fn)(struct usbnet *, u8, u8, u16, u16, const void *, u16);
 
-	BUG_ON(!dev);
-
 	if (current != pdata->pm_task)
 		fn = usbnet_write_cmd;
 	else
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH net-next v2 0/3] Deadlock no more in LAN95xx
  2022-07-01 20:47 [PATCH net-next v2 0/3] Deadlock no more in LAN95xx Lukas Wunner
                   ` (2 preceding siblings ...)
  2022-07-01 20:47 ` [PATCH net-next v2 3/3] usbnet: smsc95xx: Clean up unnecessary BUG_ON() upon register access Lukas Wunner
@ 2022-07-04  9:50 ` patchwork-bot+netdevbpf
  3 siblings, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-07-04  9:50 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: davem, kuba, pabeni, edumazet, netdev, linux-usb,
	steve.glendinning, UNGLinuxDriver, oneukum, andre.edich, o.rempel,
	linux, martyn.welch, ghojda, chf.fritz, LinoSanfilippo,
	p.rosenberger, m.szyprowski, fntoth, andrew, stern

Hello:

This series was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Fri, 1 Jul 2022 22:47:50 +0200 you wrote:
> Second attempt at fixing a runtime resume deadlock in the LAN95xx USB driver:
> 
> In short, the driver isn't using the "nopm" register accessors in portions
> of its runtime resume path, causing a deadlock.  I'm fixing that by
> auto-detecting whether nopm accessors shall be used, instead of
> having to explicitly call them wherever it's necessary.
> As a byproduct, code size shrinks significantly (see diffstat below).
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/3] usbnet: smsc95xx: Fix deadlock on runtime resume
    https://git.kernel.org/netdev/net-next/c/7b960c967f2a
  - [net-next,v2,2/3] usbnet: smsc95xx: Clean up nopm handling
    https://git.kernel.org/netdev/net-next/c/3147242980c5
  - [net-next,v2,3/3] usbnet: smsc95xx: Clean up unnecessary BUG_ON() upon register access
    https://git.kernel.org/netdev/net-next/c/03b3df43ce1f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-07-04  9:50 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-01 20:47 [PATCH net-next v2 0/3] Deadlock no more in LAN95xx Lukas Wunner
2022-07-01 20:47 ` [PATCH net-next v2 1/3] usbnet: smsc95xx: Fix deadlock on runtime resume Lukas Wunner
2022-07-01 20:47 ` [PATCH net-next v2 2/3] usbnet: smsc95xx: Clean up nopm handling Lukas Wunner
2022-07-01 20:47 ` [PATCH net-next v2 3/3] usbnet: smsc95xx: Clean up unnecessary BUG_ON() upon register access Lukas Wunner
2022-07-04  9:50 ` [PATCH net-next v2 0/3] Deadlock no more in LAN95xx patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).