[PATCH v3] rtc-cmos: fix suspend/resume

[PATCH v3] rtc-cmos: fix suspend/resume

[Daniel Drake]

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

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
 	}
 };
 
-- 
1.7.3.3

Re: [PATCH v3] rtc-cmos: fix suspend/resume

[Rafael J. Wysocki]

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
>  	}
>  };
>  
> 

Re: [PATCH v3] rtc-cmos: fix suspend/resume

[Andrew Morton]

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?

Re: [PATCH v3] rtc-cmos: fix suspend/resume

[Paul Fox]

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

Re: [PATCH v3] rtc-cmos: fix suspend/resume

[Daniel Drake]

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