diff mbox series

brcmfmac: convert dev_init_lock mutex to completion

Message ID 1552470749-3625-1-git-send-email-p.figiel@camlintechnologies.com (mailing list archive)
State Accepted
Commit a9fd0953fa4a62887306be28641b4b0809f3b2fd
Delegated to: Kalle Valo
Headers show
Series brcmfmac: convert dev_init_lock mutex to completion | expand

Commit Message

Piotr Figiel March 13, 2019, 9:52 a.m. UTC 
Leaving dev_init_lock mutex locked in probe causes BUG and a WARNING when
kernel is compiled with CONFIG_PROVE_LOCKING. Convert mutex to completion
which silences those warnings and improves code readability.

Fix below errors when connecting the USB WiFi dongle:

brcmfmac: brcmf_fw_alloc_request: using brcm/brcmfmac43143 for chip BCM43143/2
BUG: workqueue leaked lock or atomic: kworker/0:2/0x00000000/434
     last function: hub_event
1 lock held by kworker/0:2/434:
 #0: 18d5dcdf (&devinfo->dev_init_lock){+.+.}, at: brcmf_usb_probe+0x78/0x550 [brcmfmac]
CPU: 0 PID: 434 Comm: kworker/0:2 Not tainted 4.19.23-00084-g454a789-dirty #123
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
Workqueue: usb_hub_wq hub_event
[<8011237c>] (unwind_backtrace) from [<8010d74c>] (show_stack+0x10/0x14)
[<8010d74c>] (show_stack) from [<809c4324>] (dump_stack+0xa8/0xd4)
[<809c4324>] (dump_stack) from [<8014195c>] (process_one_work+0x710/0x808)
[<8014195c>] (process_one_work) from [<80141a80>] (worker_thread+0x2c/0x564)
[<80141a80>] (worker_thread) from [<80147bcc>] (kthread+0x13c/0x16c)
[<80147bcc>] (kthread) from [<801010b4>] (ret_from_fork+0x14/0x20)
Exception stack(0xed1d9fb0 to 0xed1d9ff8)
9fa0:                                     00000000 00000000 00000000 00000000
9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
9fe0: 00000000 00000000 00000000 00000000 00000013 00000000

Comments

Kalle Valo April 4, 2019, 10:13 a.m. UTC | #1 
Piotr Figiel <p.figiel@camlintechnologies.com> wrote:

> Leaving dev_init_lock mutex locked in probe causes BUG and a WARNING when
> kernel is compiled with CONFIG_PROVE_LOCKING. Convert mutex to completion
> which silences those warnings and improves code readability.
> 
> Fix below errors when connecting the USB WiFi dongle:
> 
> brcmfmac: brcmf_fw_alloc_request: using brcm/brcmfmac43143 for chip BCM43143/2
> BUG: workqueue leaked lock or atomic: kworker/0:2/0x00000000/434
>      last function: hub_event
> 1 lock held by kworker/0:2/434:
>  #0: 18d5dcdf (&devinfo->dev_init_lock){+.+.}, at: brcmf_usb_probe+0x78/0x550 [brcmfmac]
> CPU: 0 PID: 434 Comm: kworker/0:2 Not tainted 4.19.23-00084-g454a789-dirty #123
> Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
> Workqueue: usb_hub_wq hub_event
> [<8011237c>] (unwind_backtrace) from [<8010d74c>] (show_stack+0x10/0x14)
> [<8010d74c>] (show_stack) from [<809c4324>] (dump_stack+0xa8/0xd4)
> [<809c4324>] (dump_stack) from [<8014195c>] (process_one_work+0x710/0x808)
> [<8014195c>] (process_one_work) from [<80141a80>] (worker_thread+0x2c/0x564)
> [<80141a80>] (worker_thread) from [<80147bcc>] (kthread+0x13c/0x16c)
> [<80147bcc>] (kthread) from [<801010b4>] (ret_from_fork+0x14/0x20)
> Exception stack(0xed1d9fb0 to 0xed1d9ff8)
> 9fa0:                                     00000000 00000000 00000000 00000000
> 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> 
> ======================================================
> WARNING: possible circular locking dependency detected
> 4.19.23-00084-g454a789-dirty #123 Not tainted
> ------------------------------------------------------
> kworker/0:2/434 is trying to acquire lock:
> e29cf799 ((wq_completion)"events"){+.+.}, at: process_one_work+0x174/0x808
> 
> but task is already holding lock:
> 18d5dcdf (&devinfo->dev_init_lock){+.+.}, at: brcmf_usb_probe+0x78/0x550 [brcmfmac]
> 
> which lock already depends on the new lock.
> 
> the existing dependency chain (in reverse order) is:
> 
> -> #2 (&devinfo->dev_init_lock){+.+.}:
>        mutex_lock_nested+0x1c/0x24
>        brcmf_usb_probe+0x78/0x550 [brcmfmac]
>        usb_probe_interface+0xc0/0x1bc
>        really_probe+0x228/0x2c0
>        __driver_attach+0xe4/0xe8
>        bus_for_each_dev+0x68/0xb4
>        bus_add_driver+0x19c/0x214
>        driver_register+0x78/0x110
>        usb_register_driver+0x84/0x148
>        process_one_work+0x228/0x808
>        worker_thread+0x2c/0x564
>        kthread+0x13c/0x16c
>        ret_from_fork+0x14/0x20
>          (null)
> 
> -> #1 (brcmf_driver_work){+.+.}:
>        worker_thread+0x2c/0x564
>        kthread+0x13c/0x16c
>        ret_from_fork+0x14/0x20
>          (null)
> 
> -> #0 ((wq_completion)"events"){+.+.}:
>        process_one_work+0x1b8/0x808
>        worker_thread+0x2c/0x564
>        kthread+0x13c/0x16c
>        ret_from_fork+0x14/0x20
>          (null)
> 
> other info that might help us debug this:
> 
> Chain exists of:
>   (wq_completion)"events" --> brcmf_driver_work --> &devinfo->dev_init_lock
> 
>  Possible unsafe locking scenario:
> 
>        CPU0                    CPU1
>        ----                    ----
>   lock(&devinfo->dev_init_lock);
>                                lock(brcmf_driver_work);
>                                lock(&devinfo->dev_init_lock);
>   lock((wq_completion)"events");
> 
>  *** DEADLOCK ***
> 
> 1 lock held by kworker/0:2/434:
>  #0: 18d5dcdf (&devinfo->dev_init_lock){+.+.}, at: brcmf_usb_probe+0x78/0x550 [brcmfmac]
> 
> stack backtrace:
> CPU: 0 PID: 434 Comm: kworker/0:2 Not tainted 4.19.23-00084-g454a789-dirty #123
> Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
> Workqueue: events request_firmware_work_func
> [<8011237c>] (unwind_backtrace) from [<8010d74c>] (show_stack+0x10/0x14)
> [<8010d74c>] (show_stack) from [<809c4324>] (dump_stack+0xa8/0xd4)
> [<809c4324>] (dump_stack) from [<80172838>] (print_circular_bug+0x210/0x330)
> [<80172838>] (print_circular_bug) from [<80175940>] (__lock_acquire+0x160c/0x1a30)
> [<80175940>] (__lock_acquire) from [<8017671c>] (lock_acquire+0xe0/0x268)
> [<8017671c>] (lock_acquire) from [<80141404>] (process_one_work+0x1b8/0x808)
> [<80141404>] (process_one_work) from [<80141a80>] (worker_thread+0x2c/0x564)
> [<80141a80>] (worker_thread) from [<80147bcc>] (kthread+0x13c/0x16c)
> [<80147bcc>] (kthread) from [<801010b4>] (ret_from_fork+0x14/0x20)
> Exception stack(0xed1d9fb0 to 0xed1d9ff8)
> 9fa0:                                     00000000 00000000 00000000 00000000
> 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> 
> Signed-off-by: Piotr Figiel <p.figiel@camlintechnologies.com>

Patch applied to wireless-drivers-next.git, thanks.

a9fd0953fa4a brcmfmac: convert dev_init_lock mutex to completion
diff mbox series

Patch

======================================================
WARNING: possible circular locking dependency detected
4.19.23-00084-g454a789-dirty #123 Not tainted
------------------------------------------------------
kworker/0:2/434 is trying to acquire lock:
e29cf799 ((wq_completion)"events"){+.+.}, at: process_one_work+0x174/0x808

but task is already holding lock:
18d5dcdf (&devinfo->dev_init_lock){+.+.}, at: brcmf_usb_probe+0x78/0x550 [brcmfmac]

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #2 (&devinfo->dev_init_lock){+.+.}:
       mutex_lock_nested+0x1c/0x24
       brcmf_usb_probe+0x78/0x550 [brcmfmac]
       usb_probe_interface+0xc0/0x1bc
       really_probe+0x228/0x2c0
       __driver_attach+0xe4/0xe8
       bus_for_each_dev+0x68/0xb4
       bus_add_driver+0x19c/0x214
       driver_register+0x78/0x110
       usb_register_driver+0x84/0x148
       process_one_work+0x228/0x808
       worker_thread+0x2c/0x564
       kthread+0x13c/0x16c
       ret_from_fork+0x14/0x20
         (null)

-> #1 (brcmf_driver_work){+.+.}:
       worker_thread+0x2c/0x564
       kthread+0x13c/0x16c
       ret_from_fork+0x14/0x20
         (null)

-> #0 ((wq_completion)"events"){+.+.}:
       process_one_work+0x1b8/0x808
       worker_thread+0x2c/0x564
       kthread+0x13c/0x16c
       ret_from_fork+0x14/0x20
         (null)

other info that might help us debug this:

Chain exists of:
  (wq_completion)"events" --> brcmf_driver_work --> &devinfo->dev_init_lock

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&devinfo->dev_init_lock);
                               lock(brcmf_driver_work);
                               lock(&devinfo->dev_init_lock);
  lock((wq_completion)"events");

 *** DEADLOCK ***

1 lock held by kworker/0:2/434:
 #0: 18d5dcdf (&devinfo->dev_init_lock){+.+.}, at: brcmf_usb_probe+0x78/0x550 [brcmfmac]

stack backtrace:
CPU: 0 PID: 434 Comm: kworker/0:2 Not tainted 4.19.23-00084-g454a789-dirty #123
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
Workqueue: events request_firmware_work_func
[<8011237c>] (unwind_backtrace) from [<8010d74c>] (show_stack+0x10/0x14)
[<8010d74c>] (show_stack) from [<809c4324>] (dump_stack+0xa8/0xd4)
[<809c4324>] (dump_stack) from [<80172838>] (print_circular_bug+0x210/0x330)
[<80172838>] (print_circular_bug) from [<80175940>] (__lock_acquire+0x160c/0x1a30)
[<80175940>] (__lock_acquire) from [<8017671c>] (lock_acquire+0xe0/0x268)
[<8017671c>] (lock_acquire) from [<80141404>] (process_one_work+0x1b8/0x808)
[<80141404>] (process_one_work) from [<80141a80>] (worker_thread+0x2c/0x564)
[<80141a80>] (worker_thread) from [<80147bcc>] (kthread+0x13c/0x16c)
[<80147bcc>] (kthread) from [<801010b4>] (ret_from_fork+0x14/0x20)
Exception stack(0xed1d9fb0 to 0xed1d9ff8)
9fa0:                                     00000000 00000000 00000000 00000000
9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
9fe0: 00000000 00000000 00000000 00000000 00000013 00000000

Signed-off-by: Piotr Figiel <p.figiel@camlintechnologies.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index e9cbfd0..a513990 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -160,7 +160,7 @@  struct brcmf_usbdev_info {
 
 	struct usb_device *usbdev;
 	struct device *dev;
-	struct mutex dev_init_lock;
+	struct completion dev_init_done;
 
 	int ctl_in_pipe, ctl_out_pipe;
 	struct urb *ctl_urb; /* URB for control endpoint */
@@ -1193,11 +1193,11 @@  static void brcmf_usb_probe_phase2(struct device *dev, int ret,
 	if (ret)
 		goto error;
 
-	mutex_unlock(&devinfo->dev_init_lock);
+	complete(&devinfo->dev_init_done);
 	return;
 error:
 	brcmf_dbg(TRACE, "failed: dev=%s, err=%d\n", dev_name(dev), ret);
-	mutex_unlock(&devinfo->dev_init_lock);
+	complete(&devinfo->dev_init_done);
 	device_release_driver(dev);
 }
 
@@ -1265,7 +1265,7 @@  static int brcmf_usb_probe_cb(struct brcmf_usbdev_info *devinfo)
 		if (ret)
 			goto fail;
 		/* we are done */
-		mutex_unlock(&devinfo->dev_init_lock);
+		complete(&devinfo->dev_init_done);
 		return 0;
 	}
 	bus->chip = bus_pub->devid;
@@ -1325,11 +1325,10 @@  brcmf_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
 
 	devinfo->usbdev = usb;
 	devinfo->dev = &usb->dev;
-	/* Take an init lock, to protect for disconnect while still loading.
+	/* Init completion, to protect for disconnect while still loading.
 	 * Necessary because of the asynchronous firmware load construction
 	 */
-	mutex_init(&devinfo->dev_init_lock);
-	mutex_lock(&devinfo->dev_init_lock);
+	init_completion(&devinfo->dev_init_done);
 
 	usb_set_intfdata(intf, devinfo);
 
@@ -1407,7 +1406,7 @@  brcmf_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	return 0;
 
 fail:
-	mutex_unlock(&devinfo->dev_init_lock);
+	complete(&devinfo->dev_init_done);
 	kfree(devinfo);
 	usb_set_intfdata(intf, NULL);
 	return ret;
@@ -1422,7 +1421,7 @@  brcmf_usb_disconnect(struct usb_interface *intf)
 	devinfo = (struct brcmf_usbdev_info *)usb_get_intfdata(intf);
 
 	if (devinfo) {
-		mutex_lock(&devinfo->dev_init_lock);
+		wait_for_completion(&devinfo->dev_init_done);
 		/* Make sure that devinfo still exists. Firmware probe routines
 		 * may have released the device and cleared the intfdata.
 		 */