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

[Greg KH]

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?

Other than that, this looks great to me, thanks for doing this.  Do you
want me to take it through my tree, or yours?

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/