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

[Peng Fan]

> Subject: Re: [PATCH] firmware: arm_scmi: power_control: support suspend
> command
> 
> 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.

Thanks for explaining the details. I see that in kernel/reboot.c

> 
> > > 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.

Thanks for helping.

> 
> Anyway the change seems good to me as I said.

ok, I will post v2 with commit log updated later, waiting some feedback from
others.

Thanks
Peng.
> 
> Thanks,
> Cristian