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

[Peng Fan]

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