[PULL -- 5.1 REGRESSION] Bluetooth: btusb: request wake pin with NOAUTOEN

[PULL -- 5.1 REGRESSION] Bluetooth: btusb: request wake pin with NOAUTOEN

[Brian Norris]

Badly-designed systems might have (for example) active-high wake pins
that default to high (e.g., because of external pull ups) until they
have an active firmware which starts driving it low. This can cause an
interrupt storm in the time between request_irq() and disable_irq().

We don't support shared interrupts here, so let's just pre-configure the
interrupt to avoid auto-enabling it.

Fixes: fd913ef7ce61 ("Bluetooth: btusb: Add out-of-band wakeup support")
Fixes: 5364a0b4f4be ("arm64: dts: rockchip: move QCA6174A wakeup pin into its USB node")
Signed-off-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
Hi Linus,

Can you please apply this for v5.1-rc5? This fixes a regression that I
reported several times. I nearly just sent a proper pull request, but I
figured this 2-line patch was clearer.

This patch was originally sent here:
https://lore.kernel.org/patchwork/patch/1044896/
https://lkml.kernel.org/lkml/20190222003051.127006-1-briannorris@chromium.org/

My other series was merged, even though it gave a clear dependency on
this bugfix:

https://lkml.kernel.org/lkml/CA+ASDXMePkQDRfaSwNGnRYyGdsuvfUCXBtDK79o2mP=1hdNQUA@mail.gmail.com

Several pings were ignored, so here I'm submitting this to you, Linus.

Thanks,
Brian

 drivers/bluetooth/btusb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index ded198328f21..7db48ae65cd2 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2942,6 +2942,7 @@ static int btusb_config_oob_wake(struct hci_dev *hdev)
 		return 0;
 	}
 
+	irq_set_status_flags(irq, IRQ_NOAUTOEN);
 	ret = devm_request_irq(&hdev->dev, irq, btusb_oob_wake_handler,
 			       0, "OOB Wake-on-BT", data);
 	if (ret) {
@@ -2956,7 +2957,6 @@ static int btusb_config_oob_wake(struct hci_dev *hdev)
 	}
 
 	data->oob_wake_irq = irq;
-	disable_irq(irq);
 	bt_dev_info(hdev, "OOB Wake-on-BT configured at IRQ %u", irq);
 	return 0;
 }
-- 
2.21.0.392.gf8f6787159e-goog

Re: [PULL -- 5.1 REGRESSION] Bluetooth: btusb: request wake pin with NOAUTOEN

[Linus Torvalds]

On Tue, Apr 9, 2019 at 8:49 AM Brian Norris <briannorris@chromium.org> wrote:
>
> Badly-designed systems might have (for example) active-high wake pins
> that default to high (e.g., because of external pull ups) until they
> have an active firmware which starts driving it low. This can cause an
> interrupt storm in the time between request_irq() and disable_irq().

Why is the fix not to move the request_irq() down to below the proper
initialization sequence?

That's what drivers *should* do: initialize their hardware first,
request interrupts only after that. Initializing the interrupt handler
before the hw is actually up seems wrong..

              Linus

Re: [PULL -- 5.1 REGRESSION] Bluetooth: btusb: request wake pin with NOAUTOEN

[Brian Norris]

Hi Linus,

Thanks for the reply! It's amusing that you're the only one (well,
besides the gentleman who sits a few feet from me and kindly provided
his Reviewed-by) to review my patch.

On Tue, Apr 9, 2019 at 7:20 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Apr 9, 2019 at 8:49 AM Brian Norris <briannorris@chromium.org> wrote:
> > Badly-designed systems might have (for example) active-high wake pins
> > that default to high (e.g., because of external pull ups) until they
> > have an active firmware which starts driving it low. This can cause an
> > interrupt storm in the time between request_irq() and disable_irq().
>
> Why is the fix not to move the request_irq() down to below the proper
> initialization sequence?
>
> That's what drivers *should* do: initialize their hardware first,
> request interrupts only after that. Initializing the interrupt handler
> before the hw is actually up seems wrong..

I suppose that's a possible solution. It appears that would involve
moving this IRQ management into btusb_{open,close}, after
->setup_on_usb(). I don't immediately see a good reason why we
couldn't do that.

But the use of NOAUTOEN is still important, because there's not really
any direct way to ack/clear the underlying signal, even when the
hardware is fully initialized. It's just a level-triggered wakeup
signal, which represents "activity" from the device, and we're relying
on the disable_irq_nosync() / BTUSB_OOB_WAKE_ENABLED dance to keep
things in sync. (I'm not proud of that exactly, but I think it's
otherwise correct.) Currently, this is the only window of time where
we trust that the device remains "quiet". Even if we move this until
after full HW initialization, I think we're still trusting the BT
firmware (a bit too much) not to assert this signal.

So, I think the problem is still potentially present no matter when we
request the IRQ. The "uninitialized" state of the hardware (or,
firmware) just exposes the issue extremely clearly.

If you'd rather send this back to the drawing board, I can just send a
revert for commit 5364a0b4f4be ("arm64: dts: rockchip: move QCA6174A
wakeup pin into its USB node") for 5.1 instead. I don't mean to take
too much of your time; I just want my regression fixed, and nothing
further up the MAINTAINERS file helped so far.

Thanks,
Brian

Re: [PULL -- 5.1 REGRESSION] Bluetooth: btusb: request wake pin with NOAUTOEN

[Linus Torvalds]

On Tue, Apr 9, 2019 at 5:26 PM Brian Norris <briannorris@chromium.org> wrote:
>
> So, I think the problem is still potentially present no matter when we
> request the IRQ. The "uninitialized" state of the hardware (or,
> firmware) just exposes the issue extremely clearly.

Well, I think that as long as you don't request the irq, and it's not
shared with anything else, the bogus state of the irq line simply
doesn't matter. So the NOAUTOEN shouldn't matter if the irq is
requested properly late.

Either it's edge-triggered and you'll get one possibly spurious
interrupt for an old issue, or it's level-triggered and setting up the
hw should bring the irq line inactive and you'll be ok.

But I've applied your patch for now simply because it seems to be a
smaller change.

But I think you should look into whether it can be fixed by just
requesting the irq once the hardware is really up (which may indeed be
as late as open time).

                   Linus

Re: [PULL -- 5.1 REGRESSION] Bluetooth: btusb: request wake pin with NOAUTOEN

[Brian Norris]

On Tue, Apr 9, 2019 at 8:43 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> Either it's edge-triggered and you'll get one possibly spurious
> interrupt for an old issue, or it's level-triggered and setting up the
> hw should bring the irq line inactive and you'll be ok.

I think our key difference here is in how much we trust the device:
knowing the quality of the firmware running on some of these devices,
I wouldn't totally trust that they get it right. (It's not fatal if we
allow a buggy firmware to provide an occasional spurious wakeup, but
it's much less OK to allow it a window for flooding us with
interrupts.) So while your suggestion is indeed correct for
well-behaved devices, I think both approaches have value.

> But I've applied your patch for now simply because it seems to be a
> smaller change.

Awesome, thanks!

> But I think you should look into whether it can be fixed by just
> requesting the irq once the hardware is really up (which may indeed be
> as late as open time).

Yes, I will take a look at that for future release cycles. It's a nice
addition regardless, I think.

Brian

Re: [PULL -- 5.1 REGRESSION] Bluetooth: btusb: request wake pin with NOAUTOEN

[Linus Torvalds]

On Wed, Apr 10, 2019 at 7:44 AM Brian Norris <briannorris@chromium.org> wrote:
>
> I think our key difference here is in how much we trust the device:
> knowing the quality of the firmware running on some of these devices,
> I wouldn't totally trust that they get it right.

No.

You claim that IRQ_NOAUTOEN makes any difference, It doesn't.

I claim that you should get rid of the disable/enable_irq() games you
play, and replace them with just requesting the interrupt.

At which point the whole  IRQ_NOAUTOEN dance is entirely pointless.

Just don't do it.

This has nothing to do with trusting hardware, and everything to do
with "why do you request an interrupt that you aren't actually ready
to accept, and the hardware isn't even properly configured to generate
yet"?

See my point?

               Linus