Re: [RFC][Update][PATCH 1/2] Introduce struct syscore_ops and related functionality

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Friday, March 11, 2011, Greg KH wrote:
> On Fri, Mar 11, 2011 at 09:13:13PM +0100, Rafael J. Wysocki wrote:
> > On Friday, March 11, 2011, Greg KH wrote:
> > > On Thu, Mar 10, 2011 at 12:30:45PM +0100, Rafael J. Wysocki wrote:
> > > > On Thursday, March 10, 2011, Alan Stern wrote:
> > > > > On Thu, 10 Mar 2011, Rafael J. Wysocki wrote:
> > > > > 
> > > > > > Some subsystems need to carry out suspend/resume and shutdown
> > > > > > operations with one CPU on-line and interrupts disabled.  The only
> > > > > > way to register such operations is to define a sysdev class and
> > > > > > a sysdev specifically for this purpose which is cumbersome and
> > > > > > inefficient.  Moreover, the arguments taken by sysdev suspend,
> > > > > > resume and shutdown callbacks are practically never necessary.
> > > > > > 
> > > > > > For this reason, introduce a simpler interface allowing subsystems
> > > > > > to register operations to be executed very late during system suspend
> > > > > > and shutdown and very early during resume in the form of
> > > > > > strcut syscore_ops objects.
> > > > > 
> > > > > ...
> > > > > 
> > > > > > Index: linux-2.6/drivers/base/syscore.c
> > > > > > ===================================================================
> > > > > > --- /dev/null
> > > > > > +++ linux-2.6/drivers/base/syscore.c
> > > > > 
> > > > > It's true that the existing sys.c file lies in drivers/base; this is
> > > > > presumably because it handles a bunch of class-related registration
> > > > > stuff.  Now you're getting rid of all that, leaving just the
> > > > > power-related operations, so doesn't it make more sense to put this
> > > > > file in drivers/base/power?
> > > > > 
> > > > > > +/**
> > > > > > + * syscore_suspend - Execute all the registered system core suspend callbacks.
> > > > > > + *
> > > > > > + * This function is executed with one CPU on-line and disabled interrupts.
> > > > > > + */
> > > > > > +int syscore_suspend(void)
> > > > > > +{
> > > > > > +	struct syscore_ops *ops;
> > > > > > +
> > > > > > +	list_for_each_entry_reverse(ops, &syscore_ops_list, node)
> > > > > > +		if (ops->suspend) {
> > > > > > +			int ret = ops->suspend();
> > > > > > +			if (ret) {
> > > > > > +				pr_err("PM: System core suspend callback "
> > > > > > +					"%pF failed.\n", ops->suspend);
> > > > > > +				return ret;
> > > > > 
> > > > > If an error occurs, you need to go back and resume all the things that
> > > > > were suspended.  At least, that's what the code in sysdev_suspend does.
> > > > > 
> > > > > > +			}
> > > > > > +		}
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > 
> > > > Below is a new version of the patch.  I've taken your comment on the failing
> > > > suspend into account, fix the list traversal direction in syscore_shutdown()
> > > > and added some debug statements.
> > > > 
> > > > Thanks,
> > > > Rafael
> > > > 
> > > > ---
> > > > Some subsystems need to carry out suspend/resume and shutdown
> > > > operations with one CPU on-line and interrupts disabled.  The only
> > > > way to register such operations is to define a sysdev class and
> > > > a sysdev specifically for this purpose which is cumbersome and
> > > > inefficient.  Moreover, the arguments taken by sysdev suspend,
> > > > resume and shutdown callbacks are practically never necessary.
> > > > 
> > > > For this reason, introduce a simpler interface allowing subsystems
> > > > to register operations to be executed very late during system suspend
> > > > and shutdown and very early during resume in the form of
> > > > strcut syscore_ops objects.
> > > > 
> > > > ---
> > > >  drivers/base/Makefile       |    2 
> > > >  drivers/base/syscore.c      |  107 ++++++++++++++++++++++++++++++++++++++++++++
> > > >  include/linux/syscore_ops.h |   29 +++++++++++
> > > >  kernel/power/hibernate.c    |    9 +++
> > > >  kernel/power/suspend.c      |    4 +
> > > >  kernel/sys.c                |    4 +
> > > >  6 files changed, 154 insertions(+), 1 deletion(-)
> > > > 
> > > > Index: linux-2.6/include/linux/syscore_ops.h
> > > > ===================================================================
> > > > --- /dev/null
> > > > +++ linux-2.6/include/linux/syscore_ops.h
> > > > @@ -0,0 +1,29 @@
> > > > +/*
> > > > + *  syscore_ops.h - System core operations.
> > > > + *
> > > > + *  Copyright (C) 2011 Rafael J. Wysocki <rjw@sisk.pl>, Novell Inc.
> > > > + *
> > > > + *  This file is released under the GPLv2.
> > > > + */
> > > > +
> > > > +#ifndef _LINUX_SYSCORE_OPS_H
> > > > +#define _LINUX_SYSCORE_OPS_H
> > > > +
> > > > +#include <linux/list.h>
> > > > +
> > > > +struct syscore_ops {
> > > > +	struct list_head node;
> > > > +	int (*suspend)(void);
> > > > +	void (*resume)(void);
> > > > +	void (*shutdown)(void);
> > > > +};
> > > > +
> > > > +extern void register_syscore_ops(struct syscore_ops *ops);
> > > > +extern void unregister_syscore_ops(struct syscore_ops *ops);
> > > > +#ifdef CONFIG_PM_SLEEP
> > > > +extern int syscore_suspend(void);
> > > > +extern void syscore_resume(void);
> > > > +#endif
> > > 
> > > Minor nit, provide inline functions for these when CONFIG_PM_SLEEP is
> > > not defined so the code still builds?
> > 
> > The code using them depends on CONFIG_PM_SLEEP and they are nobody else's
> > business. :-)
> 
> Ah, ok.
> 
> > I could avoid using the #ifdef here, but I thought I'd make it clear that
> > these things were only available when CONFIG_PM_SLEEP was set.
> 
> That's fine.
> 
> > > Other than that, this looks great to me, thanks for doing this.
> > 
> > No problem. :-)
> > 
> > > Do you want me to take it through my tree, or yours?
> > 
> > I can handle it if you give me an ack.
> 
> Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
> 
> > Do you think I should push [1/2] alone for 2.6.39 or wait for the patches
> > converting subsystems to use this stuff to be ready?  I think it'll take
> > some time to prepare them, especialy for things in the ARM tree that use
> > sysdevs in some interesting ways ...
> 
> Send it for .39, and then start converting everyone over to using it.
> It's easier once the code is in place to handle the different trees,
> that way you don't have to worry about ordering issues.

OK, I will.

Thanks,
Rafael