[PATCH] PM / s2idle: Export s2idle_set_ops

[PATCH] PM / s2idle: Export s2idle_set_ops

[Claude Yen]

This series based on 5.9-rc1

As suspend_set_ops is exported in commit a5e4fd8783a2
("PM / Suspend: Export suspend_set_ops, suspend_valid_only_mem"),
exporting s2idle_set_ops to make kernel module setup s2idle ops too.

In this way, kernel module can hook platform suspend functions
regardless of Suspend-to-Ram(S2R) or Suspend-to-Idle(S2I)

*** BLURB HERE ***

Claude Yen (1):
  PM / s2idle: Export s2idle_set_ops

 kernel/power/suspend.c | 1 +
 1 file changed, 1 insertion(+)

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH] PM / s2idle: Export s2idle_set_ops

[Claude Yen]

As suspend_set_ops is exported in commit a5e4fd8783a2
("PM / Suspend: Export suspend_set_ops, suspend_valid_only_mem"),
exporting s2idle_set_ops to make kernel module setup s2idle ops too.

In this way, kernel module can hook platform suspend
functions regardless of Suspend-to-Ram(S2R) or
Suspend-to-Idle(S2I)

Signed-off-by: Claude Yen <claude.yen@mediatek.com>
---
 kernel/power/suspend.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 8b1bb5ee7e5d..0dc3e8344318 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -79,6 +79,7 @@ void s2idle_set_ops(const struct platform_s2idle_ops *ops)
 	s2idle_ops = ops;
 	unlock_system_sleep();
 }
+EXPORT_SYMBOL_GPL(s2idle_set_ops);
 
 static void s2idle_begin(void)
 {
-- 
2.18.0
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH] PM / s2idle: Export s2idle_set_ops

[Sudeep Holla]

On Thu, Oct 22, 2020 at 02:17:48PM +0800, Claude Yen wrote:
> As suspend_set_ops is exported in commit a5e4fd8783a2
> ("PM / Suspend: Export suspend_set_ops, suspend_valid_only_mem"),
> exporting s2idle_set_ops to make kernel module setup s2idle ops too.
> 
> In this way, kernel module can hook platform suspend
> functions regardless of Suspend-to-Ram(S2R) or
> Suspend-to-Idle(S2I)
>

If this is for arm64 platform, then NACK. You must use PSCI and it will
set the ops and it can't be module.

-- 
Regards,
Sudeep

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH] PM / s2idle: Export s2idle_set_ops

[claude yen]

On Thu, 2020-10-22 at 08:02 +0100, Sudeep Holla wrote:
> On Thu, Oct 22, 2020 at 02:17:48PM +0800, Claude Yen wrote:
> > As suspend_set_ops is exported in commit a5e4fd8783a2
> > ("PM / Suspend: Export suspend_set_ops, suspend_valid_only_mem"),
> > exporting s2idle_set_ops to make kernel module setup s2idle ops too.
> > 
> > In this way, kernel module can hook platform suspend
> > functions regardless of Suspend-to-Ram(S2R) or
> > Suspend-to-Idle(S2I)
> >
> 
> If this is for arm64 platform, then NACK. You must use PSCI and it will
> set the ops and it can't be module.
> 

PSCI uses suspend_set_ops instead. And suspend_set_ops has been
exported years ago.

Suspend-to_Idle(S2I) is another suspend method supported by linux
kernel. The corresponding s2idle_ops can be hooked by s2idle_set_ops
by underlying platforms.  For example, S2I is now introduced into
Mediatek SoC platforms. Besides, power management driver is built as
kernel module.

Mobile platforms are now call for kernel drivers to be kernel modules.
This could help drivers easier to migrate to newer linux kernel.
Ref: https://linuxplumbersconf.org/event/7/contributions/790/

Regards,
Claude 
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH] PM / s2idle: Export s2idle_set_ops

[Sudeep Holla]

On Fri, Oct 23, 2020 at 12:28:20PM +0800, claude yen wrote:
> On Thu, 2020-10-22 at 08:02 +0100, Sudeep Holla wrote:
> > On Thu, Oct 22, 2020 at 02:17:48PM +0800, Claude Yen wrote:
> > > As suspend_set_ops is exported in commit a5e4fd8783a2
> > > ("PM / Suspend: Export suspend_set_ops, suspend_valid_only_mem"),
> > > exporting s2idle_set_ops to make kernel module setup s2idle ops too.
> > > 
> > > In this way, kernel module can hook platform suspend
> > > functions regardless of Suspend-to-Ram(S2R) or
> > > Suspend-to-Idle(S2I)
> > >
> > 
> > If this is for arm64 platform, then NACK. You must use PSCI and it will
> > set the ops and it can't be module.
> > 
> 
> PSCI uses suspend_set_ops instead. And suspend_set_ops has been
> exported years ago.
> 
> Suspend-to_Idle(S2I) is another suspend method supported by linux
> kernel. The corresponding s2idle_ops can be hooked by s2idle_set_ops
> by underlying platforms.  For example, S2I is now introduced into
> Mediatek SoC platforms. Besides, power management driver is built as
> kernel module.
> 
> Mobile platforms are now call for kernel drivers to be kernel modules.
> This could help drivers easier to migrate to newer linux kernel.
> Ref: https://linuxplumbersconf.org/event/7/contributions/790/
> 

I understand that. But I am interested in looking at the module you want
to use this and how that interacts with PSCI. If this is arm64, you must
use PSCI for system suspend and cpu suspend. What does this module do on
top of those is what I want to know. Please post that module or point
me if it is already present in the tree.

-- 
Regards,
Sudeep

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH] PM / s2idle: Export s2idle_set_ops

[Rafael J. Wysocki]

On Fri, Oct 23, 2020 at 4:48 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, Oct 23, 2020 at 12:28:20PM +0800, claude yen wrote:
> > On Thu, 2020-10-22 at 08:02 +0100, Sudeep Holla wrote:
> > > On Thu, Oct 22, 2020 at 02:17:48PM +0800, Claude Yen wrote:
> > > > As suspend_set_ops is exported in commit a5e4fd8783a2
> > > > ("PM / Suspend: Export suspend_set_ops, suspend_valid_only_mem"),
> > > > exporting s2idle_set_ops to make kernel module setup s2idle ops too.
> > > >
> > > > In this way, kernel module can hook platform suspend
> > > > functions regardless of Suspend-to-Ram(S2R) or
> > > > Suspend-to-Idle(S2I)
> > > >
> > >
> > > If this is for arm64 platform, then NACK. You must use PSCI and it will
> > > set the ops and it can't be module.
> > >
> >
> > PSCI uses suspend_set_ops instead. And suspend_set_ops has been
> > exported years ago.
> >
> > Suspend-to_Idle(S2I) is another suspend method supported by linux
> > kernel. The corresponding s2idle_ops can be hooked by s2idle_set_ops
> > by underlying platforms.  For example, S2I is now introduced into
> > Mediatek SoC platforms. Besides, power management driver is built as
> > kernel module.
> >
> > Mobile platforms are now call for kernel drivers to be kernel modules.
> > This could help drivers easier to migrate to newer linux kernel.
> > Ref: https://linuxplumbersconf.org/event/7/contributions/790/
> >
>
> I understand that. But I am interested in looking at the module you want
> to use this and how that interacts with PSCI. If this is arm64, you must
> use PSCI for system suspend and cpu suspend. What does this module do on
> top of those is what I want to know. Please post that module or point
> me if it is already present in the tree.

Regardless, generally speaking, patches that export stuff to modules
without an in-the-tree user needing this are not applicable to the
mainline kernel source tree IMV.

Cheers!

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH] PM / s2idle: Export s2idle_set_ops

[Christoph Hellwig]

On Thu, Oct 22, 2020 at 02:17:47PM +0800, Claude Yen wrote:
> This series based on 5.9-rc1
> 
> As suspend_set_ops is exported in commit a5e4fd8783a2
> ("PM / Suspend: Export suspend_set_ops, suspend_valid_only_mem"),
> exporting s2idle_set_ops to make kernel module setup s2idle ops too.
> 
> In this way, kernel module can hook platform suspend functions
> regardless of Suspend-to-Ram(S2R) or Suspend-to-Idle(S2I)

Where is the actual users of the export?

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH] PM / s2idle: Export s2idle_set_ops

[claude yen]

On Fri, 2020-10-23 at 16:58 +0200, Rafael J. Wysocki wrote:
> On Fri, Oct 23, 2020 at 4:48 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Fri, Oct 23, 2020 at 12:28:20PM +0800, claude yen wrote:
> > > On Thu, 2020-10-22 at 08:02 +0100, Sudeep Holla wrote:
> > > > On Thu, Oct 22, 2020 at 02:17:48PM +0800, Claude Yen wrote:
> > > > > As suspend_set_ops is exported in commit a5e4fd8783a2
> > > > > ("PM / Suspend: Export suspend_set_ops, suspend_valid_only_mem"),
> > > > > exporting s2idle_set_ops to make kernel module setup s2idle ops too.
> > > > >
> > > > > In this way, kernel module can hook platform suspend
> > > > > functions regardless of Suspend-to-Ram(S2R) or
> > > > > Suspend-to-Idle(S2I)
> > > > >
> > > >
> > > > If this is for arm64 platform, then NACK. You must use PSCI and it will
> > > > set the ops and it can't be module.
> > > >
> > >
> > > PSCI uses suspend_set_ops instead. And suspend_set_ops has been
> > > exported years ago.
> > >
> > > Suspend-to_Idle(S2I) is another suspend method supported by linux
> > > kernel. The corresponding s2idle_ops can be hooked by s2idle_set_ops
> > > by underlying platforms.  For example, S2I is now introduced into
> > > Mediatek SoC platforms. Besides, power management driver is built as
> > > kernel module.
> > >
> > > Mobile platforms are now call for kernel drivers to be kernel modules.
> > > This could help drivers easier to migrate to newer linux kernel.
> > > Ref: https://linuxplumbersconf.org/event/7/contributions/790/
> > >
> >
> > I understand that. But I am interested in looking at the module you want
> > to use this and how that interacts with PSCI. If this is arm64, you must
> > use PSCI for system suspend and cpu suspend. What does this module do on
> > top of those is what I want to know. Please post that module or point
> > me if it is already present in the tree.
> 
> Regardless, generally speaking, patches that export stuff to modules
> without an in-the-tree user needing this are not applicable to the
> mainline kernel source tree IMV.
> 
> Cheers!


Thank for your feedbacks!

Indeed, there is no actual kernel module which uses s2idle_set_ops in
Mainline kernel right now.

However, Google recently ask SoC vendors to build drivers as kernel
modules for reducing migration efforts. For example, The power
management driver on Mediatek platform is now built as vendor module,
which has no plan to upstream so far. 

From Mainline kernel's perspective, I am wondering whether such vendor
modules are applicable to export APIs. Exporting APIs only at Google's
Android Common Kernel is an option, but this would make Android Kernel
and Mainline kernel much more diverse.

Regards,
Claude

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek