[PATCH 1/2] Bluetooth: hci_bcm: Don't change runtime-status when not using runtime-pm

[PATCH 1/2] Bluetooth: hci_bcm: Don't change runtime-status when not using runtime-pm

[Hans de Goede]

The pm_runtime_set_suspended() call in bcm_close() is intended as a
counterpart to the pm_runtime_set_active() call from bcm_request_irq()
(which gets called from bcm_setup()).

The pm_runtime_set_active() call only happens if our pre-requisites for
doing runtime-pm are met. This commit adds the same check to the
pm_runtime_set_suspended() call to avoid the runtime-pm state getting
stuck in suspended after after a bcm_close().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/hci_bcm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index eee33b0e4d67..06e2434c9b3d 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -464,7 +464,7 @@ static int bcm_close(struct hci_uart *hu)
 		err = bcm_gpio_set_power(bdev, false);
 		if (err)
 			bt_dev_err(hu->hdev, "Failed to power down");
-		else
+		else if (IS_ENABLED(CONFIG_PM) && bdev->irq > 0)
 			pm_runtime_set_suspended(bdev->dev);
 	}
 	mutex_unlock(&bcm_device_lock);
-- 
2.14.3

[PATCH 2/2] Bluetooth: hci_bcm: Add broken-irq dmi blacklist and add Meegopad T08 to it

[Hans de Goede]

The Meegopad T08 hdmi-stick (think Intel computestick) has a brcm43430
wifi/bt combo chip. The BCM2E90 ACPI device describing the BT part does
contain a valid ActiveLow GpioInt entry, but the GPIO it points to never
goes low, so either the IRQ pin is not connected, or the ACPI resource-
table points to the wrong GPIO.

Eitherway things will not work if we try to use the specified IRQ, this
commits adds a DMI based broken-irq blacklist and disables use of the IRQ
and thus also runtime-pm for devices on this list.

This blacklist starts with the the Meegopad T08, fixing bluetooth not
working on this hdmi-stick. Since this is not a battery powered device
the loss of runtime-pm is not really an issue.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/hci_bcm.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 06e2434c9b3d..fba43902af83 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -794,6 +794,20 @@ static const struct acpi_gpio_mapping acpi_bcm_int_first_gpios[] = {
 	{ },
 };
 
+/* Some firmware reports an IRQ which does not work (wrong pin in fw table?) */
+static const struct dmi_system_id bcm_broken_irq_dmi_table[] = {
+	{
+		.ident = "Meegopad T08",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
+					"To be filled by OEM."),
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "T3 MRD"),
+			DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V1.1"),
+		},
+	},
+	{ }
+};
+
 #ifdef CONFIG_ACPI
 /* IRQ polarity of some chipsets are not defined correctly in ACPI table. */
 static const struct dmi_system_id bcm_active_low_irq_dmi_table[] = {
@@ -911,6 +925,8 @@ static int bcm_gpio_set_shutdown(struct bcm_device *dev, bool powered)
 
 static int bcm_get_resources(struct bcm_device *dev)
 {
+	const struct dmi_system_id *dmi_id;
+
 	dev->name = dev_name(dev->dev);
 
 	if (x86_apple_machine && !bcm_apple_get_resources(dev))
@@ -943,6 +959,13 @@ static int bcm_get_resources(struct bcm_device *dev)
 		dev->irq = gpiod_to_irq(gpio);
 	}
 
+	dmi_id = dmi_first_match(bcm_broken_irq_dmi_table);
+	if (dmi_id) {
+		dev_info(dev->dev, "%s: Has a broken IRQ config, disabling IRQ support / runtime-pm\n",
+			 dmi_id->ident);
+		dev->irq = 0;
+	}
+
 	dev_dbg(dev->dev, "BCM irq: %d\n", dev->irq);
 	return 0;
 }
-- 
2.14.3

Re: [PATCH 1/2] Bluetooth: hci_bcm: Don't change runtime-status when not using runtime-pm

[Lukas Wunner]

On Mon, Mar 19, 2018 at 10:02:36PM +0100, Hans de Goede wrote:
> The pm_runtime_set_suspended() call in bcm_close() is intended as a
> counterpart to the pm_runtime_set_active() call from bcm_request_irq()
> (which gets called from bcm_setup()).
> 
> The pm_runtime_set_active() call only happens if our pre-requisites for
> doing runtime-pm are met. This commit adds the same check to the
> pm_runtime_set_suspended() call to avoid the runtime-pm state getting
> stuck in suspended after after a bcm_close().

Have you actually observed the state getting stuck in suspended?

I'm asking because I don't quite see how this could happen:
pm_runtime_set_suspended() becomes a no-op unless runtime PM is disabled.
Now, a few lines further up in bcm_close(), pm_runtime_disable() is only
called under the conditions you're adding here.  If it's not called,
pm_runtime_set_suspended() doesn't have any effect anyway so adding the
conditions is pointless.  Am I missing something?

Related:  I notice that bcm_resume() calls pm_runtime_set_active().
Doesn't that have the effect that if the device is not opened and
the system goes to sleep, the device is kept in runtime active state
after resume and thus prevents the UART from runtime suspending until
the device is opened?

Another thing that I find utterly confusing:  We set up the sleep
parameters on ->setup if and only if the device has an IRQ.
The sleep parameters influence how the wake pin is used (polarity etc).
However we toggle the pin *before* setting up the sleep parameters,
namely by calling bcm_gpio_set_power() in bcm_open(), bcm_probe() and
bcm_serdev_probe().  Is that actually correct?  Shouldn't we avoid
toggling the wake pin until the sleep parameters have been set up?
Would it be conceivable that the sleep parameters are somehow botched
when the device comes out of power-on reset, such that toggling the
wake pin before setting the sleep parameters to sane values causes
the device to become unresponsive or something like that?

Thanks,

Lukas

> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/bluetooth/hci_bcm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index eee33b0e4d67..06e2434c9b3d 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -464,7 +464,7 @@ static int bcm_close(struct hci_uart *hu)
>  		err = bcm_gpio_set_power(bdev, false);
>  		if (err)
>  			bt_dev_err(hu->hdev, "Failed to power down");
> -		else
> +		else if (IS_ENABLED(CONFIG_PM) && bdev->irq > 0)
>  			pm_runtime_set_suspended(bdev->dev);
>  	}
>  	mutex_unlock(&bcm_device_lock);
> -- 
> 2.14.3
> 

Re: [PATCH 1/2] Bluetooth: hci_bcm: Don't change runtime-status when not using runtime-pm

[Hans de Goede]

Hi,

On 24-03-18 09:02, Lukas Wunner wrote:
> On Mon, Mar 19, 2018 at 10:02:36PM +0100, Hans de Goede wrote:
>> The pm_runtime_set_suspended() call in bcm_close() is intended as a
>> counterpart to the pm_runtime_set_active() call from bcm_request_irq()
>> (which gets called from bcm_setup()).
>>
>> The pm_runtime_set_active() call only happens if our pre-requisites for
>> doing runtime-pm are met. This commit adds the same check to the
>> pm_runtime_set_suspended() call to avoid the runtime-pm state getting
>> stuck in suspended after after a bcm_close().
> 
> Have you actually observed the state getting stuck in suspended?

No, that would require an open / close / open cycle which normally
never happens, this patch is purely for correctness sake not to
fix an actual observed problem.

> I'm asking because I don't quite see how this could happen:
> pm_runtime_set_suspended() becomes a no-op unless runtime PM is disabled.
> Now, a few lines further up in bcm_close(), pm_runtime_disable() is only
> called under the conditions you're adding here.  If it's not called,
> pm_runtime_set_suspended() doesn't have any effect anyway so adding the
> conditions is pointless.  Am I missing something?

All devices start with runtime-pm disabled and on devices without
an IRQ we never enable it. So when the pm_runtime_set_suspended()
gets called runtime-pm is still in its initial disabled state.

> Related:  I notice that bcm_resume() calls pm_runtime_set_active().
> Doesn't that have the effect that if the device is not opened and
> the system goes to sleep, the device is kept in runtime active state
> after resume and thus prevents the UART from runtime suspending until
> the device is opened?

AFAIK this just tells runtime pm that we've woken up the device
(which we have), so that it does not call bcm_resume_device() *again*
on the first runtime_pm_get(). The pm_runtime_enable() will start the
autosuspend timer I believe (I could be wrong I'm not an expert on this)
and then we will autosuspend again after the timer expires.

> Another thing that I find utterly confusing:  We set up the sleep
> parameters on ->setup if and only if the device has an IRQ.
> The sleep parameters influence how the wake pin is used (polarity etc).
> However we toggle the pin *before* setting up the sleep parameters,
> namely by calling bcm_gpio_set_power() in bcm_open(), bcm_probe() and
> bcm_serdev_probe().  Is that actually correct?  Shouldn't we avoid
> toggling the wake pin until the sleep parameters have been set up?
> Would it be conceivable that the sleep parameters are somehow botched
> when the device comes out of power-on reset, such that toggling the
> wake pin before setting the sleep parameters to sane values causes
> the device to become unresponsive or something like that?

A valid question, to which I've no answer, it seems that the device
either defaults to bt_wake_active = 1 (as we set it later), or to
sleep_mode = 0 (disabled) so what we set the device-wakeup gpio to
does not matter as it does not sleep.

I'm afraid we lack documentation here, an other interesting observation
is that we set:

         sleep_params.host_wake_active = !bcm->dev->irq_active_low;

So theoretically if we chose the IRQ to be active-low or active-high
does not matter, because we tell the bluetooth device what we want,
yet on a whole bunch of Bay Trail devices things do not work unless
we force the IRQ to be active-low (trigger on falling edge), suggesting
that the firmware ignores this bit.

Regards,

Hans





> 
> Thanks,
> 
> Lukas
> 
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/bluetooth/hci_bcm.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>> index eee33b0e4d67..06e2434c9b3d 100644
>> --- a/drivers/bluetooth/hci_bcm.c
>> +++ b/drivers/bluetooth/hci_bcm.c
>> @@ -464,7 +464,7 @@ static int bcm_close(struct hci_uart *hu)
>>   		err = bcm_gpio_set_power(bdev, false);
>>   		if (err)
>>   			bt_dev_err(hu->hdev, "Failed to power down");
>> -		else
>> +		else if (IS_ENABLED(CONFIG_PM) && bdev->irq > 0)
>>   			pm_runtime_set_suspended(bdev->dev);
>>   	}
>>   	mutex_unlock(&bcm_device_lock);
>> -- 
>> 2.14.3
>>

Re: [PATCH 1/2] Bluetooth: hci_bcm: Don't change runtime-status when not using runtime-pm

[Lukas Wunner]

On Sat, Mar 24, 2018 at 12:07:10PM +0100, Hans de Goede wrote:
> On 24-03-18 09:02, Lukas Wunner wrote:
> >On Mon, Mar 19, 2018 at 10:02:36PM +0100, Hans de Goede wrote:
> >>The pm_runtime_set_suspended() call in bcm_close() is intended as a
> >>counterpart to the pm_runtime_set_active() call from bcm_request_irq()
> >>(which gets called from bcm_setup()).
> >>
> >>The pm_runtime_set_active() call only happens if our pre-requisites for
> >>doing runtime-pm are met. This commit adds the same check to the
> >>pm_runtime_set_suspended() call to avoid the runtime-pm state getting
> >>stuck in suspended after after a bcm_close().
> >
> >Have you actually observed the state getting stuck in suspended?
> >I'm asking because I don't quite see how this could happen:
> >pm_runtime_set_suspended() becomes a no-op unless runtime PM is disabled.
> >Now, a few lines further up in bcm_close(), pm_runtime_disable() is only
> >called under the conditions you're adding here.  If it's not called,
> >pm_runtime_set_suspended() doesn't have any effect anyway so adding the
> >conditions is pointless.  Am I missing something?
> 
> All devices start with runtime-pm disabled and on devices without
> an IRQ we never enable it. So when the pm_runtime_set_suspended()
> gets called runtime-pm is still in its initial disabled state.

Okay but the device's initial state is set to RPM_SUSPENDED by
pm_runtime_init() and AFAICS neither the platform nor acpi buses
subsequently set it to RPM_ACTIVE, so the device is in suspended
state anyway, unless I've overlooked some detail.

Which raises another question, bcm_suspend() only calls
bcm_suspend_device() if it's runtime active.  So devices
without IRQ, which are always in runtime suspended state,
are apparently never orderly put to sleep on ->suspend.

Also, if the device is always in suspended state, there's nothing
that keeps the UART in active state.  Has anyone ever actually
tested if this driver works without IRQ?


> >Related:  I notice that bcm_resume() calls pm_runtime_set_active().
> >Doesn't that have the effect that if the device is not opened and
> >the system goes to sleep, the device is kept in runtime active state
> >after resume and thus prevents the UART from runtime suspending until
> >the device is opened?
> 
> AFAIK this just tells runtime pm that we've woken up the device
> (which we have), so that it does not call bcm_resume_device() *again*
> on the first runtime_pm_get().

That's odd, by default the PM core runtime resumes a device before
going to system sleep, the only exception is if the devices uses
the "direct_complete" procedure, which requires that ->prepare
returns a positive int.  The platform bus doesn't even have a
->prepare hook, so never uses direct complete AFAICS, and the acpi
bus uses direct_complete only under certain conditions and even then
devices seem to be resumed to runtime active state when the system wakes,
according to the code comment in acpi_subsys_resume_noirq().

So the pm_runtime_set_active() in bcm_resume() looks odd to me.


> The pm_runtime_enable() will start the
> autosuspend timer I believe (I could be wrong I'm not an expert on this)
> and then we will autosuspend again after the timer expires.

No, pm_runtime_enable() doesn't do that, but device_complete calls
pm_runtime_put() as the last step when coming out of system sleep,
and that will start the autosuspend timer.

Thanks,

Lukas

Re: [PATCH 1/2] Bluetooth: hci_bcm: Don't change runtime-status when not using runtime-pm

[Hans de Goede]

Hi,

On 24-03-18 15:19, Lukas Wunner wrote:
> On Sat, Mar 24, 2018 at 12:07:10PM +0100, Hans de Goede wrote:
>> On 24-03-18 09:02, Lukas Wunner wrote:
>>> On Mon, Mar 19, 2018 at 10:02:36PM +0100, Hans de Goede wrote:
>>>> The pm_runtime_set_suspended() call in bcm_close() is intended as a
>>>> counterpart to the pm_runtime_set_active() call from bcm_request_irq()
>>>> (which gets called from bcm_setup()).
>>>>
>>>> The pm_runtime_set_active() call only happens if our pre-requisites for
>>>> doing runtime-pm are met. This commit adds the same check to the
>>>> pm_runtime_set_suspended() call to avoid the runtime-pm state getting
>>>> stuck in suspended after after a bcm_close().
>>>
>>> Have you actually observed the state getting stuck in suspended?
>>> I'm asking because I don't quite see how this could happen:
>>> pm_runtime_set_suspended() becomes a no-op unless runtime PM is disabled.
>>> Now, a few lines further up in bcm_close(), pm_runtime_disable() is only
>>> called under the conditions you're adding here.  If it's not called,
>>> pm_runtime_set_suspended() doesn't have any effect anyway so adding the
>>> conditions is pointless.  Am I missing something?
>>
>> All devices start with runtime-pm disabled and on devices without
>> an IRQ we never enable it. So when the pm_runtime_set_suspended()
>> gets called runtime-pm is still in its initial disabled state.
> 
> Okay but the device's initial state is set to RPM_SUSPENDED by
> pm_runtime_init() and AFAICS neither the platform nor acpi buses
> subsequently set it to RPM_ACTIVE, so the device is in suspended
> state anyway, unless I've overlooked some detail.

Ah, no you're right, I assumed that devices would default to
RPM_ACTIVE (which corresponds to value 0), but you are right
they are set to suspended by default.

So this patch can be dropped.

Marcel: It would still be nice to get the second patch from
this series merged.

> Which raises another question, bcm_suspend() only calls
> bcm_suspend_device() if it's runtime active.  So devices
> without IRQ, which are always in runtime suspended state,
> are apparently never orderly put to sleep on ->suspend.

No that is not a problem, pm_runtime_active() is defined as:

static inline bool pm_runtime_active(struct device *dev)
{
         return dev->power.runtime_status == RPM_ACTIVE
                 || dev->power.disable_depth;
}

On devices without an IRQ we never enable runtime_pm, so
dev->power.disable_depth stays at its initial value of 1 and
this function will return 1.

> Also, if the device is always in suspended state, there's nothing
> that keeps the UART in active state.  Has anyone ever actually
> tested if this driver works without IRQ?

AFAIK the code deciding to suspend parents also uses
pm_runtime_active()

The reason why I was worried about the suspended status field is
because there also is pm_runtime_status_suspended() which only
checks the status field, but I guess that is only used in
special cases.

>>> Related:  I notice that bcm_resume() calls pm_runtime_set_active().
>>> Doesn't that have the effect that if the device is not opened and
>>> the system goes to sleep, the device is kept in runtime active state
>>> after resume and thus prevents the UART from runtime suspending until
>>> the device is opened?
>>
>> AFAIK this just tells runtime pm that we've woken up the device
>> (which we have), so that it does not call bcm_resume_device() *again*
>> on the first runtime_pm_get().
> 
> That's odd, by default the PM core runtime resumes a device before
> going to system sleep, the only exception is if the devices uses
> the "direct_complete" procedure, which requires that ->prepare
> returns a positive int.  The platform bus doesn't even have a
> ->prepare hook, so never uses direct complete AFAICS, and the acpi
> bus uses direct_complete only under certain conditions and even then
> devices seem to be resumed to runtime active state when the system wakes,
> according to the code comment in acpi_subsys_resume_noirq().
> 
> So the pm_runtime_set_active() in bcm_resume() looks odd to me.

This code precedes my involvement in the driver, I agree this looks
a bit weird.

>> The pm_runtime_enable() will start the
>> autosuspend timer I believe (I could be wrong I'm not an expert on this)
>> and then we will autosuspend again after the timer expires.
> 
> No, pm_runtime_enable() doesn't do that, but device_complete calls
> pm_runtime_put() as the last step when coming out of system sleep,
> and that will start the autosuspend timer.

Ok.

Regards,

Hans

Re: [PATCH 1/2] Bluetooth: hci_bcm: Don't change runtime-status when not using runtime-pm

[Marcel Holtmann]

Hi Hans,

>>>>> The pm_runtime_set_suspended() call in bcm_close() is intended as a
>>>>> counterpart to the pm_runtime_set_active() call from bcm_request_irq()
>>>>> (which gets called from bcm_setup()).
>>>>> 
>>>>> The pm_runtime_set_active() call only happens if our pre-requisites for
>>>>> doing runtime-pm are met. This commit adds the same check to the
>>>>> pm_runtime_set_suspended() call to avoid the runtime-pm state getting
>>>>> stuck in suspended after after a bcm_close().
>>>> 
>>>> Have you actually observed the state getting stuck in suspended?
>>>> I'm asking because I don't quite see how this could happen:
>>>> pm_runtime_set_suspended() becomes a no-op unless runtime PM is disabled.
>>>> Now, a few lines further up in bcm_close(), pm_runtime_disable() is only
>>>> called under the conditions you're adding here.  If it's not called,
>>>> pm_runtime_set_suspended() doesn't have any effect anyway so adding the
>>>> conditions is pointless.  Am I missing something?
>>> 
>>> All devices start with runtime-pm disabled and on devices without
>>> an IRQ we never enable it. So when the pm_runtime_set_suspended()
>>> gets called runtime-pm is still in its initial disabled state.
>> Okay but the device's initial state is set to RPM_SUSPENDED by
>> pm_runtime_init() and AFAICS neither the platform nor acpi buses
>> subsequently set it to RPM_ACTIVE, so the device is in suspended
>> state anyway, unless I've overlooked some detail.
> 
> Ah, no you're right, I assumed that devices would default to
> RPM_ACTIVE (which corresponds to value 0), but you are right
> they are set to suspended by default.
> 
> So this patch can be dropped.
> 
> Marcel: It would still be nice to get the second patch from
> this series merged.

can you resent that patch? And should it also go into stable?

Regards

Marcel

Re: [PATCH 1/2] Bluetooth: hci_bcm: Don't change runtime-status when not using runtime-pm

[Hans de Goede]

Hi Marcel,

On 03-04-18 16:16, Marcel Holtmann wrote:
> Hi Hans,
> 
>>>>>> The pm_runtime_set_suspended() call in bcm_close() is intended as a
>>>>>> counterpart to the pm_runtime_set_active() call from bcm_request_irq()
>>>>>> (which gets called from bcm_setup()).
>>>>>>
>>>>>> The pm_runtime_set_active() call only happens if our pre-requisites for
>>>>>> doing runtime-pm are met. This commit adds the same check to the
>>>>>> pm_runtime_set_suspended() call to avoid the runtime-pm state getting
>>>>>> stuck in suspended after after a bcm_close().
>>>>>
>>>>> Have you actually observed the state getting stuck in suspended?
>>>>> I'm asking because I don't quite see how this could happen:
>>>>> pm_runtime_set_suspended() becomes a no-op unless runtime PM is disabled.
>>>>> Now, a few lines further up in bcm_close(), pm_runtime_disable() is only
>>>>> called under the conditions you're adding here.  If it's not called,
>>>>> pm_runtime_set_suspended() doesn't have any effect anyway so adding the
>>>>> conditions is pointless.  Am I missing something?
>>>>
>>>> All devices start with runtime-pm disabled and on devices without
>>>> an IRQ we never enable it. So when the pm_runtime_set_suspended()
>>>> gets called runtime-pm is still in its initial disabled state.
>>> Okay but the device's initial state is set to RPM_SUSPENDED by
>>> pm_runtime_init() and AFAICS neither the platform nor acpi buses
>>> subsequently set it to RPM_ACTIVE, so the device is in suspended
>>> state anyway, unless I've overlooked some detail.
>>
>> Ah, no you're right, I assumed that devices would default to
>> RPM_ACTIVE (which corresponds to value 0), but you are right
>> they are set to suspended by default.
>>
>> So this patch can be dropped.
>>
>> Marcel: It would still be nice to get the second patch from
>> this series merged.
> 
> can you resent that patch?

Done.

> And should it also go into stable?

I'm not sure, the hardware which needs this is somewhat rare I guess,
so I did not add a Cc: stable myself. But if you think this makes sense
for stable feel free to add a Cc: stable I've no objections against that.

Regards,

Hans