Received-SPF: pass (google.com: domain of linux-kernel+bounces-145887-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 15 Apr 2024 21:45:20 +0100
From: Cristian Marussi <cristian.marussi@arm.com>
To: "Peng Fan (OSS)" <peng.fan@oss.nxp.com>
Cc: sudeep.holla@arm.com, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Peng Fan <peng.fan@nxp.com>
Subject: Re: [PATCH] firmware: arm_scmi: power_control: support suspend
 command
Message-ID: <Zh2R4FZPmVOigfT9@pluto>
References: <20240415101141.1591112-1-peng.fan@oss.nxp.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240415101141.1591112-1-peng.fan@oss.nxp.com>

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
>