Re: [PATCH] firmware: arm_scmi: power_control: support suspend command

[Cristian Marussi]

On Tue, Apr 16, 2024 at 07:01:42AM +0000, Peng Fan wrote:
> Hi Cristian,
> 

Hi,

> > Subject: Re: [PATCH] firmware: arm_scmi: power_control: support suspend
> > command
> > 
> > On Mon, Apr 15, 2024 at 06:11:41PM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@xxxxxxx>
> > >
> > > Support System suspend notification. Using a work struct to call
> > > pm_suspend. There is no way to pass suspend level to pm_suspend, so
> > > use MEM as of now.
> > >
> > 
> > Hi Peng,
> > 
> > > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > > ---
> > >  .../firmware/arm_scmi/scmi_power_control.c    | 20 ++++++++++++++++++-
> > >  1 file changed, 19 insertions(+), 1 deletion(-)
> > >
> > 
> > This LGTM in general, the only obsservation here is that while on shutdown
> > by issuing a orderly_reboot() we in fact ask PID_1 to start a shutdown from
> 
> Would you please share why PID_1 is involved here? orderly_reboot is just
> schedule a work.

For the shutdown case, orderly_reboot related scheduled work ends up in a
call to /sbin/reboot via usermodehelper kernel facilities, so that, depending
on what PID_1 you have and how it is configured, PID_1 does have a chance to
perform some additional task (if configured) before triggering the real final
shutdown....this is done because the SCMI Notification is supposed to be a
graceful request, so we dont just kernel_poweroff or similar.

As an example with SystemD PID_1 /sbin/reboot is just a link to systemctl
and you can place whatever you want in

/usr/lib/systemd/system-shutdown/

and that it will executed by systemctl before kernel shutdown is really
triggered.

> > userspace, when triggering a system suspend with pm_suspend() we do not
> > involve userspace in the process, but I dont think there is another way indeed.
> 
> Userspace process should not be involved, otherwise the freezing process will
> never finish, and suspend will abort.
> 

Similarly in the suspend case when you initiate it from userspace (systemcl suspend)
you can place something in /usr/lib/systemd/system-sleep/ and have it executed before
suspend is passwed on to the kernel, BUT in our case we are not passing through
userspace and it seems there is no way to do it, indeed, like we do for shutdown with
orderly_reboot(). Moreover userspace, as Sudeep mentioned in the other thread, could
be configured to trigger a different type of suspend, it it was involved at all 
in this process.

But, as said, I dont think there is a way to trigger a userspace
suspennd from jernel like we do for shutdown/reboot...I'll try to
investigate a bit more.

Anyway the change seems good to me as I said.

Thanks,
Cristian