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

From: Brian Norris <briannorris@chromium.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Marcel Holtmann <marcel@holtmann.org>,
	Johan Hedberg <johan.hedberg@gmail.com>,
	linux-bluetooth <linux-bluetooth@vger.kernel.org>,
	Linux List Kernel Mailing <linux-kernel@vger.kernel.org>,
	Matthias Kaehlcke <mka@chromium.org>,
	Rajat Jain <rajatja@google.com>, Heiko Stuebner <heiko@sntech.de>
Subject: Re: [PULL -- 5.1 REGRESSION] Bluetooth: btusb: request wake pin with NOAUTOEN
Date: Tue, 9 Apr 2019 20:26:43 -0700	[thread overview]
Message-ID: <CA+ASDXMptGi2H+E77xRJQEryWgEeqWYb8Xn=XxkK8Tio8RBkMw@mail.gmail.com> (raw)
In-Reply-To: <CAHk-=wg6_i9WcqiGu4kg97yAg1du26RpXdxd=uEjo4yXjotdiQ@mail.gmail.com>

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