Re: [PATCH] Bluetooth: hci_serdev: Init hci_uart proto_lock to avoid oops

[Marcel Holtmann]

Hi John,

 John Stultz reports a boot time crash with the HiKey board (which uses
 hci_serdev) occurring in hci_uart_tx_wakeup().  That function is
 contained in hci_ldisc.c, but also called from the newer hci_serdev.c.
 It acquires the proto_lock in struct hci_uart and it turns out that we
 forgot to init the lock in the serdev code path, thus causing the crash.
 
 John bisected the crash to commit 67d2f8781b9f ("Bluetooth: hci_ldisc:
 Allow sleeping while proto locks are held"), but the issue was present
 before and the commit merely exposed it.  (Perhaps by luck, the crash
 did not occur with rwlocks.)
 
 Init the proto_lock in the serdev code path to avoid the oops.
 
>> [snip]
>>> patch has been applied to bluetooth-next tree.
>> 
>> Sorry to be a nuisance if its just a timing thing, but I wanted to
>> follow up on this just to make sure it didn't fall through the cracks,
>> as I noticed w/ -rc3 it hasn't landed yet.
> 
> Happy new year all,
>  Just wanted to send another ping on this as it seems it hasn't made
> it into -rc6. Did this get missed as being tagged as a (at least
> functional) regression fix?

since it wasn’t marked as urgent fix, it never went on the path to -rc. It is 
on the path for the next release. And then can be included in -stable if needed.

Regards

Marcel

Re: [PATCH] Bluetooth: hci_serdev: Init hci_uart proto_lock to avoid oops

[Marcel Holtmann]

Hi John,

 John Stultz reports a boot time crash with the HiKey board (which uses
 hci_serdev) occurring in hci_uart_tx_wakeup().  That function is
 contained in hci_ldisc.c, but also called from the newer hci_serdev.c.
 It acquires the proto_lock in struct hci_uart and it turns out that we
 forgot to init the lock in the serdev code path, thus causing the crash.
 
 John bisected the crash to commit 67d2f8781b9f ("Bluetooth: hci_ldisc:
 Allow sleeping while proto locks are held"), but the issue was present
 before and the commit merely exposed it.  (Perhaps by luck, the crash
 did not occur with rwlocks.)
 
 Init the proto_lock in the serdev code path to avoid the oops.
 
>> [snip]
>>> patch has been applied to bluetooth-next tree.
>> 
>> Sorry to be a nuisance if its just a timing thing, but I wanted to
>> follow up on this just to make sure it didn't fall through the cracks,
>> as I noticed w/ -rc3 it hasn't landed yet.
> 
> Happy new year all,
>  Just wanted to send another ping on this as it seems it hasn't made
> it into -rc6. Did this get missed as being tagged as a (at least
> functional) regression fix?

since it wasn’t marked as urgent fix, it never went on the path to -rc. It is 
on the path for the next release. And then can be included in -stable if needed.

Regards

Marcel

Re: [PATCH] Bluetooth: hci_serdev: Init hci_uart proto_lock to avoid oops

[John Stultz]

On Mon, Dec 11, 2017 at 1:49 PM, John Stultz  wrote:
> On Thu, Nov 16, 2017 at 10:07 PM, Marcel Holtmann  wrote:
>> Hi Lukas,
>>
>>> John Stultz reports a boot time crash with the HiKey board (which uses
>>> hci_serdev) occurring in hci_uart_tx_wakeup().  That function is
>>> contained in hci_ldisc.c, but also called from the newer hci_serdev.c.
>>> It acquires the proto_lock in struct hci_uart and it turns out that we
>>> forgot to init the lock in the serdev code path, thus causing the crash.
>>>
>>> John bisected the crash to commit 67d2f8781b9f ("Bluetooth: hci_ldisc:
>>> Allow sleeping while proto locks are held"), but the issue was present
>>> before and the commit merely exposed it.  (Perhaps by luck, the crash
>>> did not occur with rwlocks.)
>>>
>>> Init the proto_lock in the serdev code path to avoid the oops.
>>>
> [snip]
>> patch has been applied to bluetooth-next tree.
>
> Sorry to be a nuisance if its just a timing thing, but I wanted to
> follow up on this just to make sure it didn't fall through the cracks,
> as I noticed w/ -rc3 it hasn't landed yet.

Happy new year all,
  Just wanted to send another ping on this as it seems it hasn't made
it into -rc6. Did this get missed as being tagged as a (at least
functional) regression fix?

thanks
-john

Re: [PATCH] Bluetooth: hci_serdev: Init hci_uart proto_lock to avoid oops

[John Stultz]

On Mon, Dec 11, 2017 at 1:49 PM, John Stultz  wrote:
> On Thu, Nov 16, 2017 at 10:07 PM, Marcel Holtmann  wrote:
>> Hi Lukas,
>>
>>> John Stultz reports a boot time crash with the HiKey board (which uses
>>> hci_serdev) occurring in hci_uart_tx_wakeup().  That function is
>>> contained in hci_ldisc.c, but also called from the newer hci_serdev.c.
>>> It acquires the proto_lock in struct hci_uart and it turns out that we
>>> forgot to init the lock in the serdev code path, thus causing the crash.
>>>
>>> John bisected the crash to commit 67d2f8781b9f ("Bluetooth: hci_ldisc:
>>> Allow sleeping while proto locks are held"), but the issue was present
>>> before and the commit merely exposed it.  (Perhaps by luck, the crash
>>> did not occur with rwlocks.)
>>>
>>> Init the proto_lock in the serdev code path to avoid the oops.
>>>
> [snip]
>> patch has been applied to bluetooth-next tree.
>
> Sorry to be a nuisance if its just a timing thing, but I wanted to
> follow up on this just to make sure it didn't fall through the cracks,
> as I noticed w/ -rc3 it hasn't landed yet.

Happy new year all,
  Just wanted to send another ping on this as it seems it hasn't made
it into -rc6. Did this get missed as being tagged as a (at least
functional) regression fix?

thanks
-john

Re: [PATCH] Bluetooth: hci_serdev: Init hci_uart proto_lock to avoid oops

[John Stultz]

On Thu, Nov 16, 2017 at 10:07 PM, Marcel Holtmann  wrote:
> Hi Lukas,
>
>> John Stultz reports a boot time crash with the HiKey board (which uses
>> hci_serdev) occurring in hci_uart_tx_wakeup().  That function is
>> contained in hci_ldisc.c, but also called from the newer hci_serdev.c.
>> It acquires the proto_lock in struct hci_uart and it turns out that we
>> forgot to init the lock in the serdev code path, thus causing the crash.
>>
>> John bisected the crash to commit 67d2f8781b9f ("Bluetooth: hci_ldisc:
>> Allow sleeping while proto locks are held"), but the issue was present
>> before and the commit merely exposed it.  (Perhaps by luck, the crash
>> did not occur with rwlocks.)
>>
>> Init the proto_lock in the serdev code path to avoid the oops.
>>
[snip]
> patch has been applied to bluetooth-next tree.

Sorry to be a nuisance if its just a timing thing, but I wanted to
follow up on this just to make sure it didn't fall through the cracks,
as I noticed w/ -rc3 it hasn't landed yet.

thanks
-john

Re: [PATCH] Bluetooth: hci_serdev: Init hci_uart proto_lock to avoid oops

[John Stultz]

On Thu, Nov 16, 2017 at 10:07 PM, Marcel Holtmann  wrote:
> Hi Lukas,
>
>> John Stultz reports a boot time crash with the HiKey board (which uses
>> hci_serdev) occurring in hci_uart_tx_wakeup().  That function is
>> contained in hci_ldisc.c, but also called from the newer hci_serdev.c.
>> It acquires the proto_lock in struct hci_uart and it turns out that we
>> forgot to init the lock in the serdev code path, thus causing the crash.
>>
>> John bisected the crash to commit 67d2f8781b9f ("Bluetooth: hci_ldisc:
>> Allow sleeping while proto locks are held"), but the issue was present
>> before and the commit merely exposed it.  (Perhaps by luck, the crash
>> did not occur with rwlocks.)
>>
>> Init the proto_lock in the serdev code path to avoid the oops.
>>
[snip]
> patch has been applied to bluetooth-next tree.

Sorry to be a nuisance if its just a timing thing, but I wanted to
follow up on this just to make sure it didn't fall through the cracks,
as I noticed w/ -rc3 it hasn't landed yet.

thanks
-john

Re: [PATCH] Bluetooth: hci_serdev: Init hci_uart proto_lock to avoid oops

[Marcel Holtmann]

Hi Lukas,

> John Stultz reports a boot time crash with the HiKey board (which uses
> hci_serdev) occurring in hci_uart_tx_wakeup().  That function is
> contained in hci_ldisc.c, but also called from the newer hci_serdev.c.
> It acquires the proto_lock in struct hci_uart and it turns out that we
> forgot to init the lock in the serdev code path, thus causing the crash.
> 
> John bisected the crash to commit 67d2f8781b9f ("Bluetooth: hci_ldisc:
> Allow sleeping while proto locks are held"), but the issue was present
> before and the commit merely exposed it.  (Perhaps by luck, the crash
> did not occur with rwlocks.)
> 
> Init the proto_lock in the serdev code path to avoid the oops.
> 
> Stack trace for posterity:
> 
> Unable to handle kernel read from unreadable memory at 406f127000
> [00406f127000] user address but active_mm is swapper
> Internal error: Oops: 9605 [#1] PREEMPT SMP
> Hardware name: HiKey Development Board (DT)
> Call trace:
> hci_uart_tx_wakeup+0x38/0x148
> hci_uart_send_frame+0x28/0x38
> hci_send_frame+0x64/0xc0
> hci_cmd_work+0x98/0x110
> process_one_work+0x134/0x330
> worker_thread+0x130/0x468
> kthread+0xf8/0x128
> ret_from_fork+0x10/0x18
> 
> Link: https://lkml.org/lkml/2017/11/15/908
> Reported-and-tested-by: John Stultz 
> Cc: Ronald Tschalär 
> Cc: Rob Herring 
> Cc: Sumit Semwal 
> Signed-off-by: Lukas Wunner 
> ---
> @Rob (and everyone else):  I'm not sure if this is in fact the correct
> approach, or if we should instead duplicate hci_uart_tx_wakeup() in
> hci_serdev.c (sans locking?), much as we've duplicated a lot of other
> functions there.  Let me know what your preference is.  Thanks!
> 
> drivers/bluetooth/hci_serdev.c | 1 +
> 1 file changed, 1 insertion(+)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH] Bluetooth: hci_serdev: Init hci_uart proto_lock to avoid oops

[Marcel Holtmann]

Hi Lukas,

> John Stultz reports a boot time crash with the HiKey board (which uses
> hci_serdev) occurring in hci_uart_tx_wakeup().  That function is
> contained in hci_ldisc.c, but also called from the newer hci_serdev.c.
> It acquires the proto_lock in struct hci_uart and it turns out that we
> forgot to init the lock in the serdev code path, thus causing the crash.
> 
> John bisected the crash to commit 67d2f8781b9f ("Bluetooth: hci_ldisc:
> Allow sleeping while proto locks are held"), but the issue was present
> before and the commit merely exposed it.  (Perhaps by luck, the crash
> did not occur with rwlocks.)
> 
> Init the proto_lock in the serdev code path to avoid the oops.
> 
> Stack trace for posterity:
> 
> Unable to handle kernel read from unreadable memory at 406f127000
> [00406f127000] user address but active_mm is swapper
> Internal error: Oops: 9605 [#1] PREEMPT SMP
> Hardware name: HiKey Development Board (DT)
> Call trace:
> hci_uart_tx_wakeup+0x38/0x148
> hci_uart_send_frame+0x28/0x38
> hci_send_frame+0x64/0xc0
> hci_cmd_work+0x98/0x110
> process_one_work+0x134/0x330
> worker_thread+0x130/0x468
> kthread+0xf8/0x128
> ret_from_fork+0x10/0x18
> 
> Link: https://lkml.org/lkml/2017/11/15/908
> Reported-and-tested-by: John Stultz 
> Cc: Ronald Tschalär 
> Cc: Rob Herring 
> Cc: Sumit Semwal 
> Signed-off-by: Lukas Wunner 
> ---
> @Rob (and everyone else):  I'm not sure if this is in fact the correct
> approach, or if we should instead duplicate hci_uart_tx_wakeup() in
> hci_serdev.c (sans locking?), much as we've duplicated a lot of other
> functions there.  Let me know what your preference is.  Thanks!
> 
> drivers/bluetooth/hci_serdev.c | 1 +
> 1 file changed, 1 insertion(+)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH] Bluetooth: hci_serdev: Init hci_uart proto_lock to avoid oops

[Lukas Wunner]

On Fri, Nov 17, 2017 at 12:54:53AM +0100, Lukas Wunner wrote:
> John Stultz reports a boot time crash with the HiKey board (which uses
> hci_serdev) occurring in hci_uart_tx_wakeup().  That function is
> contained in hci_ldisc.c, but also called from the newer hci_serdev.c.
> It acquires the proto_lock in struct hci_uart and it turns out that we
> forgot to init the lock in the serdev code path, thus causing the crash.
> 
> John bisected the crash to commit 67d2f8781b9f ("Bluetooth: hci_ldisc:
> Allow sleeping while proto locks are held"), but the issue was present
> before and the commit merely exposed it.  (Perhaps by luck, the crash
> did not occur with rwlocks.)
> 
> Init the proto_lock in the serdev code path to avoid the oops.

Apologies, I botched Gustavo Padovan's and Johan Hedberg's e-mail
address in the To: header, please fix up when replying.

Lukas

Re: [PATCH] Bluetooth: hci_serdev: Init hci_uart proto_lock to avoid oops

[Lukas Wunner]

On Fri, Nov 17, 2017 at 12:54:53AM +0100, Lukas Wunner wrote:
> John Stultz reports a boot time crash with the HiKey board (which uses
> hci_serdev) occurring in hci_uart_tx_wakeup().  That function is
> contained in hci_ldisc.c, but also called from the newer hci_serdev.c.
> It acquires the proto_lock in struct hci_uart and it turns out that we
> forgot to init the lock in the serdev code path, thus causing the crash.
> 
> John bisected the crash to commit 67d2f8781b9f ("Bluetooth: hci_ldisc:
> Allow sleeping while proto locks are held"), but the issue was present
> before and the commit merely exposed it.  (Perhaps by luck, the crash
> did not occur with rwlocks.)
> 
> Init the proto_lock in the serdev code path to avoid the oops.

Apologies, I botched Gustavo Padovan's and Johan Hedberg's e-mail
address in the To: header, please fix up when replying.

Lukas

[PATCH] Bluetooth: hci_serdev: Init hci_uart proto_lock to avoid oops

[Lukas Wunner]

John Stultz reports a boot time crash with the HiKey board (which uses
hci_serdev) occurring in hci_uart_tx_wakeup().  That function is
contained in hci_ldisc.c, but also called from the newer hci_serdev.c.
It acquires the proto_lock in struct hci_uart and it turns out that we
forgot to init the lock in the serdev code path, thus causing the crash.

John bisected the crash to commit 67d2f8781b9f ("Bluetooth: hci_ldisc:
Allow sleeping while proto locks are held"), but the issue was present
before and the commit merely exposed it.  (Perhaps by luck, the crash
did not occur with rwlocks.)

Init the proto_lock in the serdev code path to avoid the oops.

Stack trace for posterity:

Unable to handle kernel read from unreadable memory at 406f127000
[00406f127000] user address but active_mm is swapper
Internal error: Oops: 9605 [#1] PREEMPT SMP
Hardware name: HiKey Development Board (DT)
Call trace:
 hci_uart_tx_wakeup+0x38/0x148
 hci_uart_send_frame+0x28/0x38
 hci_send_frame+0x64/0xc0
 hci_cmd_work+0x98/0x110
 process_one_work+0x134/0x330
 worker_thread+0x130/0x468
 kthread+0xf8/0x128
 ret_from_fork+0x10/0x18

Link: https://lkml.org/lkml/2017/11/15/908
Reported-and-tested-by: John Stultz 
Cc: Ronald Tschalär 
Cc: Rob Herring 
Cc: Sumit Semwal 
Signed-off-by: Lukas Wunner 
---
@Rob (and everyone else):  I'm not sure if this is in fact the correct
approach, or if we should instead duplicate hci_uart_tx_wakeup() in
hci_serdev.c (sans locking?), much as we've duplicated a lot of other
functions there.  Let me know what your preference is.  Thanks!

 drivers/bluetooth/hci_serdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bluetooth/hci_serdev.c b/drivers/bluetooth/hci_serdev.c
index 71664b2..e0e6461 100644
--- a/drivers/bluetooth/hci_serdev.c
+++ b/drivers/bluetooth/hci_serdev.c
@@ -303,6 +303,7 @@ int hci_uart_register_device(struct hci_uart *hu,
hci_set_drvdata(hdev, hu);
 
INIT_WORK(>write_work, hci_uart_write_work);
+   percpu_init_rwsem(>proto_lock);
 
/* Only when vendor specific setup callback is provided, consider
 * the manufacturer information valid. This avoids filling in the
-- 
2.11.0

[PATCH] Bluetooth: hci_serdev: Init hci_uart proto_lock to avoid oops

[Lukas Wunner]

John Stultz reports a boot time crash with the HiKey board (which uses
hci_serdev) occurring in hci_uart_tx_wakeup().  That function is
contained in hci_ldisc.c, but also called from the newer hci_serdev.c.
It acquires the proto_lock in struct hci_uart and it turns out that we
forgot to init the lock in the serdev code path, thus causing the crash.

John bisected the crash to commit 67d2f8781b9f ("Bluetooth: hci_ldisc:
Allow sleeping while proto locks are held"), but the issue was present
before and the commit merely exposed it.  (Perhaps by luck, the crash
did not occur with rwlocks.)

Init the proto_lock in the serdev code path to avoid the oops.

Stack trace for posterity:

Unable to handle kernel read from unreadable memory at 406f127000
[00406f127000] user address but active_mm is swapper
Internal error: Oops: 9605 [#1] PREEMPT SMP
Hardware name: HiKey Development Board (DT)
Call trace:
 hci_uart_tx_wakeup+0x38/0x148
 hci_uart_send_frame+0x28/0x38
 hci_send_frame+0x64/0xc0
 hci_cmd_work+0x98/0x110
 process_one_work+0x134/0x330
 worker_thread+0x130/0x468
 kthread+0xf8/0x128
 ret_from_fork+0x10/0x18

Link: https://lkml.org/lkml/2017/11/15/908
Reported-and-tested-by: John Stultz 
Cc: Ronald Tschalär 
Cc: Rob Herring 
Cc: Sumit Semwal 
Signed-off-by: Lukas Wunner 
---
@Rob (and everyone else):  I'm not sure if this is in fact the correct
approach, or if we should instead duplicate hci_uart_tx_wakeup() in
hci_serdev.c (sans locking?), much as we've duplicated a lot of other
functions there.  Let me know what your preference is.  Thanks!

 drivers/bluetooth/hci_serdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bluetooth/hci_serdev.c b/drivers/bluetooth/hci_serdev.c
index 71664b2..e0e6461 100644
--- a/drivers/bluetooth/hci_serdev.c
+++ b/drivers/bluetooth/hci_serdev.c
@@ -303,6 +303,7 @@ int hci_uart_register_device(struct hci_uart *hu,
hci_set_drvdata(hdev, hu);
 
INIT_WORK(>write_work, hci_uart_write_work);
+   percpu_init_rwsem(>proto_lock);
 
/* Only when vendor specific setup callback is provided, consider
 * the manufacturer information valid. This avoids filling in the
-- 
2.11.0