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

[Greg KH]

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@xxxxxxx>, 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@xxxxxxx>

> 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.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/