LinuxLists.cc - [PATCH 1/3] Bluetooth: hci_bcm: Support Apple GPIO handling

[permalink] [raw]

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

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 <[email protected]> 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 <[email protected]> wrote:
> > >> > On Thu, 2017-12-28 at 13:29 +0100, Linus Walleij wrote:
> > >> >> On Thu, Dec 28, 2017 at 10:26 AM, Andy Shevchenko <[email protected]> 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

2017-12-29 15:18:41

[permalink] [raw]

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

On Fri, 2017-12-29 at 16:12 +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 <[email protected]> wrote:

> > 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.

I was about to propose to get rid of _optional there and thus depends on
GPIOLIB should work fine for !GPIOLIB case, right?

Otherwise GPIO library should be fixed to be transparent for !GPIOLIB
cases.

--
Andy Shevchenko <[email protected]>
Intel Finland Oy

2017-12-29 15:12:41

[permalink] [raw]

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

On Fri, Dec 29, 2017 at 03:18:44PM +0100, Loic Poulain wrote:
> On 29 December 2017 at 10:51, Lukas Wunner <[email protected]> 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 <[email protected]> wrote:
> >> > On Thu, 2017-12-28 at 13:29 +0100, Linus Walleij wrote:
> >> >> On Thu, Dec 28, 2017 at 10:26 AM, Andy Shevchenko <[email protected]> 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.

Thanks,

Lukas

2017-12-29 14:18:44

by Loic Poulain

[permalink] [raw]

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

On 29 December 2017 at 10:51, Lukas Wunner <[email protected]> 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 <[email protected]> wrote:
>> > On Thu, 2017-12-28 at 13:29 +0100, Linus Walleij wrote:
>> >> On Thu, Dec 28, 2017 at 10:26 AM, Andy Shevchenko <[email protected]> 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.

Regards,
Loic

2017-12-29 09:51:35

[permalink] [raw]

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

On Thu, Dec 28, 2017 at 01:45:34PM +0100, Linus Walleij wrote:
> On Thu, Dec 28, 2017 at 1:40 PM, Andy Shevchenko <[email protected]> wrote:
> > On Thu, 2017-12-28 at 13:29 +0100, Linus Walleij wrote:
> >> On Thu, Dec 28, 2017 at 10:26 AM, Andy Shevchenko <[email protected]> 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. What if there are boards where
the chip is off by default and must be powered on by the driver?
In that case the pins aren't optional and enabling GPIOLIB is required.

I guess this driver was never really tested without these pins present
as users would immediately get a NULL pointer deref on probe.

> And BT_HCIUART_NOKIA compiles drivers/bluetooth/hci_nokia.c
> which does depend on GPIOLIB.

Sorry, you weren't cc'ed on the original patch, I stated in the commit
message:

The same issue is present in hci_intel.c and hci_nokia.c, fix those up
as well.

These two use devm_gpiod_get(), so we agree that they need to depend
on GPIOLIB, right?

By the way, what is the rationale for this rule that consumers shall
depend on rather than select GPIOLIB? So that users are forced to
enable at least one GPIO driver? If that is the motivation, it's
not bullet-proof as users can still select GPIOLIB without selecting
a driver, or select an input-only GPIO driver even though the GPIOs
are used as outputs by hci_*.c.

Thanks,

Lukas

2017-12-28 12:45:34

by Linus Walleij

[permalink] [raw]

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

On Thu, Dec 28, 2017 at 1:40 PM, Andy Shevchenko
<[email protected]> wrote:
> On Thu, 2017-12-28 at 13:29 +0100, Linus Walleij wrote:
>> On Thu, Dec 28, 2017 at 10:26 AM, Andy Shevchenko
>> <[email protected]> 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().

I'm as confused as you are :D

The patch says:

> Loading hci_bcm with CONFIG_GPIOLIB=n results in the following splat
> when calling gpiod_to_irq() from bcm_get_resources():

Which leads to bcm and it is correct as you state it, but it patches:

> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 45a2f59cd935..41932f0e68d0 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -110,6 +110,7 @@ config BT_HCIUART_NOKIA
> depends on PM
> select BT_HCIUART_H4
> select BT_BCM
> + select GPIOLIB

And BT_HCIUART_NOKIA compiles drivers/bluetooth/hci_nokia.c
which does depend on GPIOLIB.

Apart from selectng BT_BCM.

So I suspect the descripton and/or patch is wrong...

Yours,
Linus Walleij

2017-12-28 12:40:58

[permalink] [raw]

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

On Thu, 2017-12-28 at 13:29 +0100, Linus Walleij wrote:
> On Thu, Dec 28, 2017 at 10:26 AM, Andy Shevchenko
> <[email protected]> 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().

>
> The only thing you achieve if you do not have a GPIOLIB is a driver
> that always exits probe with an error.

--
Andy Shevchenko <[email protected]>
Intel Finland Oy

2017-12-28 12:40:33

by Hans de Goede

[permalink] [raw]

Subject: Re: [PATCH 1/3] Bluetooth: hci_bcm: Support Apple GPIO handling

Hi,

On 28-12-17 08:15, Lukas Wunner wrote:
> On Tue, Dec 26, 2017 at 09:50:30PM +0100, Marcel Holtmann wrote:
>>> +}
>>> +
>>> +static int bcm_apple_set_power(struct bcm_device *dev, bool enable)
>>> +{
>>> + return ACPI_SUCCESS(acpi_evaluate_object(enable ? dev->btpu : dev->btpd,
>>> + NULL, NULL, NULL))
>>> + ? 0 : -EFAULT;
>>
>> Same here. Trying to mush everything in a single statement seems overkill.
>> And btw. why -EFAULT? Is that standard error practice with ACPI?
>
> -EFAULT was just chosen as a generic error because we don't really know
> what went wrong with the ACPI call. It seems there's no function to
> convert an ACPI exception code to an errno, only a function to convert it
> to a human-readable string, acpi_format_exception(). If you have a
> different preference than -EFAULT I'd be happy to change it.

In that case please use e.g. -EIO, or something else similar, EFAULT
has a very clear definition: userspace has called us with an invalid
address (which would cause a segfault when used from userspace code
directly), so please do not use it for anything else.

Regards,

Hans

2017-12-28 12:29:30

by Linus Walleij

[permalink] [raw]

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

On Thu, Dec 28, 2017 at 10:26 AM, Andy Shevchenko
<[email protected]> 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:
>> > > Loading hci_bcm with CONFIG_GPIOLIB=n results in the following
>> > > splat
>> > > when calling gpiod_to_irq() from bcm_get_resources():
>> > >
>> > > WARNING: CPU: 0 PID: 1006 at
>> > > ./include/linux/gpio/consumer.h:450
>> > > bcm_get_resources+0x50/0x80
>> > > CPU: 0 PID: 1006 Comm: kworker/u8:4 Tainted:
>> > > G A 4.15.0-rc4custom+ #4
>> > > Hardware name: Apple Inc. MacBook8,1/Mac-BE0E8AC46FE800CC,
>> > > BIOS
>> > > MB81.88Z.0168.B00.1708080033 08/08/2017
>> > > Call Trace:
>> > > bcm_serdev_probe+0x8b/0xc0
>> > > driver_probe_device+0x202/0x310
>> > > __driver_attach+0x85/0x90
>> > > ? driver_probe_device+0x310/0x310
>> > > bus_for_each_dev+0x57/0x80
>> > > async_run_entry_fn+0x2c/0xd0
>> > > process_one_work+0x1d2/0x3d0
>> > > worker_thread+0x26/0x3c0
>> > > ? process_one_work+0x3d0/0x3d0
>> > > kthread+0x10c/0x130
>> > > ? kthread_create_on_node+0x40/0x40
>> > > ret_from_fork+0x1f/0x30
>> > >
>> > > We could call gpiod_to_irq() only if IS_ENABLED(CONFIG_GPIOLIB)
>> > > but
>> > > without GPIOLIB, the driver's power saving features can't be used,
>> > > so selecting GPIOLIB seems more appropriate.
>> > >
>> > > The same issue is present in hci_intel.c and hci_nokia.c, fix
>> > > those up
>> > > as well.
>> > >
>
>> > > + select GPIOLIB
>> >
>> > This is wrong solution. GPIOLIB is meant for GPIO providers, not
>> > consumers.
>> >
>> > That's why after I did BT support for Intel MID (commit d4d96990)
>> > the necessity of exporting gpiod_add_lookup_table() had been arisen
>> > (commit 020e0b1c8f19f).
>>
>> 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().

The only thing you achieve if you do not have a GPIOLIB is a driver
that always exits probe with an error.

Yours,
Linus Walleij

2017-12-28 09:26:17

[permalink] [raw]

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

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:
> > > Loading hci_bcm with CONFIG_GPIOLIB=n results in the following
> > > splat
> > > when calling gpiod_to_irq() from bcm_get_resources():
> > >
> > > WARNING: CPU: 0 PID: 1006 at
> > > ./include/linux/gpio/consumer.h:450
> > > bcm_get_resources+0x50/0x80
> > > CPU: 0 PID: 1006 Comm: kworker/u8:4 Tainted:
> > > G A 4.15.0-rc4custom+ #4
> > > Hardware name: Apple Inc. MacBook8,1/Mac-BE0E8AC46FE800CC,
> > > BIOS
> > > MB81.88Z.0168.B00.1708080033 08/08/2017
> > > Call Trace:
> > > bcm_serdev_probe+0x8b/0xc0
> > > driver_probe_device+0x202/0x310
> > > __driver_attach+0x85/0x90
> > > ? driver_probe_device+0x310/0x310
> > > bus_for_each_dev+0x57/0x80
> > > async_run_entry_fn+0x2c/0xd0
> > > process_one_work+0x1d2/0x3d0
> > > worker_thread+0x26/0x3c0
> > > ? process_one_work+0x3d0/0x3d0
> > > kthread+0x10c/0x130
> > > ? kthread_create_on_node+0x40/0x40
> > > ret_from_fork+0x1f/0x30
> > >
> > > We could call gpiod_to_irq() only if IS_ENABLED(CONFIG_GPIOLIB)
> > > but
> > > without GPIOLIB, the driver's power saving features can't be used,
> > > so selecting GPIOLIB seems more appropriate.
> > >
> > > The same issue is present in hci_intel.c and hci_nokia.c, fix
> > > those up
> > > as well.
> > >

> > > + select GPIOLIB
> >
> > This is wrong solution. GPIOLIB is meant for GPIO providers, not
> > consumers.
> >
> > That's why after I did BT support for Intel MID (commit d4d96990)
> > the necessity of exporting gpiod_add_lookup_table() had been arisen
> > (commit 020e0b1c8f19f).
>
> 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.

--
Andy Shevchenko <[email protected]>
Intel Finland Oy

2017-12-28 09:18:05

[permalink] [raw]

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

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:
> > Loading hci_bcm with CONFIG_GPIOLIB=n results in the following splat
> > when calling gpiod_to_irq() from bcm_get_resources():
> >
> > WARNING: CPU: 0 PID: 1006 at ./include/linux/gpio/consumer.h:450
> > bcm_get_resources+0x50/0x80
> > CPU: 0 PID: 1006 Comm: kworker/u8:4 Tainted:
> > G A 4.15.0-rc4custom+ #4
> > Hardware name: Apple Inc. MacBook8,1/Mac-BE0E8AC46FE800CC, BIOS
> > MB81.88Z.0168.B00.1708080033 08/08/2017
> > Call Trace:
> > bcm_serdev_probe+0x8b/0xc0
> > driver_probe_device+0x202/0x310
> > __driver_attach+0x85/0x90
> > ? driver_probe_device+0x310/0x310
> > bus_for_each_dev+0x57/0x80
> > async_run_entry_fn+0x2c/0xd0
> > process_one_work+0x1d2/0x3d0
> > worker_thread+0x26/0x3c0
> > ? process_one_work+0x3d0/0x3d0
> > kthread+0x10c/0x130
> > ? kthread_create_on_node+0x40/0x40
> > ret_from_fork+0x1f/0x30
> >
> > We could call gpiod_to_irq() only if IS_ENABLED(CONFIG_GPIOLIB) but
> > without GPIOLIB, the driver's power saving features can't be used,
> > so selecting GPIOLIB seems more appropriate.
> >
> > The same issue is present in hci_intel.c and hci_nokia.c, fix those up
> > as well.
> >
> > Reported-by: Max Shavrick <[email protected]>
> > Signed-off-by: Lukas Wunner <[email protected]>
> > ---
> > drivers/bluetooth/Kconfig | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > index 45a2f59cd935..41932f0e68d0 100644
> > --- a/drivers/bluetooth/Kconfig
> > +++ b/drivers/bluetooth/Kconfig
> > @@ -110,6 +110,7 @@ config BT_HCIUART_NOKIA
> > depends on PM
> > select BT_HCIUART_H4
> > select BT_BCM
> > + select GPIOLIB
>
> This is wrong solution. GPIOLIB is meant for GPIO providers, not
> consumers.
>
> That's why after I did BT support for Intel MID (commit d4d96990)
> the necessity of exporting gpiod_add_lookup_table() had been arisen
> (commit 020e0b1c8f19f).

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?

Thanks,

Lukas

2017-12-28 08:41:17

[permalink] [raw]

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

On Tue, 2017-12-26 at 17:07 +0200, Lukas Wunner wrote:
> Loading hci_bcm with CONFIG_GPIOLIB=n results in the following splat
> when calling gpiod_to_irq() from bcm_get_resources():
>
> WARNING: CPU: 0 PID: 1006 at ./include/linux/gpio/consumer.h:450
> bcm_get_resources+0x50/0x80
> CPU: 0 PID: 1006 Comm: kworker/u8:4 Tainted:
> G A 4.15.0-rc4custom+ #4
> Hardware name: Apple Inc. MacBook8,1/Mac-BE0E8AC46FE800CC, BIOS
> MB81.88Z.0168.B00.1708080033 08/08/2017
> Call Trace:
> bcm_serdev_probe+0x8b/0xc0
> driver_probe_device+0x202/0x310
> __driver_attach+0x85/0x90
> ? driver_probe_device+0x310/0x310
> bus_for_each_dev+0x57/0x80
> async_run_entry_fn+0x2c/0xd0
> process_one_work+0x1d2/0x3d0
> worker_thread+0x26/0x3c0
> ? process_one_work+0x3d0/0x3d0
> kthread+0x10c/0x130
> ? kthread_create_on_node+0x40/0x40
> ret_from_fork+0x1f/0x30
>
> We could call gpiod_to_irq() only if IS_ENABLED(CONFIG_GPIOLIB) but
> without GPIOLIB, the driver's power saving features can't be used,
> so selecting GPIOLIB seems more appropriate.
>
> The same issue is present in hci_intel.c and hci_nokia.c, fix those up
> as well.
>
> Reported-by: Max Shavrick <[email protected]>
> Signed-off-by: Lukas Wunner <[email protected]>
> ---
> drivers/bluetooth/Kconfig | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 45a2f59cd935..41932f0e68d0 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -110,6 +110,7 @@ config BT_HCIUART_NOKIA
> depends on PM
> select BT_HCIUART_H4
> select BT_BCM
> + select GPIOLIB

This is wrong solution. GPIOLIB is meant for GPIO providers, not
consumers.

That's why after I did BT support for Intel MID (commit d4d96990)
the necessity of exporting gpiod_add_lookup_table() had been arisen
(commit 020e0b1c8f19f).

+Cc Linus W

--
Andy Shevchenko <[email protected]>
Intel Finland Oy

2017-12-28 07:38:36

[permalink] [raw]

Subject: Re: [PATCH 1/3] Bluetooth: hci_bcm: Support Apple GPIO handling

On Tue, 2017-12-26 at 21:50 +0100, Marcel Holtmann wrote:
> Hi Lukas,
>
> >

> > +
> > + if (!acpi_dev_get_property(adev, "baud", ACPI_TYPE_BUFFER,
> > &obj) &&
> > + obj->buffer.length == 8)
> > + dev->init_speed = *(u64 *)obj->buffer.pointer;
>
> if (!ACPI_SUCCESS(..))
> return -ENODEV;

ACPI_FAILURE()

--
Andy Shevchenko <[email protected]>
Intel Finland Oy

2017-12-28 07:15:51

[permalink] [raw]

Subject: Re: [PATCH 1/3] Bluetooth: hci_bcm: Support Apple GPIO handling

On Tue, Dec 26, 2017 at 09:50:30PM +0100, Marcel Holtmann wrote:
> > +}
> > +
> > +static int bcm_apple_set_power(struct bcm_device *dev, bool enable)
> > +{
> > + return ACPI_SUCCESS(acpi_evaluate_object(enable ? dev->btpu : dev->btpd,
> > + NULL, NULL, NULL))
> > + ? 0 : -EFAULT;
>
> Same here. Trying to mush everything in a single statement seems overkill.
> And btw. why -EFAULT? Is that standard error practice with ACPI?

-EFAULT was just chosen as a generic error because we don't really know
what went wrong with the ACPI call. It seems there's no function to
convert an ACPI exception code to an errno, only a function to convert it
to a human-readable string, acpi_format_exception(). If you have a
different preference than -EFAULT I'd be happy to change it.

D'accord on all your other comments, will respin in the coming days.

Thanks,

Lukas

2017-12-27 14:17:55

[permalink] [raw]

Subject: Re: [PATCH 1/3] Bluetooth: hci_bcm: Support Apple GPIO handling

Hi Frederic,

On Tue, Dec 26, 2017 at 09:50:30PM +0100, Marcel Holtmann wrote:
> > +static int bcm_gpio_set_device_wake(struct bcm_device *dev, bool awake)
> > +{
> > + int err = 0;
> > +
> > + if (x86_apple_machine)
> > + err = bcm_apple_set_low_power(dev, !awake);
> > + else if (dev->device_wakeup)
> > + gpiod_set_value(dev->device_wakeup, awake);
> > + else
> > + return 0;
> > +
> > + bt_dev_dbg(dev, "%s, delaying 15 ms", awake ? "resume" : "suspend");
> > + mdelay(15);
>
> Any chance for a comment here on why a delay of 15ms is needed?

The delay was introduced by your commit 118612fb9165 ("Bluetooth:
hci_bcm: Add suspend/resume PM functions").

Could you provide a rationale for it? Perhaps 15 ms is the time
after which the controller auto-suspends following deassertion of
the device wake pin (which forces it into "on" state AFAIUI)?

Is there public documentation on this controller? I've tried to
search for it but my google-fu is failing me.

Thanks,

Lukas

2017-12-26 20:57:25

[permalink] [raw]

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

Hi Lukas,

> Loading hci_bcm with CONFIG_GPIOLIB=n results in the following splat
> when calling gpiod_to_irq() from bcm_get_resources():
>
> WARNING: CPU: 0 PID: 1006 at ./include/linux/gpio/consumer.h:450 bcm_get_resources+0x50/0x80
> CPU: 0 PID: 1006 Comm: kworker/u8:4 Tainted: G A 4.15.0-rc4custom+ #4
> Hardware name: Apple Inc. MacBook8,1/Mac-BE0E8AC46FE800CC, BIOS MB81.88Z.0168.B00.1708080033 08/08/2017
> Call Trace:
> bcm_serdev_probe+0x8b/0xc0
> driver_probe_device+0x202/0x310
> __driver_attach+0x85/0x90
> ? driver_probe_device+0x310/0x310
> bus_for_each_dev+0x57/0x80
> async_run_entry_fn+0x2c/0xd0
> process_one_work+0x1d2/0x3d0
> worker_thread+0x26/0x3c0
> ? process_one_work+0x3d0/0x3d0
> kthread+0x10c/0x130
> ? kthread_create_on_node+0x40/0x40
> ret_from_fork+0x1f/0x30
>
> We could call gpiod_to_irq() only if IS_ENABLED(CONFIG_GPIOLIB) but
> without GPIOLIB, the driver's power saving features can't be used,
> so selecting GPIOLIB seems more appropriate.
>
> The same issue is present in hci_intel.c and hci_nokia.c, fix those up
> as well.
>
> Reported-by: Max Shavrick <[email protected]>
> Signed-off-by: Lukas Wunner <[email protected]>
> ---
> drivers/bluetooth/Kconfig | 3 +++
> 1 file changed, 3 insertions(+)

patch has been applied to bluetooth-next tree.

Regards

Marcel

2017-12-26 20:57:23

[permalink] [raw]

Subject: Re: [PATCH 2/3] Bluetooth: hci_bcm: Streamline runtime PM code

Hi Lukas,

> This driver seeks to force the Bluetooth device on for the duration of
> 5 seconds when the Bluetooth device has woken the host and after a
> complete packet has been received. It does that by calling:
>
> pm_runtime_get();
> pm_runtime_mark_last_busy();
> pm_runtime_put_autosuspend();
>
> The same can be achieved more succinctly with:
>
> pm_request_resume();
>
> That's because after runtime resuming the device, rpm_resume() invokes
> pm_runtime_mark_last_busy() followed by rpm_idle(), which will cause
> the device to be suspended after expiration of the autosuspend_delay.
>
> No functional change intended.
>
> Cc: Frédéric Danis <[email protected]>
> Signed-off-by: Lukas Wunner <[email protected]>
> ---
> drivers/bluetooth/hci_bcm.c | 11 +++--------
> 1 file changed, 3 insertions(+), 8 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

2017-12-26 20:50:30

[permalink] [raw]

Subject: Re: [PATCH 1/3] Bluetooth: hci_bcm: Support Apple GPIO handling

Hi Lukas,

> Enable Bluetooth on the following Macs which provide custom ACPI methods
> to toggle the GPIOs for device wakeup and shutdown instead of accessing
> the pins directly:
>
> MacBook8,1 2015 12"
> MacBook9,1 2016 12"
> MacBook10,1 2017 12"
> MacBookPro13,1 2016 13"
> MacBookPro13,2 2016 13" with Touch Bar
> MacBookPro13,3 2016 15" with Touch Bar
> MacBookPro14,1 2017 13"
> MacBookPro14,2 2017 13" with Touch Bar
> MacBookPro14,3 2017 15" with Touch Bar
>
> On the MacBook8,1 Bluetooth is muxed with a second device (a debug port
> on the SSD) under the control of PCH GPIO 36. Because serdev cannot
> deal with multiple slaves yet, it is currently necessary to patch the
> DSDT and remove the SSDC device.
>
> The custom ACPI methods are called:
>
> BTLP (Low Power) takes one argument, toggles device wakeup GPIO
> BTPU (Power Up) tells SMC to drive shutdown GPIO high
> BTPD (Power Down) tells SMC to drive shutdown GPIO low
> BTRS (Reset) calls BTPD followed by BTPU
> BTRB unknown, not present on all MacBooks
>
> Search for the BTLP, BTPU and BTPD methods on ->probe and cache them in
> struct bcm_device if the machine is a Mac.
>
> Additionally, set the init_speed based on a custom device property
> provided by Apple in lieu of _CRS resources. The Broadcom UART's speed
> is fixed on Apple Macs: Any attempt to change it results in Bluetooth
> status code 0x0c and bcm_set_baudrate() thus always returns -EBUSY.
> By setting only the init_speed and leaving oper_speed at zero, we can
> achieve that the host UART's speed is adjusted but the Broadcom UART's
> speed is left as is.

can we get a text version of the ACPI resources printout included in the commit messages. So that they are available to look up if anybody has question or new devices come along.

>
> The host wakeup pin goes into the SMC which handles it independently
> from the OS, so there's no IRQ for it. The ->close, ->suspend and
> ->resume hooks assume presence of a valid IRQ if the device is wakeup
> capable. That's a problem on Macs where the ACPI core marks the device
> wakeup capable due to presence of a _PRW method. (It specifies the
> SMC's GPE as wake event.) Amend the three hooks to not fiddle with the
> IRQ if it's invalid, i.e. <= 0.
>
> Runtime PM is currently set up in bcm_request_irq() even though it's
> independent of host wake. Move the function calls related to runtime PM
> to bcm_setup() so that they get executed even if setup of the IRQ is
> skipped.
>
> The existing code is a bit fishy as it ignores the return value of
> bcm_gpio_set_power(), even though portions of it may fail (such as
> enabling the clock). The present commit returns an error if calling
> the ACPI methods fails and the code needs to be fixed up in a separate
> commit to evaluate the return value of bcm_gpio_set_power() and
> bcm_gpio_set_device_wake(), as well as check for failure of clock
> enablement and so on.
>
> Thanks to Ronald Tschalär who did extensive debugging and testing of
> this patch and contributed fixes.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=110901
> Reported-by: Leif Liddy <[email protected]>
> Cc: Mika Westerberg <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: Frédéric Danis <[email protected]>
> Cc: Loic Poulain <[email protected]>
> Cc: Hans de Goede <[email protected]>
> Tested-by: Max Shavrick <[email protected]> [MacBook8,1]
> Tested-by: Leif Liddy <[email protected]> [MacBook9,1]
> Tested-by: Daniel Roschka <[email protected]> [MacBookPro13,2]
> Tested-by: Ronald Tschalär <[email protected]> [MacBookPro13,3]
> Tested-by: Peter Y. Chuang <[email protected]> [MacBookPro14,1]
> Signed-off-by: Ronald Tschalär <[email protected]>
> Signed-off-by: Lukas Wunner <[email protected]>
> ---
> drivers/bluetooth/hci_bcm.c | 101 +++++++++++++++++++++++++++++++++++---------
> 1 file changed, 82 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 47fc58c9eb49..a1e59fc4acbc 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -29,6 +29,7 @@
> #include <linux/acpi.h>
> #include <linux/of.h>
> #include <linux/property.h>
> +#include <linux/platform_data/x86/apple.h>
> #include <linux/platform_device.h>
> #include <linux/clk.h>
> #include <linux/gpio/consumer.h>
> @@ -76,6 +77,9 @@ struct bcm_device {
> struct hci_uart *hu;
> bool is_suspended; /* suspend/resume flag */
> #endif
> +#ifdef CONFIG_ACPI
> + acpi_handle btlp, btpu, btpd;
> +#endif
> };
>
> /* generic bcm uart resources */
> @@ -168,8 +172,47 @@ static bool bcm_device_exists(struct bcm_device *device)
> return false;
> }
>
> +#ifdef CONFIG_ACPI
> +static int bcm_apple_probe(struct bcm_device *dev)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(dev->dev);
> + acpi_handle dev_handle = adev->handle;
> + const union acpi_object *obj;
> +
> + if (!acpi_dev_get_property(adev, "baud", ACPI_TYPE_BUFFER, &obj) &&
> + obj->buffer.length == 8)
> + dev->init_speed = *(u64 *)obj->buffer.pointer;

if (!ACPI_SUCCESS(..))
return -ENODEV;

..

return 0;

> +
> + return ACPI_SUCCESS(acpi_get_handle(dev_handle, "BTLP", &dev->btlp)) &&
> + ACPI_SUCCESS(acpi_get_handle(dev_handle, "BTPU", &dev->btpu)) &&
> + ACPI_SUCCESS(acpi_get_handle(dev_handle, "BTPD", &dev->btpd))
> + ? 0 : -ENODEV;

I wonder if the above reads easier than just doing it line by line. We have enough lines to use, but squeezing it all in with a ? : operator seems not easy to read.

> +}
> +
> +static int bcm_apple_set_power(struct bcm_device *dev, bool enable)
> +{
> + return ACPI_SUCCESS(acpi_evaluate_object(enable ? dev->btpu : dev->btpd,
> + NULL, NULL, NULL))
> + ? 0 : -EFAULT;

Same here. Trying to mush everything in a single statement seems overkill. And btw. why -EFAULT? Is that standard error practice with ACPI?

> +}
> +
> +static int bcm_apple_set_low_power(struct bcm_device *dev, bool enable)
> +{
> + return ACPI_SUCCESS(acpi_execute_simple_method(dev->btlp, NULL, enable))
> + ? 0 : -EFAULT;
> +}
> +#else
> +static inline int bcm_apple_set_power(struct bcm_device *dev, bool powered)
> +{ return -EINVAL; }
> +static inline int bcm_apple_set_low_power(struct bcm_device *dev, bool enable)
> +{ return -EINVAL; }
> +#endif

Not -EOPNOTSUPP here. At least that is what we are doing within the btbcm.h. I just like to hear an opinion on why -EINVAL.

> +
> static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
> {
> + if (x86_apple_machine)
> + return bcm_apple_set_power(dev, powered);
> +
> if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled)
> clk_prepare_enable(dev->clk);
>
> @@ -184,6 +227,23 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
> return 0;
> }
>
> +static int bcm_gpio_set_device_wake(struct bcm_device *dev, bool awake)
> +{
> + int err = 0;
> +
> + if (x86_apple_machine)
> + err = bcm_apple_set_low_power(dev, !awake);
> + else if (dev->device_wakeup)
> + gpiod_set_value(dev->device_wakeup, awake);
> + else
> + return 0;
> +
> + bt_dev_dbg(dev, "%s, delaying 15 ms", awake ? "resume" : "suspend");
> + mdelay(15);

Any chance for a comment here on why a delay of 15ms is needed?

> +
> + return err;
> +}
> +
> #ifdef CONFIG_PM
> static irqreturn_t bcm_host_wake(int irq, void *data)
> {
> @@ -209,6 +269,15 @@ static int bcm_request_irq(struct bcm_data *bcm)
> goto unlock;
> }
>
> + /*
> + * Macs wire the host wake pin to the System Management Controller,
> + * which handles wakeup independently from the operating system.
> + */

Network subsystem comment style please.

> + if (x86_apple_machine) {
> + err = 0;
> + goto unlock;
> + }
> +
> if (bdev->irq <= 0) {
> err = -EOPNOTSUPP;
> goto unlock;
> @@ -223,12 +292,6 @@ static int bcm_request_irq(struct bcm_data *bcm)
>
> device_init_wakeup(bdev->dev, true);
>
> - pm_runtime_set_autosuspend_delay(bdev->dev,
> - BCM_AUTOSUSPEND_DELAY);
> - pm_runtime_use_autosuspend(bdev->dev);
> - pm_runtime_set_active(bdev->dev);
> - pm_runtime_enable(bdev->dev);
> -
> unlock:
> mutex_unlock(&bcm_device_lock);
>
> @@ -379,7 +442,7 @@ static int bcm_close(struct hci_uart *hu)
> pm_runtime_disable(bdev->dev);
> pm_runtime_set_suspended(bdev->dev);
>
> - if (device_can_wakeup(bdev->dev)) {
> + if (bdev->irq > 0) {
> devm_free_irq(bdev->dev, bdev->irq, bdev);
> device_init_wakeup(bdev->dev, false);
> }
> @@ -470,6 +533,11 @@ static int bcm_setup(struct hci_uart *hu)
> if (!bcm_request_irq(bcm))
> err = bcm_setup_sleep(hu);
>
> + pm_runtime_set_autosuspend_delay(bcm->dev->dev, BCM_AUTOSUSPEND_DELAY);
> + pm_runtime_use_autosuspend(bcm->dev->dev);
> + pm_runtime_set_active(bcm->dev->dev);
> + pm_runtime_enable(bcm->dev->dev);
> +

Is this block really part of this patch or should it be done as a pre-patch with the other patch you have?

> return err;
> }
>
> @@ -577,11 +645,7 @@ static int bcm_suspend_device(struct device *dev)
> }
>
> /* Suspend the device */
> - if (bdev->device_wakeup) {
> - gpiod_set_value(bdev->device_wakeup, false);
> - bt_dev_dbg(bdev, "suspend, delaying 15 ms");
> - mdelay(15);
> - }
> + bcm_gpio_set_device_wake(bdev, false);

Seems unrelated and can be done in an initial cleanup patch?

>
> return 0;
> }
> @@ -592,11 +656,7 @@ static int bcm_resume_device(struct device *dev)
>
> bt_dev_dbg(bdev, "");
>
> - if (bdev->device_wakeup) {
> - gpiod_set_value(bdev->device_wakeup, true);
> - bt_dev_dbg(bdev, "resume, delaying 15 ms");
> - mdelay(15);
> - }
> + bcm_gpio_set_device_wake(bdev, true);

Same as above.

>
> /* When this executes, the device has woken up already */
> if (bdev->is_suspended && bdev->hu) {
> @@ -632,7 +692,7 @@ static int bcm_suspend(struct device *dev)
> if (pm_runtime_active(dev))
> bcm_suspend_device(dev);
>
> - if (device_may_wakeup(dev)) {
> + if (device_may_wakeup(dev) && bdev->irq > 0) {
> error = enable_irq_wake(bdev->irq);
> if (!error)
> bt_dev_dbg(bdev, "BCM irq: enabled");
> @@ -662,7 +722,7 @@ static int bcm_resume(struct device *dev)
> if (!bdev->hu)
> goto unlock;
>
> - if (device_may_wakeup(dev)) {
> + if (device_may_wakeup(dev) && bdev->irq > 0) {
> disable_irq_wake(bdev->irq);
> bt_dev_dbg(bdev, "BCM irq: disabled");
> }
> @@ -816,6 +876,9 @@ static int bcm_acpi_probe(struct bcm_device *dev)
> struct resource_entry *entry;
> int ret;
>
> + if (x86_apple_machine)
> + return bcm_apple_probe(dev);
> +
> /* Retrieve GPIO data */
> id = acpi_match_device(dev->dev->driver->acpi_match_table, dev->dev);
> if (id)

Regards

Marcel

2017-12-26 17:08:15

[permalink] [raw]

Subject: Re: [PATCH 1/3] Bluetooth: hci_bcm: Support Apple GPIO handling

Hi Ulrich,

On Tue, Dec 26, 2017 at 05:07:34PM +0200, Lukas Wunner wrote:
> On the MacBook8,1 Bluetooth is muxed with a second device (a debug port
> on the SSD) under the control of PCH GPIO 36. Because serdev cannot
> deal with multiple slaves yet, it is currently necessary to patch the
> DSDT and remove the SSDC device.

Last August you posted a series to add multiplexer support for serdev:

https://www.spinics.net/lists/linux-serial/msg27329.html

Another use case has now popped up where a GPIO-controlled mux is used
to switch between two serdev slaves: As mentioned in a series I've just
posted to linux-bluetooth@ (quoted above), on the MacBook 8,1 the UART
is attached to a serial debug port on the SSD as well as to a Bluetooth
controller, subject to a mux controlled by PCH GPIO 36.

The SSD debug ported is listed first in the ACPI namespace and is attached
as a serdev slave without a hitch (see DSDT excerpt included below).
The Bluetooth controller comes next and fails to register due to a sanity
check in serdev_device_add():

[ 0.361650] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
[ 0.362633] 0000:00:15.5: ttyS0 at MMIO 0xc1819000 (irq = 21, base_baud = 2764800) is a 16550A
[ 0.362769] serial serial0-0: controller busy
[ 0.362815] serial serial0-0: failure adding ACPI serdev device. status -16
[ 0.362860] serial serial0: tty port ttyS0 registered

The series you've posted in August hasn't been merged. Taking a closer
look at patch [2/6] (linked above), you've put the calls to lock and
unlock the mux in the slave, i.e. max9260.c. This works because in your
use case, the UART is shared only by MAX9260 chips. I think a more
generic solution is called for which puts the locking of the mux in the
serdev controller and makes it fully transparent to the slaves.

So whenever data is read or written to the UART's FIFO or whenever
the modem control lines are accessed, the mux needs to be locked to
the respective slave first.

The sanity check introduced by 08fcee289f34 to prevent registration
of multiple serdev slaves needs to be amended to check for presence
of a mux if more than one slave registers, and check whether the
number of switch states is sufficient to accomodate the additional
slave.

If you do find the time to amend the series in this way, it would
result in something truly useful that we could also employ to solve
Bluetooth muxing on the MacBook8,1 in a clean way.

Thanks,

Lukas

-- cut here --

Device (URT0)
{
Name (_ADR, 0x00150005) // _ADR: Address
Name (RBUF, ResourceTemplate ()
{
Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, )
{
0x00000015,
}
})
Method (_STA, 0, NotSerialized) // _STA: Status
{
Return (0x0F)
}

Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method
{
If (LEqual (Arg0, ToUUID ("a0b5b7c6-1318-441c-b0c9-fe695eaf949b")))
{
Store (Package (0x02)
{
"uart-channel-number",
Buffer (0x08)
{
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 /* ........ */
}
}, Local0)
DTGP (Arg0, Arg1, Arg2, Arg3, RefOf (Local0))
Return (Local0)
}

Return (0x00)
}

Name (DBUF, ResourceTemplate ()
{
FixedDMA (0x0014, 0x0004, Width32bit, )
FixedDMA (0x0015, 0x0005, Width32bit, )
})
Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
{
Return (ConcatenateResTemplate (RBUF, DBUF))
}
}

Scope (\_SB.PCI0.URT0)
{
Device (SSDC)
{
Name (_CID, "apple-uart-ssdc") // _CID: Compatible ID
Name (_UID, 0x01) // _UID: Unique ID
Name (_ADR, 0x00) // _ADR: Address
Method (_STA, 0, NotSerialized) // _STA: Status
{
Return (0x0F)
}

Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method
{
If (LEqual (Arg0, ToUUID ("a0b5b7c6-1318-441c-b0c9-fe695eaf949b")))
{
Store (Package (0x08)
{
"baud",
Buffer (0x08)
{
0xD0, 0x12, 0x13, 0x00, 0x00, 0x00, 0x00, 0x00 /* ........ */
},

"parity",
Buffer (0x08)
{
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 /* ........ */
},

"dataBits",
Buffer (0x08)
{
0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 /* ........ */
},

"stopBits",
Buffer (0x08)
{
0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 /* ........ */
}
}, Local0)
DTGP (Arg0, Arg1, Arg2, Arg3, RefOf (Local0))
Return (Local0)
}

Return (0x00)
}
}
}

Scope (\_SB.PCI0.URT0)
{
Device (BLTH)
{
Name (_HID, EisaId ("BCM2E7C")) // _HID: Hardware ID
Name (_CID, "apple-uart-blth") // _CID: Compatible ID
Name (_UID, 0x01) // _UID: Unique ID
Name (_ADR, 0x00) // _ADR: Address
Method (_STA, 0, NotSerialized) // _STA: Status
{
Return (0x0F)
}

Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
{
Name (UBUF, ResourceTemplate ()
{
UartSerialBus (0x0001C200, DataBitsEight, StopBitsOne,
0xC0, LittleEndian, ParityTypeNone, FlowControlHardware,
0x0020, 0x0020, "\\_SB.PCI0.URT0",
0x00, ResourceProducer, ,
)
})
Name (ABUF, ResourceTemplate ()
{
})
If (LNot (OSDW ()))
{
Return (UBUF) /* \_SB_.PCI0.URT0.BLTH._CRS.UBUF */
}

Return (ABUF) /* \_SB_.PCI0.URT0.BLTH._CRS.ABUF */
}

Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method
{
If (LEqual (Arg0, ToUUID ("a0b5b7c6-1318-441c-b0c9-fe695eaf949b")))
{
Store (Package (0x08)
{
"baud",
Buffer (0x08)
{
0xC0, 0xC6, 0x2D, 0x00, 0x00, 0x00, 0x00, 0x00 /* ..-..... */
},

"parity",
Buffer (0x08)
{
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 /* ........ */
},

"dataBits",
Buffer (0x08)
{
0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 /* ........ */
},

"stopBits",
Buffer (0x08)
{
0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 /* ........ */
}
}, Local0)
DTGP (Arg0, Arg1, Arg2, Arg3, RefOf (Local0))
Return (Local0)
}

Return (0x00)
}

Method (BTPU, 0, Serialized)
{
Store (0x01, \_SB.PCI0.LPCB.EC.BTPC)
Sleep (0x0A)
}

Method (BTPD, 0, Serialized)
{
Store (0x00, \_SB.PCI0.LPCB.EC.BTPC)
Sleep (0x0A)
}

Method (BTRS, 0, Serialized)
{
BTPD ()
BTPU ()
}

Method (BTLP, 1, Serialized)
{
If (LEqual (Arg0, 0x00))
{
Store (0x01, GD54) /* \GD54 */
}

If (LEqual (Arg0, 0x01))
{
Store (0x00, GD54) /* \GD54 */
Store (0x00, GP54) /* \GP54 */
}
}
}
}
}

2017-12-26 15:07:34

[permalink] [raw]

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

Loading hci_bcm with CONFIG_GPIOLIB=n results in the following splat
when calling gpiod_to_irq() from bcm_get_resources():

WARNING: CPU: 0 PID: 1006 at ./include/linux/gpio/consumer.h:450 bcm_get_resources+0x50/0x80
CPU: 0 PID: 1006 Comm: kworker/u8:4 Tainted: G A 4.15.0-rc4custom+ #4
Hardware name: Apple Inc. MacBook8,1/Mac-BE0E8AC46FE800CC, BIOS MB81.88Z.0168.B00.1708080033 08/08/2017
Call Trace:
bcm_serdev_probe+0x8b/0xc0
driver_probe_device+0x202/0x310
__driver_attach+0x85/0x90
? driver_probe_device+0x310/0x310
bus_for_each_dev+0x57/0x80
async_run_entry_fn+0x2c/0xd0
process_one_work+0x1d2/0x3d0
worker_thread+0x26/0x3c0
? process_one_work+0x3d0/0x3d0
kthread+0x10c/0x130
? kthread_create_on_node+0x40/0x40
ret_from_fork+0x1f/0x30

We could call gpiod_to_irq() only if IS_ENABLED(CONFIG_GPIOLIB) but
without GPIOLIB, the driver's power saving features can't be used,
so selecting GPIOLIB seems more appropriate.

The same issue is present in hci_intel.c and hci_nokia.c, fix those up
as well.

Reported-by: Max Shavrick <[email protected]>
Signed-off-by: Lukas Wunner <[email protected]>
---
drivers/bluetooth/Kconfig | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index 45a2f59cd935..41932f0e68d0 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -110,6 +110,7 @@ config BT_HCIUART_NOKIA
depends on PM
select BT_HCIUART_H4
select BT_BCM
+ select GPIOLIB
help
Nokia H4+ is serial protocol for communication between Bluetooth
device and host. This protocol is required for Bluetooth devices
@@ -170,6 +171,7 @@ config BT_HCIUART_INTEL
depends on BT_HCIUART
select BT_HCIUART_H4
select BT_INTEL
+ select GPIOLIB
help
The Intel protocol support enables Bluetooth HCI over serial
port interface for Intel Bluetooth controllers.
@@ -183,6 +185,7 @@ config BT_HCIUART_BCM
depends on (!ACPI || SERIAL_DEV_CTRL_TTYPORT)
select BT_HCIUART_H4
select BT_BCM
+ select GPIOLIB
help
The Broadcom protocol support enables Bluetooth HCI over serial
port interface for Broadcom Bluetooth controllers.
--
2.15.1

2017-12-26 15:07:34

[permalink] [raw]

Subject: [PATCH 2/3] Bluetooth: hci_bcm: Streamline runtime PM code

This driver seeks to force the Bluetooth device on for the duration of
5 seconds when the Bluetooth device has woken the host and after a
complete packet has been received. It does that by calling:

pm_runtime_get();
pm_runtime_mark_last_busy();
pm_runtime_put_autosuspend();

The same can be achieved more succinctly with:

pm_request_resume();

That's because after runtime resuming the device, rpm_resume() invokes
pm_runtime_mark_last_busy() followed by rpm_idle(), which will cause
the device to be suspended after expiration of the autosuspend_delay.

No functional change intended.

Cc: Frédéric Danis <[email protected]>
Signed-off-by: Lukas Wunner <[email protected]>
---
drivers/bluetooth/hci_bcm.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index a1e59fc4acbc..539ba1c8615c 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -251,9 +251,7 @@ static irqreturn_t bcm_host_wake(int irq, void *data)

bt_dev_dbg(bdev, "Host wake IRQ");

- pm_runtime_get(bdev->dev);
- pm_runtime_mark_last_busy(bdev->dev);
- pm_runtime_put_autosuspend(bdev->dev);
+ pm_request_resume(bdev->dev);

return IRQ_HANDLED;
}
@@ -580,11 +578,8 @@ static int bcm_recv(struct hci_uart *hu, const void *data, int count)
} else if (!bcm->rx_skb) {
/* Delay auto-suspend when receiving completed packet */
mutex_lock(&bcm_device_lock);
- if (bcm->dev && bcm_device_exists(bcm->dev)) {
- pm_runtime_get(bcm->dev->dev);
- pm_runtime_mark_last_busy(bcm->dev->dev);
- pm_runtime_put_autosuspend(bcm->dev->dev);
- }
+ if (bcm->dev && bcm_device_exists(bcm->dev))
+ pm_request_resume(bcm->dev->dev);
mutex_unlock(&bcm_device_lock);
}

--
2.15.1

2018-01-02 17:10:05

[permalink] [raw]

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

Hi Lukas,

>>>> By the way, what is the rationale for this rule that consumers shall
>>>> depend on rather than select GPIOLIB?
>>>
>>> I think consumers should depend on GPIOLIB and producers
>>> should select GPIOLIB.
>>
>> so what do we do here? Should I revert the patch or can I get an updated
>> patch that uses the agreed upon select vs depends.
>
> I've prepared this branch which still needs to be tested & debugged,
> it contains a patch to change the "select" to a "depends on":
>
> https://github.com/l1k/linux/commits/hci_bcm_v2/
> https://github.com/l1k/linux/commit/71aa06c610d0
>
> If you prefer reverting 27378f4c1b92 or rebase your branch and drop it,
> I'll be happy to provide a rectified version instead (basically
> 27378f4c1b92 and 71aa06c610d0 squashed together). I was assuming that
> bluetooth-next/master is a non-rebasing branch, hence prepared
> 71aa06c610d0 as a fix of the existing commit.

I will revert and rebase if I have to, but if we can just get a fix applied on top of it, then that works for me as well.

Regards

Marcel

2018-01-02 16:58:29

[permalink] [raw]

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

On Tue, Jan 02, 2018 at 04:27:02PM +0100, Marcel Holtmann wrote:
> >> By the way, what is the rationale for this rule that consumers shall
> >> depend on rather than select GPIOLIB?
> >
> > I think consumers should depend on GPIOLIB and producers
> > should select GPIOLIB.
>
> so what do we do here? Should I revert the patch or can I get an updated
> patch that uses the agreed upon select vs depends.

I've prepared this branch which still needs to be tested & debugged,
it contains a patch to change the "select" to a "depends on":

https://github.com/l1k/linux/commits/hci_bcm_v2/
https://github.com/l1k/linux/commit/71aa06c610d0

If you prefer reverting 27378f4c1b92 or rebase your branch and drop it,
I'll be happy to provide a rectified version instead (basically
27378f4c1b92 and 71aa06c610d0 squashed together). I was assuming that
bluetooth-next/master is a non-rebasing branch, hence prepared
71aa06c610d0 as a fix of the existing commit.

Thanks,

Lukas

2018-01-02 15:27:02