Re: [PATCH 3/3] Bluetooth: Avoid WARN splat due to missing GPIOLIB

[Lukas Wunner <lukas@wunner.de>]

On Fri, Dec 29, 2017 at 04:12:41PM +0100, Lukas Wunner wrote:
> On Fri, Dec 29, 2017 at 03:18:44PM +0100, Loic Poulain wrote:
> > On 29 December 2017 at 10:51, Lukas Wunner <lukas@wunner.de> wrote:
> > > On Thu, Dec 28, 2017 at 01:45:34PM +0100, Linus Walleij wrote:
> > >> On Thu, Dec 28, 2017 at 1:40 PM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > >> > On Thu, 2017-12-28 at 13:29 +0100, Linus Walleij wrote:
> > >> >> On Thu, Dec 28, 2017 at 10:26 AM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > >> >> > On Thu, 2017-12-28 at 10:18 +0100, Lukas Wunner wrote:
> > >> >> > > On Thu, Dec 28, 2017 at 10:41:17AM +0200, Andy Shevchenko wrote:
> > >> >> > > > On Tue, 2017-12-26 at 17:07 +0200, Lukas Wunner wrote:
> > >> >> > > Hm okay, Documentation/gpio/consumer.txt says:
> > >> >> > >
> > >> >> > >     Guidelines for GPIOs consumers
> > >> >> > >     ==============================
> > >> >> > >
> > >> >> > >     Drivers that can't work without standard GPIO calls should have
> > >> >> > >     Kconfig entries that depend on GPIOLIB.
> > >> >> > >
> > >> >> > > So a "depends on GPIOLIB" would be more appropriate, right?
> > >> >> >
> > >> >> > Yes, but still wrong for this certain driver. It *can* work w/o
> > >> >> > GPIOLIB.
> > >> >> > Now you have done unnecessary dependency for that case.
> > >> >>
> > >> >> No I think it should use depends on GPIOLIB.
> > >> >>
> > >> >> The reason is that the driver uses unconditional devm_gpiod_get(),
> > >> >> not devm_gpiod_get_optional().
> > >> >
> > >> > How come?
> > >> > I just checked the code, all three use _optional() variant.
> > >> >
> > >> > I checked in bcm_get_resources().
> > >
> > > Even though hci_bcm.c uses devm_gpiod_get_optional() for the device
> > > wakeup and shutdown pins, it calls gpiod_set_value() on both pins
> > > without checking if the're NULL in bcm_gpio_set_power().
> > >
> > > It also calls gpiod_to_irq() on the host wakeup pin without checking
> > > if it's NULL in bcm_get_resources(), which results in a WARN splat
> > > if GPIOLIB is not enabled.
> > >
> > > So this is clearly wrong.  The problem is, I don't have this hardware
> > > to test myself, I don't have a spec for the chip and I don't know
> > > what the driver author's intention was.  Perhaps these are just glitches
> > > that snuck in when power management was retrofitted into the driver
> > > and we can fix them with a few NULL pointer checks.  But I'm not sure
> > > if these pins are really optional.
> > 
> > I think this is due to the adaptation to serdev bus support, originally a
> > platform device was only added to describe power control resources
> > (via ACPI/DT), there was no associated pdev for non 'gpio-controllable'
> > devices and so no gpio action. Now that serdev is supported I agree
> > that some pointer checks should be added.
> 
> You're correct that GPIO use was originally mandatory in this driver,
> but serdev has nothing to do with it becoming optional.
> 
> Rather, commit 62aaefa7d038 ("Bluetooth: hci_bcm: improve use of gpios
> API") added the _optional "to simplify error handling".
> 
> So the _optional is a red herring and GPIO use is not optional at all
> in this driver.  Adding Uwe Kleine-König to cc.

Oh I guess you mean this part:

	/* Make sure at-least one of the GPIO is defined and that
	 * a name is specified for this instance
	 */
	if ((!dev->device_wakeup && !dev->shutdown) || !dev->name) {
		dev_err(dev->dev, "invalid platform data\n");
		return -EINVAL;
	}

Indeed this was removed by Hans with 8a92056837fd ("Bluetooth: hci_bcm:
Add (runtime)pm support to the serdev driver").

Hans, was this intentional?

BTW why was only one of the two pins required to be non-NULL?  Since
*both* pins are unconditionally accessed in bcm_gpio_set_power(),
which is called from the ->probe hook, we'd still get a NULL pointer
deref.  And this has been present ever since power management was
introduced with 0395ffc1ee05 ("Bluetooth: hci_bcm: Add PM for BCM
devices").

Adding Uwe to cc for real now, sorry.

Thanks,

Lukas