| Message ID | 20240415101141.1591112-1-peng.fan@oss.nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | firmware: arm_scmi: power_control: support suspend command | expand |
On Mon, Apr 15, 2024 at 06:11:41PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > 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@nxp.com> > --- > .../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 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. Thanks, Cristian > diff --git a/drivers/firmware/arm_scmi/scmi_power_control.c b/drivers/firmware/arm_scmi/scmi_power_control.c > index 6eb7d2a4b6b1..beafca9957c7 100644 > --- a/drivers/firmware/arm_scmi/scmi_power_control.c > +++ b/drivers/firmware/arm_scmi/scmi_power_control.c > @@ -50,6 +50,7 @@ > #include <linux/reboot.h> > #include <linux/scmi_protocol.h> > #include <linux/slab.h> > +#include <linux/suspend.h> > #include <linux/time64.h> > #include <linux/timer.h> > #include <linux/types.h> > @@ -90,6 +91,7 @@ struct scmi_syspower_conf { > struct notifier_block reboot_nb; > > struct delayed_work forceful_work; > + struct work_struct suspend_work; > }; > > #define userspace_nb_to_sconf(x) \ > @@ -249,6 +251,9 @@ static void scmi_request_graceful_transition(struct scmi_syspower_conf *sc, > case SCMI_SYSTEM_WARMRESET: > orderly_reboot(); > break; > + case SCMI_SYSTEM_SUSPEND: > + schedule_work(&sc->suspend_work); > + break; > default: > break; > } > @@ -277,7 +282,8 @@ static int scmi_userspace_notifier(struct notifier_block *nb, > struct scmi_system_power_state_notifier_report *er = data; > struct scmi_syspower_conf *sc = userspace_nb_to_sconf(nb); > > - if (er->system_state >= SCMI_SYSTEM_POWERUP) { > + if (er->system_state >= SCMI_SYSTEM_MAX || > + er->system_state == SCMI_SYSTEM_POWERUP) { > dev_err(sc->dev, "Ignoring unsupported system_state: 0x%X\n", > er->system_state); > return NOTIFY_DONE; > @@ -315,6 +321,16 @@ static int scmi_userspace_notifier(struct notifier_block *nb, > return NOTIFY_OK; > } > > +static void scmi_suspend_work_func(struct work_struct *work) > +{ > + struct scmi_syspower_conf *sc = > + container_of(work, struct scmi_syspower_conf, suspend_work); > + > + pm_suspend(PM_SUSPEND_MEM); > + > + sc->state = SCMI_SYSPOWER_IDLE; > +} > + > static int scmi_syspower_probe(struct scmi_device *sdev) > { > int ret; > @@ -338,6 +354,8 @@ static int scmi_syspower_probe(struct scmi_device *sdev) > sc->userspace_nb.notifier_call = &scmi_userspace_notifier; > sc->dev = &sdev->dev; > > + INIT_WORK(&sc->suspend_work, scmi_suspend_work_func); > + > return handle->notify_ops->devm_event_notifier_register(sdev, > SCMI_PROTOCOL_SYSTEM, > SCMI_EVENT_SYSTEM_POWER_STATE_NOTIFIER, > -- > 2.37.1 >
Hi Cristian, > 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@nxp.com> > > > > 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@nxp.com> > > --- > > .../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. > 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. Thanks, Peng. > > Thanks, > Cristian > > > diff --git a/drivers/firmware/arm_scmi/scmi_power_control.c > > b/drivers/firmware/arm_scmi/scmi_power_control.c > > index 6eb7d2a4b6b1..beafca9957c7 100644 > > --- a/drivers/firmware/arm_scmi/scmi_power_control.c > > +++ b/drivers/firmware/arm_scmi/scmi_power_control.c > > @@ -50,6 +50,7 @@ > > #include <linux/reboot.h> > > #include <linux/scmi_protocol.h> > > #include <linux/slab.h> > > +#include <linux/suspend.h> > > #include <linux/time64.h> > > #include <linux/timer.h> > > #include <linux/types.h> > > @@ -90,6 +91,7 @@ struct scmi_syspower_conf { > > struct notifier_block reboot_nb; > > > > struct delayed_work forceful_work; > > + struct work_struct suspend_work; > > }; > > > > #define userspace_nb_to_sconf(x) \ > > @@ -249,6 +251,9 @@ static void scmi_request_graceful_transition(struct > scmi_syspower_conf *sc, > > case SCMI_SYSTEM_WARMRESET: > > orderly_reboot(); > > break; > > + case SCMI_SYSTEM_SUSPEND: > > + schedule_work(&sc->suspend_work); > > + break; > > default: > > break; > > } > > @@ -277,7 +282,8 @@ static int scmi_userspace_notifier(struct > notifier_block *nb, > > struct scmi_system_power_state_notifier_report *er = data; > > struct scmi_syspower_conf *sc = userspace_nb_to_sconf(nb); > > > > - if (er->system_state >= SCMI_SYSTEM_POWERUP) { > > + if (er->system_state >= SCMI_SYSTEM_MAX || > > + er->system_state == SCMI_SYSTEM_POWERUP) { > > dev_err(sc->dev, "Ignoring unsupported system_state: > 0x%X\n", > > er->system_state); > > return NOTIFY_DONE; > > @@ -315,6 +321,16 @@ static int scmi_userspace_notifier(struct > notifier_block *nb, > > return NOTIFY_OK; > > } > > > > +static void scmi_suspend_work_func(struct work_struct *work) { > > + struct scmi_syspower_conf *sc = > > + container_of(work, struct scmi_syspower_conf, > suspend_work); > > + > > + pm_suspend(PM_SUSPEND_MEM); > > + > > + sc->state = SCMI_SYSPOWER_IDLE; > > +} > > + > > static int scmi_syspower_probe(struct scmi_device *sdev) { > > int ret; > > @@ -338,6 +354,8 @@ static int scmi_syspower_probe(struct scmi_device > *sdev) > > sc->userspace_nb.notifier_call = &scmi_userspace_notifier; > > sc->dev = &sdev->dev; > > > > + INIT_WORK(&sc->suspend_work, scmi_suspend_work_func); > > + > > return handle->notify_ops->devm_event_notifier_register(sdev, > > > SCMI_PROTOCOL_SYSTEM, > > > SCMI_EVENT_SYSTEM_POWER_STATE_NOTIFIER, > > -- > > 2.37.1 > >
On Mon, Apr 15, 2024 at 06:11:41PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > 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. > While the change itself is simple and no-controversial, I am bit worried about: 1. The choice of S2R(MEM) by default - not sure if different system prefer different things. The userspace can configure whatever default behaviour expected as S2R IIUC, so should be OK. I need to check though. 2. The userspace needs to keep the wakeup source enabled always which I need to check if it is feasible on all the platforms. If wakeups are not configured properly and suspend is triggered, the system can never resume back. We may need to mention above points at-least as part of commit log. I would wait for some feedback from SCMI users. -- Regards, Sudeep
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@nxp.com> > > > > > > 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@nxp.com> > > > --- > > > .../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
Hi Sudeep, > 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@nxp.com> > > > > 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. > > > > While the change itself is simple and no-controversial, I am bit worried > about: > > 1. The choice of S2R(MEM) by default - not sure if different system > prefer different things. The userspace can configure whatever default > behaviour expected as S2R IIUC, so should be OK. I need to check though. > > 2. The userspace needs to keep the wakeup source enabled always which > I need to check if it is feasible on all the platforms. If wakeups > are not configured properly and suspend is triggered, the system can > never resume back. > > We may need to mention above points at-least as part of commit log. ok, I will add the info you listed in V2 commit log. I would > wait for some feedback from SCMI users. Agree. Thanks, Peng. > > -- > Regards, > Sudeep
> 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@nxp.com> > > > > > > > > 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@nxp.com> > > > > --- > > > > .../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
On Tue, Apr 16, 2024 at 12:34:14PM +0000, Peng Fan wrote: > > 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, 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@nxp.com> > > > > > > > > > > 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. [snip] > > 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. .. also scmi_power_control.c:95: warning: Function parameter or struct member 'suspend_work' not described in 'scmi_syspower_conf' Thanks, Cristian
> Subject: Re: [PATCH] firmware: arm_scmi: power_control: support suspend > command > > On Tue, Apr 16, 2024 at 12:34:14PM +0000, Peng Fan wrote: > > > 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, > > 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@nxp.com> > > > > > > > > > > > > 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. > > [snip] > > > > 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. > > .. also > > scmi_power_control.c:95: warning: Function parameter or struct member > 'suspend_work' not described in 'scmi_syspower_conf' Oh. "@suspend_work: A worker used to trigger system suspend" Thanks, Peng. > > Thanks, > Cristian
diff --git a/drivers/firmware/arm_scmi/scmi_power_control.c b/drivers/firmware/arm_scmi/scmi_power_control.c index 6eb7d2a4b6b1..beafca9957c7 100644 --- a/drivers/firmware/arm_scmi/scmi_power_control.c +++ b/drivers/firmware/arm_scmi/scmi_power_control.c @@ -50,6 +50,7 @@ #include <linux/reboot.h> #include <linux/scmi_protocol.h> #include <linux/slab.h> +#include <linux/suspend.h> #include <linux/time64.h> #include <linux/timer.h> #include <linux/types.h> @@ -90,6 +91,7 @@ struct scmi_syspower_conf { struct notifier_block reboot_nb; struct delayed_work forceful_work; + struct work_struct suspend_work; }; #define userspace_nb_to_sconf(x) \ @@ -249,6 +251,9 @@ static void scmi_request_graceful_transition(struct scmi_syspower_conf *sc, case SCMI_SYSTEM_WARMRESET: orderly_reboot(); break; + case SCMI_SYSTEM_SUSPEND: + schedule_work(&sc->suspend_work); + break; default: break; } @@ -277,7 +282,8 @@ static int scmi_userspace_notifier(struct notifier_block *nb, struct scmi_system_power_state_notifier_report *er = data; struct scmi_syspower_conf *sc = userspace_nb_to_sconf(nb); - if (er->system_state >= SCMI_SYSTEM_POWERUP) { + if (er->system_state >= SCMI_SYSTEM_MAX || + er->system_state == SCMI_SYSTEM_POWERUP) { dev_err(sc->dev, "Ignoring unsupported system_state: 0x%X\n", er->system_state); return NOTIFY_DONE; @@ -315,6 +321,16 @@ static int scmi_userspace_notifier(struct notifier_block *nb, return NOTIFY_OK; } +static void scmi_suspend_work_func(struct work_struct *work) +{ + struct scmi_syspower_conf *sc = + container_of(work, struct scmi_syspower_conf, suspend_work); + + pm_suspend(PM_SUSPEND_MEM); + + sc->state = SCMI_SYSPOWER_IDLE; +} + static int scmi_syspower_probe(struct scmi_device *sdev) { int ret; @@ -338,6 +354,8 @@ static int scmi_syspower_probe(struct scmi_device *sdev) sc->userspace_nb.notifier_call = &scmi_userspace_notifier; sc->dev = &sdev->dev; + INIT_WORK(&sc->suspend_work, scmi_suspend_work_func); + return handle->notify_ops->devm_event_notifier_register(sdev, SCMI_PROTOCOL_SYSTEM, SCMI_EVENT_SYSTEM_POWER_STATE_NOTIFIER,