Message ID | 20101219190828.A6C189D401C@zog.reactivated.net |
---|---|
State | Accepted |
Headers | show |
On Sunday, December 19, 2010, Daniel Drake wrote: > From: Paul Fox <pgf@laptop.org> > > rtc-cmos was setting suspend/resume hooks at the device_driver level. > However, the platform bus code (drivers/base/platform.c) only looks > for resume hooks at the dev_pm_ops level, or within the platform_driver. > > Switch rtc_cmos to use dev_pm_ops so that suspend/resume code is > executed again. > > Signed-off-by: Paul Fox <pgf@laptop.org> > Signed-off-by: Daniel Drake <dsd@laptop.org> Acked-by: Rafael J. Wysocki <rjw@sisk.pl> > --- > drivers/rtc/rtc-cmos.c | 16 +++++++++------- > 1 files changed, 9 insertions(+), 7 deletions(-) > > v2: incorporate feedback from Rafael J. Wysocki, fix tabs, make a bit more > consistent with typical SIMPLE_DEV_PM_OPS users. > > v3: remove const keyword already set by macro, thanks to Rafael > > diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c > index 5856167..dd8242d 100644 > --- a/drivers/rtc/rtc-cmos.c > +++ b/drivers/rtc/rtc-cmos.c > @@ -36,6 +36,7 @@ > #include <linux/platform_device.h> > #include <linux/mod_devicetable.h> > #include <linux/log2.h> > +#include <linux/pm.h> > > /* this is for "generic access to PC-style RTC" using CMOS_READ/CMOS_WRITE */ > #include <asm-generic/rtc.h> > @@ -850,7 +851,7 @@ static void __exit cmos_do_remove(struct device *dev) > > #ifdef CONFIG_PM > > -static int cmos_suspend(struct device *dev, pm_message_t mesg) > +static int cmos_suspend(struct device *dev) > { > struct cmos_rtc *cmos = dev_get_drvdata(dev); > unsigned char tmp; > @@ -898,7 +899,7 @@ static int cmos_suspend(struct device *dev, pm_message_t mesg) > */ > static inline int cmos_poweroff(struct device *dev) > { > - return cmos_suspend(dev, PMSG_HIBERNATE); > + return cmos_suspend(dev); > } > > static int cmos_resume(struct device *dev) > @@ -945,9 +946,9 @@ static int cmos_resume(struct device *dev) > return 0; > } > > +static SIMPLE_DEV_PM_OPS(cmos_pm_ops, cmos_suspend, cmos_resume); > + > #else > -#define cmos_suspend NULL > -#define cmos_resume NULL > > static inline int cmos_poweroff(struct device *dev) > { > @@ -1077,7 +1078,7 @@ static void __exit cmos_pnp_remove(struct pnp_dev *pnp) > > static int cmos_pnp_suspend(struct pnp_dev *pnp, pm_message_t mesg) > { > - return cmos_suspend(&pnp->dev, mesg); > + return cmos_suspend(&pnp->dev); > } > > static int cmos_pnp_resume(struct pnp_dev *pnp) > @@ -1157,8 +1158,9 @@ static struct platform_driver cmos_platform_driver = { > .shutdown = cmos_platform_shutdown, > .driver = { > .name = (char *) driver_name, > - .suspend = cmos_suspend, > - .resume = cmos_resume, > +#ifdef CONFIG_PM > + .pm = &cmos_pm_ops, > +#endif > } > }; > >
On Sun, 19 Dec 2010 19:08:28 +0000 (GMT) Daniel Drake <dsd@laptop.org> wrote: > From: Paul Fox <pgf@laptop.org> > > rtc-cmos was setting suspend/resume hooks at the device_driver level. > However, the platform bus code (drivers/base/platform.c) only looks > for resume hooks at the dev_pm_ops level, or within the platform_driver. > > Switch rtc_cmos to use dev_pm_ops so that suspend/resume code is > executed again. > > Signed-off-by: Paul Fox <pgf@laptop.org> > Signed-off-by: Daniel Drake <dsd@laptop.org> > --- > drivers/rtc/rtc-cmos.c | 16 +++++++++------- > 1 files changed, 9 insertions(+), 7 deletions(-) > > v2: incorporate feedback from Rafael J. Wysocki, fix tabs, make a bit more > consistent with typical SIMPLE_DEV_PM_OPS users. > > v3: remove const keyword already set by macro, thanks to Rafael It's unclear what the user-visible effects of this bug were. Machine fails to suspend? RTC loses its brains on resume? Something else? That's really important information for a bugfix's changelog. Please never omit it. I'm going to assume that whatever-the-behaviour-is is fairly serious, and that we want this patch in 2.6.37. So I tagged it for backporting into 2.6.37.1, as we're getting pretty close to 2.6.37. The patch also applies to 2.6.36. Is it needed there? And in earlier kernels?
andrew wrote: > On Sun, 19 Dec 2010 19:08:28 +0000 (GMT) > Daniel Drake <dsd@laptop.org> wrote: > > > From: Paul Fox <pgf@laptop.org> > > > > rtc-cmos was setting suspend/resume hooks at the device_driver level. > > However, the platform bus code (drivers/base/platform.c) only looks > > for resume hooks at the dev_pm_ops level, or within the platform_driver. > > > > Switch rtc_cmos to use dev_pm_ops so that suspend/resume code is > > executed again. > > > > Signed-off-by: Paul Fox <pgf@laptop.org> > > Signed-off-by: Daniel Drake <dsd@laptop.org> > > --- > > drivers/rtc/rtc-cmos.c | 16 +++++++++------- > > 1 files changed, 9 insertions(+), 7 deletions(-) > > > > v2: incorporate feedback from Rafael J. Wysocki, fix tabs, make a bit more > > consistent with typical SIMPLE_DEV_PM_OPS users. > > > > v3: remove const keyword already set by macro, thanks to Rafael > > It's unclear what the user-visible effects of this bug were. Machine > fails to suspend? RTC loses its brains on resume? Something else? the user visible symptom in our (XO laptop) case was that rtcwake would fail to wake the laptop. the RTC alarm would expire, but the wakeup wasn't unmasked. as for severity, the impact may have been reduced because if i recall correctly, the bug only affected platforms with CONFIG_PNP disabled. paul > > That's really important information for a bugfix's changelog. Please > never omit it. > > > > I'm going to assume that whatever-the-behaviour-is is fairly serious, > and that we want this patch in 2.6.37. So I tagged it for backporting > into 2.6.37.1, as we're getting pretty close to 2.6.37. > > The patch also applies to 2.6.36. Is it needed there? And in earlier > kernels? =--------------------- paul fox, pgf@laptop.org
On 22 December 2010 22:19, Andrew Morton <akpm@linux-foundation.org> wrote: > It's unclear what the user-visible effects of this bug were. Machine > fails to suspend? RTC loses its brains on resume? Something else? > > That's really important information for a bugfix's changelog. Please > never omit it. Sorry about that. After looking in more detail, I agree with Paul. This bug only affects setups where no PNP RTC is available, or where CONFIG_PNP is disabled. It also affects OLPC's coming-soon addition of an XO-specific RTC driver. So, a clear and simple fix, but not a wide-reaching bug in the first place. 2.6.38 would be fine, no real need for -stable backporting IMO. Thanks, Daniel
diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c index 5856167..dd8242d 100644 --- a/drivers/rtc/rtc-cmos.c +++ b/drivers/rtc/rtc-cmos.c @@ -36,6 +36,7 @@ #include <linux/platform_device.h> #include <linux/mod_devicetable.h> #include <linux/log2.h> +#include <linux/pm.h> /* this is for "generic access to PC-style RTC" using CMOS_READ/CMOS_WRITE */ #include <asm-generic/rtc.h> @@ -850,7 +851,7 @@ static void __exit cmos_do_remove(struct device *dev) #ifdef CONFIG_PM -static int cmos_suspend(struct device *dev, pm_message_t mesg) +static int cmos_suspend(struct device *dev) { struct cmos_rtc *cmos = dev_get_drvdata(dev); unsigned char tmp; @@ -898,7 +899,7 @@ static int cmos_suspend(struct device *dev, pm_message_t mesg) */ static inline int cmos_poweroff(struct device *dev) { - return cmos_suspend(dev, PMSG_HIBERNATE); + return cmos_suspend(dev); } static int cmos_resume(struct device *dev) @@ -945,9 +946,9 @@ static int cmos_resume(struct device *dev) return 0; } +static SIMPLE_DEV_PM_OPS(cmos_pm_ops, cmos_suspend, cmos_resume); + #else -#define cmos_suspend NULL -#define cmos_resume NULL static inline int cmos_poweroff(struct device *dev) { @@ -1077,7 +1078,7 @@ static void __exit cmos_pnp_remove(struct pnp_dev *pnp) static int cmos_pnp_suspend(struct pnp_dev *pnp, pm_message_t mesg) { - return cmos_suspend(&pnp->dev, mesg); + return cmos_suspend(&pnp->dev); } static int cmos_pnp_resume(struct pnp_dev *pnp) @@ -1157,8 +1158,9 @@ static struct platform_driver cmos_platform_driver = { .shutdown = cmos_platform_shutdown, .driver = { .name = (char *) driver_name, - .suspend = cmos_suspend, - .resume = cmos_resume, +#ifdef CONFIG_PM + .pm = &cmos_pm_ops, +#endif } };