Received-SPF: pass (google.com: domain of linux-kernel+bounces-146793-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Peng Fan <peng.fan@nxp.com>
To: Cristian Marussi <cristian.marussi@arm.com>
CC: "Peng Fan (OSS)" <peng.fan@oss.nxp.com>, "sudeep.holla@arm.com"
	<sudeep.holla@arm.com>, "linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] firmware: arm_scmi: power_control: support suspend
 command
Thread-Topic: [PATCH] firmware: arm_scmi: power_control: support suspend
 command
Thread-Index: AQHajxwygJMjjnYsXEamYB79VvBL0LFpzXAAgACrohCAADbUAIAAJIBg
Date: Tue, 16 Apr 2024 12:34:14 +0000
Message-ID:
 <DU0PR04MB9417FFF8A9FB5018125C524688082@DU0PR04MB9417.eurprd04.prod.outlook.com>
References: <20240415101141.1591112-1-peng.fan@oss.nxp.com>
 <Zh2R4FZPmVOigfT9@pluto>
 <DU0PR04MB9417AC020C28AD5847CAB24088082@DU0PR04MB9417.eurprd04.prod.outlook.com>
 <Zh5P2EtruMmTrgXM@pluto>
In-Reply-To: <Zh5P2EtruMmTrgXM@pluto>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0:
 =?us-ascii?Q?dJ96POSyIuXZ1SeRMcLc+hQG1XoxTuH2RgUGUSsiwUJZy71tqqLEZkGNMEHS?=
 =?us-ascii?Q?z79vZ1Epq9aJViCW5TgWMM24VV9J0qNi29FL5aJ2G6K39rftVRN8Bvywj8Yu?=
 =?us-ascii?Q?zGYkua5v8a7/sl389y046ReZGMcdYzbibZwlSqsWnLRTkD7yhryWStV2oBI9?=
 =?us-ascii?Q?wAX8Ry6m8lk1AnR0YVNRO5vG9jid2PmzykpoZ4rihP7n3wJkLAIDLlmNYWhB?=
 =?us-ascii?Q?uHeUKu3+MkfYy1+vq+pF2JlqlhjpndgrmWeFH71mXTZ4tYNL6rd9y2c216UU?=
 =?us-ascii?Q?qLU3Pxb+CbZ+A9pzuzZjDvSDb2D+GSeSp/mnqJ/N/Mtm3LLZgwOvxP2L6Rpn?=
 =?us-ascii?Q?9dY82yeOTvyo0OzfuX+RilBAGHazYTnByEw1Td6EPIurNHXHBJ5jqvyttNfE?=
 =?us-ascii?Q?3a0aKtrbY7W0f2BtWyZsjvEyx4WgZMRXSrsve9oroLyPTYhLrPYfz0fU6RBh?=
 =?us-ascii?Q?ztnCJpaX+UXFI4AEjl/zNAv2o9bsvcuw5R0e+Mgca/vmluaewDTLPq1C9SMG?=
 =?us-ascii?Q?3oXlxN31ffrJOV7vvOd7hoi7sYdvJ0R1PAt1HIoljNXXO5xNOS4P7P4m7fvt?=
 =?us-ascii?Q?Bt8JDdKkGa2ZnM9vaoZnuc7JbGkG6ym139vlQQ67o40F8zTnkclXLxp3Qbq7?=
 =?us-ascii?Q?1b22BlRxSUMHliTk46ei5hsts4mCku5RbWwl5RP5HQqqOk8Ok+c8Q1KxOqf/?=
 =?us-ascii?Q?vTlb8chlGcLR9n6/fUiJgsWacl+twx2IS1Nit32sODqf/qTZ6eFc8iwZmSKI?=
 =?us-ascii?Q?RtzYKqhUT4R4vFFmjMiaRklu3v75ckXdIFe7idSr6aZjZjwZKFz5N+Ovvm5r?=
 =?us-ascii?Q?yrykplINzv9Rvscjo0nnpX6NwwNgSLGmbcT9mKLvpDhuMW+v2CtzNPwoKix5?=
 =?us-ascii?Q?xGnCn08UMkpn3yoHUBSRrxJMC5CoEIMoQ6IVelbIpEQHErWMqjILCjC/5reb?=
 =?us-ascii?Q?b6uJH0uD5kg5tj417+gfBzYm8lHrLHOYe7Cysjo/w2C4WN9A1SpHhNJkobAJ?=
 =?us-ascii?Q?vpIeUQ5Isk0yijT4Y/d1xtk/nAWQEeUN4zQWMsCoj91cdLy4pM7PnRg9/2rR?=
 =?us-ascii?Q?VlCxCpBizINJAHZyIN8l0un5HokWL5tZ6LLsjDFHg3AIy8LkYbdFLv+u6z6Q?=
 =?us-ascii?Q?QErRmoAYrE88zGOgzJqp5Wr5GpJ5A8IeLdG4Wj/voBaDTQ9dK/SK+tRUIpAL?=
 =?us-ascii?Q?+177RqXU0v+42OeaBRKhx4anEUK8MsU6WWMBGkhIZIWRe7Uv6pZ2EP1PpLbm?=
 =?us-ascii?Q?eLzUayfwKX38TgbnUWuFljTGs4r4TNPSx/Wr+LE5xbcJbtfNFf9p8mDdSmEg?=
 =?us-ascii?Q?oOeps7fv3qA2xbYLSoDMeZn3re2X9glAaGdRAZO/L8m/nxa7EzTxvbbZ4Uzb?=
 =?us-ascii?Q?NvbWbnxGDPQ4DwS9wrn0qDFz9mCyqWn4nqYtLDrpMsnzgivnGiKFmUFK6M2J?=
 =?us-ascii?Q?I9cgO0oTwEgpH0djTQlFpOOVtbPeqtRN9Q8WEJCF44wOcvS1/HvUg1c85r2R?=
 =?us-ascii?Q?KziJTRzF7jNvCm6Il/+5SP2E5j5LWjwvCkKUkt2jhlGq4ikQpMNvZf1M2Mxy?=
 =?us-ascii?Q?3xrzXaFYH/OwKrgpKug=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DU0PR04MB9417.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 989c9a9b-e4ab-4202-3bf3-08dc5e118680
X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Apr 2024 12:34:14.3556
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: MJzuuUmO4Q6YToUmMLjhLYHCXjkbQ5mhNmdWL45pgoDtXK+v7qJSjQxXx3h9uN0LHt+daGoctVAsF00eGumJkw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM9PR04MB8812

> Subject: Re: [PATCH] firmware: arm_scmi: power_control: support suspend
> command
>=20
> On Tue, Apr 16, 2024 at 07:01:42AM +0000, Peng Fan wrote:
> > Hi Cristian,
> >
>=20
> Hi,
>=20
> > > 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.
>=20
> For the shutdown case, orderly_reboot related scheduled work ends up in a
> call to /sbin/reboot via usermodehelper kernel facilities, so that, depen=
ding
> 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 rea=
l final
> shutdown....this is done because the SCMI Notification is supposed to be =
a
> graceful request, so we dont just kernel_poweroff or similar.
>=20
> As an example with SystemD PID_1 /sbin/reboot is just a link to systemctl
> and you can place whatever you want in
>=20
> /usr/lib/systemd/system-shutdown/
>=20
> 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

>=20
> > > userspace, when triggering a system suspend with pm_suspend() we do
> > > not involve userspace in the process, but I dont think there is anoth=
er way
> indeed.
> >
> > Userspace process should not be involved, otherwise the freezing
> > process will never finish, and suspend will abort.
> >
>=20
> Similarly in the suspend case when you initiate it from userspace (system=
cl
> 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 c=
ase
> we are not passing through userspace and it seems there is no way to do i=
t,
> 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.
>=20
> 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 mor=
e.

Thanks for helping.

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

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

Thanks
Peng.
>=20
> Thanks,
> Cristian