All of lore.kernel.org
 help / color / mirror / Atom feed
From: Peng Fan <peng.fan@oss.nxp.com>
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: Peng Fan <peng.fan@nxp.com>, Dhruva Gole <d-gole@ti.com>,
	Cristian Marussi <cristian.marussi@arm.com>,
	"arm-scmi@vger.kernel.org" <arm-scmi@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>,
	Chuck Cannon <chuck.cannon@nxp.com>
Subject: Re: [PATCH 2/2] firmware: arm_scmi: power_control: Set SCMI_SYSPOWER_IDLE in pm resume
Date: Tue, 1 Jul 2025 23:07:35 +0800	[thread overview]
Message-ID: <20250701150735.GD20538@nxa18884-linux> (raw)
In-Reply-To: <20250624145807.GA14878@nxa18884-linux>

Hi Sudeep,

On Tue, Jun 24, 2025 at 10:58:07PM +0800, Peng Fan wrote:
>On Tue, Jun 24, 2025 at 11:21:52AM +0100, Sudeep Holla wrote:
>>On Tue, Jun 24, 2025 at 01:23:10AM +0000, Peng Fan wrote:
>>> > 
>>> > Just to summarise my understanding here at very high level, the issue
>>> > exists as the second notification by an agent to the Linux to suspend
>>> > the system wakes up the system from suspend state. Since the
>>> > interrupts are enabled before the thaw_processes() (which eventually
>>> > continues the execution of scmi_suspend_work_func() to set the state
>>> > to SCMI_SYSPOWER_IDLE, the scmi_userspace_notifier() is executed
>>> > much before and ends up ignoring the request as the state is still not
>>> > set to SCMI_SYSPOWER_IDLE. There is a race which your patch is
>>> > addressing.
>>> 
>>> Thanks for writing this down, It is very correct and clear.
>>> 
>>
>>While I am not against adding bus PM ops as it can be useful elsewhere,
>>just wonder if this usecase is a good use of it. Does setting the state
>>before the pm_suspend() call suffice. I still need to think through the
>>possible race with that solution, but just asking you to check if that
>
>There is race condition if setting the state to SCMI_SYSPOWER_IDLE before
>pm_suspend.
>
>The 2nd suspend notification could runs into pm_suspend again
>before pm_suspend update system_state to SYSTEM_SUSPEND, if my understanding
>is correct.
>
>Per pm_suspend->enter_state,
>"Make sure that no one else is trying to put the system into a sleep state",
>not sure, but I think better not let pm_suspend to handle the race condition.
>
>Since syspower only has one per system(linux), the other approach is to
>use syscore, but need a global variable for state in scmi_power_control.c,
>because syscore_suspend/resume does not have parameter.
>
>we need to set state back to IDLE after linux wakeup and before kernel
>thread scheduling. I only see two interfaces to achieve:
>PM ops or syscore ops.

Not sure you have time to give a look. I plan to post V2 later this
week. In V2, I would still use pm ops.

Thanks,
Peng

>
>Thanks,
>Peng
>
>
>>helps.
>>
>>-- 
>>Regards,
>>Sudeep

  reply	other threads:[~2025-07-01 13:57 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-06-20  3:37 [PATCH 0/2] firmware: arm_scmi: add pm ops for scmi_power_control Peng Fan (OSS)
2025-06-20  3:37 ` [PATCH 1/2] firmware: arm_scmi: bus: Add pm ops Peng Fan (OSS)
2025-06-20  3:55   ` Dan Carpenter
2025-06-20  5:21     ` Peng Fan
2025-06-20  3:37 ` [PATCH 2/2] firmware: arm_scmi: power_control: Set SCMI_SYSPOWER_IDLE in pm resume Peng Fan (OSS)
2025-06-20 17:40   ` kernel test robot
2025-06-23 12:57   ` Dhruva Gole
2025-06-23 14:29     ` Peng Fan
2025-06-23 15:04       ` Cristian Marussi
2025-06-23 16:27       ` Sudeep Holla
2025-06-24  1:23         ` Peng Fan
2025-06-24 10:21           ` Sudeep Holla
2025-06-24 14:58             ` Peng Fan
2025-07-01 15:07               ` Peng Fan [this message]
2025-07-02 15:26                 ` Sudeep Holla
2025-06-23 14:48 ` [PATCH 0/2] firmware: arm_scmi: add pm ops for scmi_power_control Cristian Marussi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20250701150735.GD20538@nxa18884-linux \
    --to=peng.fan@oss.nxp.com \
    --cc=arm-scmi@vger.kernel.org \
    --cc=chuck.cannon@nxp.com \
    --cc=cristian.marussi@arm.com \
    --cc=d-gole@ti.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peng.fan@nxp.com \
    --cc=ranjani.vaidyanathan@nxp.com \
    --cc=sudeep.holla@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.