diff mbox series

firmware: arm_scmi: power_control: support suspend command

Message ID 20240415101141.1591112-1-peng.fan@oss.nxp.com (mailing list archive)
State New
Headers show
Series firmware: arm_scmi: power_control: support suspend command | expand

Commit Message

Peng Fan (OSS) April 15, 2024, 10:11 a.m. UTC 
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.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 .../firmware/arm_scmi/scmi_power_control.c    | 20 ++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Comments

Cristian Marussi April 15, 2024, 8:45 p.m. UTC | #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.
> 

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
>
Peng Fan April 16, 2024, 7:01 a.m. UTC | #2 
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
> >
Sudeep Holla April 16, 2024, 9:14 a.m. UTC | #3 
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
Cristian Marussi April 16, 2024, 10:15 a.m. UTC | #4 
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
Peng Fan April 16, 2024, 12:26 p.m. UTC | #5 
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
Peng Fan April 16, 2024, 12:34 p.m. UTC | #6 
> 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
Cristian Marussi April 16, 2024, 1:24 p.m. UTC | #7 
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
Peng Fan April 16, 2024, 1:28 p.m. UTC | #8 
> 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 mbox series

Patch

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,