diff mbox series

[net,v1] lan743x: fix "BUG: invalid wait context" when setting rx mode

Message ID 20201109203828.5115-1-TheSven73@gmail.com
State Accepted
Delegated to: David Miller
Headers show
Series [net,v1] lan743x: fix "BUG: invalid wait context" when setting rx mode | expand

Checks

Context Check Description
jkicinski/cover_letter success Link
jkicinski/fixes_present fail Series targets non-next tree, but doesn't contain any Fixes tags
jkicinski/patch_count success Link
jkicinski/tree_selection success Clearly marked for net
jkicinski/subject_prefix success Link
jkicinski/source_inline success Was 0 now: 0
jkicinski/verify_signedoff success Link
jkicinski/module_param success Was 0 now: 0
jkicinski/build_32bit success Errors and warnings before: 0 this patch: 0
jkicinski/kdoc success Errors and warnings before: 0 this patch: 0
jkicinski/verify_fixes success Link
jkicinski/checkpatch success total: 0 errors, 0 warnings, 0 checks, 46 lines checked
jkicinski/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
jkicinski/header_inline success Link
jkicinski/stable success Stable not CCed

Commit Message

Sven Van Asbroeck Nov. 9, 2020, 8:38 p.m. UTC 
From: Sven Van Asbroeck <thesven73@gmail.com>

In the net core, the struct net_device_ops -> ndo_set_rx_mode()
callback is called with the dev->addr_list_lock spinlock held.

However, this driver's ndo_set_rx_mode callback eventually calls
lan743x_dp_write(), which acquires a mutex. Mutex acquisition
may sleep, and this is not allowed when holding a spinlock.

Fix by removing the dp_lock mutex entirely. Its purpose is to
prevent concurrent accesses to the data port. No concurrent
accesses are possible, because the dev->addr_list_lock
spinlock in the core only lets through one thread at a time.

Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com>
---

Tree: git://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git # 4e0396c59559

To: Bryan Whitehead <bryan.whitehead@microchip.com>
To: "David S. Miller" <davem@davemloft.net>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org (open list)

 drivers/net/ethernet/microchip/lan743x_main.c | 12 +++---------
 drivers/net/ethernet/microchip/lan743x_main.h |  3 ---
 2 files changed, 3 insertions(+), 12 deletions(-)

Comments

Jakub Kicinski Nov. 11, 2020, 1:55 a.m. UTC | #1 
On Mon,  9 Nov 2020 15:38:28 -0500 Sven Van Asbroeck wrote:
> From: Sven Van Asbroeck <thesven73@gmail.com>
> 
> In the net core, the struct net_device_ops -> ndo_set_rx_mode()
> callback is called with the dev->addr_list_lock spinlock held.
> 
> However, this driver's ndo_set_rx_mode callback eventually calls
> lan743x_dp_write(), which acquires a mutex. Mutex acquisition
> may sleep, and this is not allowed when holding a spinlock.
> 
> Fix by removing the dp_lock mutex entirely. Its purpose is to
> prevent concurrent accesses to the data port. No concurrent
> accesses are possible, because the dev->addr_list_lock
> spinlock in the core only lets through one thread at a time.

Please remember about fixes tags, I've added:

Fixes: 23f0703c125b ("lan743x: Add main source files for new lan743x driver")

here. No idea what this lock was trying to protect from the start.

Applied, thanks!
Sven Van Asbroeck Nov. 11, 2020, 2:51 a.m. UTC | #2 
On Tue, Nov 10, 2020 at 8:55 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Please remember about fixes tags

Will do in the future- I posted this patch before we had this
conversation about the need for Fixes tags in net.

>
> Applied, thanks!

Thank you !
diff mbox series

Patch

diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index d0817a974f8e..eb990e036611 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -674,14 +674,12 @@  static int lan743x_intr_open(struct lan743x_adapter *adapter)
 static int lan743x_dp_write(struct lan743x_adapter *adapter,
 			    u32 select, u32 addr, u32 length, u32 *buf)
 {
-	int ret = -EIO;
 	u32 dp_sel;
 	int i;
 
-	mutex_lock(&adapter->dp_lock);
 	if (lan743x_csr_wait_for_bit(adapter, DP_SEL, DP_SEL_DPRDY_,
 				     1, 40, 100, 100))
-		goto unlock;
+		return -EIO;
 	dp_sel = lan743x_csr_read(adapter, DP_SEL);
 	dp_sel &= ~DP_SEL_MASK_;
 	dp_sel |= select;
@@ -693,13 +691,10 @@  static int lan743x_dp_write(struct lan743x_adapter *adapter,
 		lan743x_csr_write(adapter, DP_CMD, DP_CMD_WRITE_);
 		if (lan743x_csr_wait_for_bit(adapter, DP_SEL, DP_SEL_DPRDY_,
 					     1, 40, 100, 100))
-			goto unlock;
+			return -EIO;
 	}
-	ret = 0;
 
-unlock:
-	mutex_unlock(&adapter->dp_lock);
-	return ret;
+	return 0;
 }
 
 static u32 lan743x_mac_mii_access(u16 id, u16 index, int read)
@@ -2720,7 +2715,6 @@  static int lan743x_hardware_init(struct lan743x_adapter *adapter,
 
 	adapter->intr.irq = adapter->pdev->irq;
 	lan743x_csr_write(adapter, INT_EN_CLR, 0xFFFFFFFF);
-	mutex_init(&adapter->dp_lock);
 
 	ret = lan743x_gpio_init(adapter);
 	if (ret)
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index 57fce752346e..3a0e70daa88f 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -711,9 +711,6 @@  struct lan743x_adapter {
 	struct lan743x_csr      csr;
 	struct lan743x_intr     intr;
 
-	/* lock, used to prevent concurrent access to data port */
-	struct mutex		dp_lock;
-
 	struct lan743x_gpio	gpio;
 	struct lan743x_ptp	ptp;