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

[Lukas Wunner <lukas@wunner.de>]

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