From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753985Ab1CKUNS (ORCPT ); Fri, 11 Mar 2011 15:13:18 -0500 Received: from ogre.sisk.pl ([217.79.144.158]:52171 "EHLO ogre.sisk.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753031Ab1CKUNM (ORCPT ); Fri, 11 Mar 2011 15:13:12 -0500 From: "Rafael J. Wysocki" To: Greg KH Subject: Re: [RFC][Update][PATCH 1/2] Introduce struct syscore_ops and related functionality Date: Fri, 11 Mar 2011 21:13:13 +0100 User-Agent: KMail/1.13.5 (Linux/2.6.38-rc8+; KDE/4.4.4; x86_64; ; ) Cc: Alan Stern , LKML , Jesse Barnes , mingo@redhat.com, "H. Peter Anvin" , Kay Sievers , Linux PM mailing list , tglx@linutronix.de References: <201103101230.45569.rjw@sisk.pl> <20110311171131.GA10437@suse.de> In-Reply-To: <20110311171131.GA10437@suse.de> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201103112113.13807.rjw@sisk.pl> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 , Novell Inc. > > + * > > + * This file is released under the GPLv2. > > + */ > > + > > +#ifndef _LINUX_SYSCORE_OPS_H > > +#define _LINUX_SYSCORE_OPS_H > > + > > +#include > > + > > +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. :-) 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. > 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. 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 ... Thanks, Rafael