Prevent PM suspend from powering off the domain for non-wakeup in-use devices

Prevent PM suspend from powering off the domain for non-wakeup in-use devices

[Ajay Agarwal]

Hello Linux PM experts
I have a question on PM domain. As per the current PM domain driver design, the genpd_finish_suspend turns OFF a power domain if it is not already turned OFF by runtime suspend.

I have a usecase of a device having to stay ON during system suspend. This device will be used by a co-processor which is running its own OS. Thereby, it requires the power domain to stay turned ON, so genpd_finish_suspend should not be powering down the domain.

I studied the code and found the flag `GENPD_FLAG_ACTIVE_WAKEUP` which can be set for the power domain. And device_set_wakeup_path can be invoked in the suspend() callback of the device. Together, these will prevent the genpd_finish_suspend from turning OFF the domain. See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/power/domain.c#n1260

But this flag is really intended to be used for devices which are capable of waking up the system from PM suspend. But my usecase does not involve a scenario of the device potentially waking up the system, it just needs to stay powered for the co-processor to be able to use it.

Can you suggest if I should go ahead with using the existing framework of `GENPD_FLAG_ACTIVE_WAKEUP`? Or add a new flag, say `GENPD_FLAG_RPM_ONLY` for this scenario?

Thanks
Ajay

Re: Prevent PM suspend from powering off the domain for non-wakeup in-use devices

[Ulf Hansson]

+ Manivannan

On Thu, 6 Jul 2023 at 05:06, Ajay Agarwal <ajayagarwal@google.com> wrote:
>
> Hello Linux PM experts
> I have a question on PM domain. As per the current PM domain driver design, the genpd_finish_suspend turns OFF a power domain if it is not already turned OFF by runtime suspend.
>
> I have a usecase of a device having to stay ON during system suspend. This device will be used by a co-processor which is running its own OS. Thereby, it requires the power domain to stay turned ON, so genpd_finish_suspend should not be powering down the domain.
>
> I studied the code and found the flag `GENPD_FLAG_ACTIVE_WAKEUP` which can be set for the power domain. And device_set_wakeup_path can be invoked in the suspend() callback of the device. Together, these will prevent the genpd_finish_suspend from turning OFF the domain. See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/power/domain.c#n1260

To solve the problem, this is exactly what I would have started to explore too.

>
> But this flag is really intended to be used for devices which are capable of waking up the system from PM suspend. But my usecase does not involve a scenario of the device potentially waking up the system, it just needs to stay powered for the co-processor to be able to use it.

Sure, I understand your point. Maybe it's as simple as renaming the
device_set_wakeup_path() and corresponding flag to something more
generic.

Solving the problem that you are looking for, would probably be done
along the lines of device_set_wakeup_path() and
GENPD_FLAG_ACTIVE_WAKEUP anyway. And we really don't want to two
solutions for one similar problem, if you get my point.

>
> Can you suggest if I should go ahead with using the existing framework of `GENPD_FLAG_ACTIVE_WAKEUP`? Or add a new flag, say `GENPD_FLAG_RPM_ONLY` for this scenario?

Let's see what Rafael thinks, but renaming is an option (or add a
wrapper function that calls device_set_wakeup_path()) that would work
for me.

>
> Thanks
> Ajay

Note that, I have looped in Manivannan, who is working on a very
similar problem. The problem can be summarized like this:

Depending if there is an nvme storage device added to the pcie
interface, the nvmw storage must remain powered on during system
suspend. For that reason, we also need the part from the PM core where
propagation of the flag is done from child to parents.

It's been a couple of weeks ago since I chatted with Manivannan about
this - and I suggested the approach you have been exploring too. Let's
see if Manivannan has some more updates to share from his side around
this.

Kind regards
Uffe

Re: Prevent PM suspend from powering off the domain for non-wakeup in-use devices

[Manivannan Sadhasivam]

On Fri, Jul 07, 2023 at 04:49:59PM +0200, Ulf Hansson wrote:
> + Manivannan
> 
> On Thu, 6 Jul 2023 at 05:06, Ajay Agarwal <ajayagarwal@google.com> wrote:
> >
> > Hello Linux PM experts
> > I have a question on PM domain. As per the current PM domain driver design, the genpd_finish_suspend turns OFF a power domain if it is not already turned OFF by runtime suspend.
> >
> > I have a usecase of a device having to stay ON during system suspend. This device will be used by a co-processor which is running its own OS. Thereby, it requires the power domain to stay turned ON, so genpd_finish_suspend should not be powering down the domain.
> >
> > I studied the code and found the flag `GENPD_FLAG_ACTIVE_WAKEUP` which can be set for the power domain. And device_set_wakeup_path can be invoked in the suspend() callback of the device. Together, these will prevent the genpd_finish_suspend from turning OFF the domain. See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/power/domain.c#n1260
> 
> To solve the problem, this is exactly what I would have started to explore too.
> 
> >
> > But this flag is really intended to be used for devices which are capable of waking up the system from PM suspend. But my usecase does not involve a scenario of the device potentially waking up the system, it just needs to stay powered for the co-processor to be able to use it.
> 
> Sure, I understand your point. Maybe it's as simple as renaming the
> device_set_wakeup_path() and corresponding flag to something more
> generic.
> 
> Solving the problem that you are looking for, would probably be done
> along the lines of device_set_wakeup_path() and
> GENPD_FLAG_ACTIVE_WAKEUP anyway. And we really don't want to two
> solutions for one similar problem, if you get my point.
> 
> >
> > Can you suggest if I should go ahead with using the existing framework of `GENPD_FLAG_ACTIVE_WAKEUP`? Or add a new flag, say `GENPD_FLAG_RPM_ONLY` for this scenario?
> 
> Let's see what Rafael thinks, but renaming is an option (or add a
> wrapper function that calls device_set_wakeup_path()) that would work
> for me.
> 
> >
> > Thanks
> > Ajay
> 
> Note that, I have looped in Manivannan, who is working on a very
> similar problem. The problem can be summarized like this:
> 
> Depending if there is an nvme storage device added to the pcie
> interface, the nvmw storage must remain powered on during system
> suspend. For that reason, we also need the part from the PM core where
> propagation of the flag is done from child to parents.
> 
> It's been a couple of weeks ago since I chatted with Manivannan about
> this - and I suggested the approach you have been exploring too. Let's
> see if Manivannan has some more updates to share from his side around
> this.
> 

Thanks Ulf for looping me in. I worked around the issue at the hardware level
i.e., On the Qcom SoCs we can set the power domain in retention mode. So linux
genpd driver will not turn off those domains marked as retention and the
power management hardware (RPMh) will use a backup domain to preserve the state
of the domain even if it's parent domain goes down during system suspend. This
keeps the NVMe device ON during suspend and at the same time allows the SoC to
enter low power state.

I couldn't update you on the progress of this issue since I was out for a couple
of weeks.

But the solution I described above is pretty much Qcom specific, so it is indeed
useful to have a generic solution in genpd framework similar to
device_set_wakeup_path().

My suggestion would be to introduce a new helper instead of renaming the
existing device_set_wakeup_path() API, eventhough both are doing the same but
their purposes differ.

- Mani

> Kind regards
> Uffe

-- 
மணிவண்ணன் சதாசிவம்

Re: Prevent PM suspend from powering off the domain for non-wakeup in-use devices

[Ajay Agarwal]

Thanks Ulf and Mani for looking into this. I could not quite understand
your suggestions here. I believe you are talking about adding a new
helper API. What flag will this API update? Will it be
`dev->power.wakeup_path` itself? Or do we introduce a new boolean flag
in dev_pm_info structure for this?

Also, how about the genpd flag? Shall we re-use the
GENPD_FLAG_ACTIVE_WAKEUP flag? Or create a new one?

I feel that we should come up with a generic name for both the genpd
flag and the power flag to satisfy both the wakeup usecase and the
stay-on usecase. Let me know what you think.

Rafael, can you chime in as well?

Re: Prevent PM suspend from powering off the domain for non-wakeup in-use devices

[Ulf Hansson]

On Mon, 10 Jul 2023 at 20:00, Ajay Agarwal <ajayagarwal@google.com> wrote:
>
> Thanks Ulf and Mani for looking into this. I could not quite understand
> your suggestions here. I believe you are talking about adding a new
> helper API. What flag will this API update? Will it be
> `dev->power.wakeup_path` itself? Or do we introduce a new boolean flag
> in dev_pm_info structure for this?

Honestly I am not exactly sure on what I think. Someone (possibly
myself) needs to draft a patch to see what it looks like. Although,
adding a new flag that would behave exactly like
'dev->power.wakeup_path' seems a bit silly to me.

>
> Also, how about the genpd flag? Shall we re-use the
> GENPD_FLAG_ACTIVE_WAKEUP flag? Or create a new one?
>
> I feel that we should come up with a generic name for both the genpd
> flag and the power flag to satisfy both the wakeup usecase and the
> stay-on usecase. Let me know what you think.

I agree that the naming isn't particularly good for this use case. I
don't have a good suggestion for how to deal with it. If these things
aren't being used too much out in the kernel, maybe a tree wide commit
(at some good point) to rename things could work?

>
> Rafael, can you chime in as well?

Kind regards
Uffe

Re: Prevent PM suspend from powering off the domain for non-wakeup in-use devices

[Michael Shavit]

On Thu, Jul 6, 2023 at 11:06 AM Ajay Agarwal <ajayagarwal@google.com> wrote:
>
> Hello Linux PM experts
> I have a question on PM domain. As per the current PM domain driver design, the genpd_finish_suspend turns OFF a power domain if it is not already turned OFF by runtime suspend.

I don't really understand why genPD does this in the first place. Not
only does it power off domains that might have RPM_ACTIVE devices
during suspend, it also powers on domains that might not have any
active devices in them during resume (except for wakeup devices,
similar to power-off. See genpd_finish_resume).

Why isn't genPD powering-off/on domains based on device's RPM status
sufficient? Assuming those are correctly set by device drivers during
system suspend/resume.

Such that this:
> But this flag is really intended to be used for devices which are capable of waking up the system from PM suspend. But my usecase does not involve a scenario of the device potentially waking up the system, it just needs to stay powered for the co-processor to be able to use it.

becomes just a matter of the device keeping its RPM status as active
during system suspend.

Re: Prevent PM suspend from powering off the domain for non-wakeup in-use devices

[Ulf Hansson]

On Mon, 31 Jul 2023 at 14:28, Michael Shavit <mshavit@google.com> wrote:
>
> On Thu, Jul 6, 2023 at 11:06 AM Ajay Agarwal <ajayagarwal@google.com> wrote:
> >
> > Hello Linux PM experts
> > I have a question on PM domain. As per the current PM domain driver design, the genpd_finish_suspend turns OFF a power domain if it is not already turned OFF by runtime suspend.
>
> I don't really understand why genPD does this in the first place. Not
> only does it power off domains that might have RPM_ACTIVE devices
> during suspend, it also powers on domains that might not have any
> active devices in them during resume (except for wakeup devices,
> similar to power-off. See genpd_finish_resume).
>
> Why isn't genPD powering-off/on domains based on device's RPM status
> sufficient? Assuming those are correctly set by device drivers during
> system suspend/resume.

I think you kind of already answered this by yourself. We can't rely
on all drivers to update the runtime PM status of their devices during
system wide suspend - simply because there are no requirements (legacy
wise) that they need to.

Moreover, as runtime PM gets disabled/enabled for all devices by the
PM core in the suspend_late/resume_early phase (for good reasons) - we
have a window when runtime PM can't be used to power-on/off devices.
In other words, if a driver needs to power on its device (and the
corresponding PM domain) during this window, how will that be done?

>
> Such that this:
> > But this flag is really intended to be used for devices which are capable of waking up the system from PM suspend. But my usecase does not involve a scenario of the device potentially waking up the system, it just needs to stay powered for the co-processor to be able to use it.
>
> becomes just a matter of the device keeping its RPM status as active
> during system suspend.

Theoretically this could work, but unfortunately not in practice with
all the legacy code also taken into account.

Kind regards
Uffe

Re: Prevent PM suspend from powering off the domain for non-wakeup in-use devices

[Michael Shavit]

On Thu, Aug 10, 2023 at 11:57 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> I think you kind of already answered this by yourself. We can't rely
> on all drivers to update the runtime PM status of their devices during
> system wide suspend - simply because there are no requirements (legacy
> wise) that they need to.
[...]
> Moreover, as runtime PM gets disabled/enabled for all devices by the
> PM core in the suspend_late/resume_early phase (for good reasons) - we
> have a window when runtime PM can't be used to power-on/off devices.
> In other words, if a driver needs to power on its device (and the
> corresponding PM domain) during this window, how will that be done?

Ok makes sense. If we're to rename dev->power.wakeup_path, could we
then think of it as a way for device drivers to declare that their RPM
state is in fact managed during system suspend/resume?

Another question that I haven't been able to figure out by tracing
through the code history, is why the genpd domain *also* needs to
declare the GENPD_FLAG_ACTIVE_WAKEUP flag. Shouldn't device drivers
setting device_set_wakeup_path() be sufficient for the core to decide
not to force power it off during suspend? In what scenarios would a
genpd provider want to leave that flag unset?

Re: Prevent PM suspend from powering off the domain for non-wakeup in-use devices

[Ulf Hansson]

On Thu, 10 Aug 2023 at 20:00, Michael Shavit <mshavit@google.com> wrote:
>
> On Thu, Aug 10, 2023 at 11:57 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > I think you kind of already answered this by yourself. We can't rely
> > on all drivers to update the runtime PM status of their devices during
> > system wide suspend - simply because there are no requirements (legacy
> > wise) that they need to.
> [...]
> > Moreover, as runtime PM gets disabled/enabled for all devices by the
> > PM core in the suspend_late/resume_early phase (for good reasons) - we
> > have a window when runtime PM can't be used to power-on/off devices.
> > In other words, if a driver needs to power on its device (and the
> > corresponding PM domain) during this window, how will that be done?
>
> Ok makes sense. If we're to rename dev->power.wakeup_path, could we
> then think of it as a way for device drivers to declare that their RPM
> state is in fact managed during system suspend/resume?

I just posted a patch and decided to start simple, just by adding a
couple of new helper functions that act as wrappers of the existing
ones.

What you are proposing seems to be something entirely new. We can't
reuse the existing flag for this kind of thing, I think.

>
> Another question that I haven't been able to figure out by tracing
> through the code history, is why the genpd domain *also* needs to
> declare the GENPD_FLAG_ACTIVE_WAKEUP flag. Shouldn't device drivers
> setting device_set_wakeup_path() be sufficient for the core to decide
> not to force power it off during suspend? In what scenarios would a
> genpd provider want to leave that flag unset?

That's a good question. In principle we decided to keep
GENPD_FLAG_ACTIVE_WAKEUP a while ago [1], as a way to let the genpd
provider opt-in to support so called in-band wakeup irqs.

For out-band wakeup irqs there may be some additional pieces missing,
to allow the driver/subsystem to inform the PM domain that it has
configured a system wakeup, that doesn't require the in-band wakeup to
be armed.

Kind regards
Uffe

[1]
https://lore.kernel.org/all/1510588003-16650-3-git-send-email-ulf.hansson@linaro.org/