Re: [RFC] platform: detach from PM domains on shutdown

From: Ulf Hansson <ulf.hansson@linaro.org>
To: Peng Fan <peng.fan@nxp.com>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
	Fabio Estevam <fabio.estevam@nxp.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Linux PM <linux-pm@vger.kernel.org>,
	linux-imx@nxp.com
Subject: Re: [RFC] platform: detach from PM domains on shutdown
Date: Wed, 16 May 2018 11:52:33 +0200	[thread overview]
Message-ID: <CAPDyKFpKGhLrj-px1E3C_tq3_2GAxWpzSgr5TXJU7s8-jokf9g@mail.gmail.com> (raw)
In-Reply-To: <20180515090110.26820-1-peng.fan@nxp.com>

On 15 May 2018 at 11:01, Peng Fan <peng.fan@nxp.com> wrote:
> When reboot Linux, the PM domains attached to a device
> are not shutdown. To SoCs which relys on reset the whole SoC,
> there is no need to shutdown PM domains, but to Linux running
> in a virtual machine with devices pass-through, we could not
> reset the whole SoC. Currently we need Linux to shutdown its
> PM domains when reboot.

I am not sure I understand exactly why the PM domain needs to be
shutdown for these cases, could you please elaborate a bit on that.

BTW, what platform are you running on and also what PM domains are being used?

Anyway, it seems like there may be need for certain cases, but
certainly not all - especially since it may slow down the shutdown
process, when not needed.

Can we make this runtime configurable, via sysfs or whatever that makes sense!?

>
> commit 2d30bb0b3889 ("platform: Do not detach from PM domains on shutdown"),
> removes what this patch tries to add, because of a warning.
> commit e79aee49bcf9 ("PM: Avoid false-positive warnings in dev_pm_domain_set()")
> already fixes the false alarm warning. So let's detach the power domain
> to shutdown PM domains after driver shutdown.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>
> I do not find a better place to shutdown power domain when reboot Linux,
> so add back the line that commit 2d30bb0b3889 removes, because it is
> a false alarm warning as commit e79aee49bcf9 describes.
>
>  drivers/base/platform.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 8075ddc70a17..a5929f24dc3c 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -616,6 +616,7 @@ static void platform_drv_shutdown(struct device *_dev)
>
>         if (drv->shutdown)
>                 drv->shutdown(dev);
> +       dev_pm_domain_detach(_dev, true);

This would somewhat work, but only for platform devices. To make this
fully work, we need to call dev_pm_domain_detach() from amba, spi, etc
as well.

Perhaps another option to manage this more generally, an without
having detach devices, could be to extend the struct dev_pm_domain
with a new callback, "->shutdown()" and then make the driver core call
it from device_shutdown().

Typically, for genpd, I would probably count the number of calls being
made to ->shutdown() per PM domain, then when it reaches the number of
attached devices to it, allow to power off it.

Let's see what Rafael thinks about it.

Kind regards
Uffe