[rtc-linux] [RFC PATCH v2 0/2] rtc: Prevent the automatic reboot after powering off the system

[rtc-linux] [RFC PATCH v2 0/2] rtc: Prevent the automatic reboot after powering off the system

[Adrian Huang]

Commit d5a1c7e3fc38 ("rtc-cmos: Add an alarm disable quirk") added the
alarm disable quirk to work around the broken BIOSes. The same symptom
is observed on Lenovo Thinkserver. Instead of adding the impacted
servers to the "alarm disable" quirk, this patchset aims at preventing
the issue so that the commit can be reverted. 

The brief description of the root cause is that: When configuring 
the RTC registers to setup a wake up event in the S5 handler (BIOS),
the handler does not clear the interrupt flag bits by reading 
Register C of CMOS RTC.

Changes from v1:
  * Change the order of [PATCH 1/2] and [PATCH 2/2] (per Borislav)
  * Drop the approach "RTC alarm timer restoration approach". Instead,
    [PATCH 1/2] makes sure that AF is cleared in cmos_do_shutdown()
    before powering off the machine. (Suggested by Alexandre)

Adrian Huang (2):
  rtc-cmos: Clear interrupt flag if alarm time is less equal to now+1
    seconds
  rtc-cmos: Revert "rtc-cmos: Add an alarm disable quirk"

 drivers/rtc/rtc-cmos.c | 113 ++++++++++++++++++++++++++-----------------------
 1 file changed, 59 insertions(+), 54 deletions(-)

-- 
1.9.1

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] [RFC PATCH v2 1/2] rtc-cmos: Clear interrupt flag if alarm time is

[Adrian Huang]

Steps to reproduce the problem:
	1) Enable RTC wake-up option in BIOS Setup
	2) Issue one of these commands in the OS: "poweroff"
	   or "shutdown -h now"
	3) System will shut down and then reboot automatically

Root-cause of the issue:
	1) During the shutdown process, the hwclock utility is used
	   to save the system clock to hardware clock (RTC).
	2) The hwclock utility invokes ioctl() with RTC_UIE_ON. The
	   kernel configures the RTC alarm for the periodic interrupt
	   (every 1 second).
	3) The hwclock uitlity closes the /dev/rtc0 device, and the
	   kernel disables the RTC alarm irq (AIE bit of Register B)
	   via ioctl() with RTC_UIE_OFF. But, the configured alarm
	   time is the current_time + 1.
	4) After the next 1 second is elapsed, the AF (alarm
	   interrupt flag) of Register C is set.
	5) The S5 handler in BIOS is invoked to configure alarm
	   registers (enable AIE bit and configure alarm date/time).
	   But, BIOS does not clear the previous interrupt status
	   during alarm configuration. Therefore, "AF=AIE=1" causes
	   the rtc device to trigger an interrupt.
	6) So, the machine reboots automatically right after shutdown.

This patch makes sure cmos_do_shutdown() is invoked if the configured
alarm time is less equal to current_time + 1 seconds. Therefore,
the interrupt flag can be cleared before powering off the system.

The member 'alarm_expires' is introduced in struct cmos_rtc because
of the following reasons:
	1) The configured alarm time can be retrieved from
	   cmos_read_alarm(), but we need to take the 'wrapped
	   timestamp' and 'time rollover' into consideration. The
	   function __rtc_read_alarm() eliminates the concerns. To
	   avoid the duplicated code in the lower level RTC driver,
	   invoking __rtc_read_alarm from the lower level RTC driver
	   is not encouraged. Moreover, the compilation error 'the
	   undefined __rtc_read_alarm" is observed if the lower level
	   RTC driver is compiled as a kernel module.
	2) The uie_rtctimer.node.expires and aie_timer.node.expires can
	   be retrieved for the configured alarm time. But, the problem
	   is that either of them might configure the CMOS alarm time.
	   We cannot make sure who configured the CMOS alarm time
	   before (uie_rtctimer or aie_timer is enabled and then is
	   disabled).
	3) The patch introduces the member 'alarm_expires' to keep the
	   newly configured alarm time, so the above-mentioned concerns
	   can be eliminated.

The issue goes away after 20-time shutdown tests.

Signed-off-by: Adrian Huang <ahuang12@lenovo.com>
Reviewed-by: Max Asbock <amax@lenovo.com>
---
 drivers/rtc/rtc-cmos.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 60 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index a82556a..4e60ac8 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -51,6 +51,7 @@ struct cmos_rtc {
 	struct device		*dev;
 	int			irq;
 	struct resource		*iomem;
+	time64_t		alarm_expires;
 
 	void			(*wake_on)(struct device *);
 	void			(*wake_off)(struct device *);
@@ -377,6 +378,8 @@ static int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 
 	spin_unlock_irq(&rtc_lock);
 
+	cmos->alarm_expires = rtc_tm_to_time64(&t->time);
+
 	return 0;
 }
 
@@ -588,7 +591,7 @@ static struct bin_attribute nvram = {
 
 /*----------------------------------------------------------------*/
 
-static struct cmos_rtc	cmos_rtc;
+static struct cmos_rtc	cmos_rtc = { .alarm_expires = 0 };
 
 static irqreturn_t cmos_interrupt(int irq, void *p)
 {
@@ -860,6 +863,52 @@ static void __exit cmos_do_remove(struct device *dev)
 	cmos->dev = NULL;
 }
 
+static int cmos_aie_poweroff(struct device *dev)
+{
+	struct cmos_rtc	*cmos = dev_get_drvdata(dev);
+	struct rtc_time now;
+	int err;
+	time64_t t_now;
+	unsigned char rtc_control;
+
+	if (!cmos->alarm_expires)
+		return -EINVAL;
+
+	spin_lock_irq(&rtc_lock);
+	rtc_control = CMOS_READ(RTC_CONTROL);
+	spin_unlock_irq(&rtc_lock);
+
+	/* We only care about the situation where AIE is disabled. */
+	if (rtc_control & RTC_AIE)
+		return -EBUSY;
+
+	err = cmos_read_time(dev, &now);
+	if (err < 0)
+		return err;
+	t_now = rtc_tm_to_time64(&now);
+
+	/*
+	 * When enabling "RTC wake-up" in BIOS setup, the machine reboots
+	 * automatically right after shutdown on some buggy boxes.
+	 * This automatic rebooting issue won't happen when the alarm
+	 * time is larger than now+1 seconds.
+	 *
+	 * If the alarm time is equal to now+1 seconds, we need to wait
+	 * for 1 second so that AF is set by CMOS hardware. Therefore, we
+	 * can get a chance to clear AF in cmos_do_shutdown().
+	 *
+	 * If the alarm time is less equal to 'now', the function simply
+	 * returns '0' so that we can get a chance to clear AF in
+	 * cmos_do_shutdown().
+	 */
+	if (cmos->alarm_expires > (t_now + 1))
+		return -EBUSY;
+	else if (cmos->alarm_expires == (t_now + 1))
+		msleep(1000);
+
+	return 0;
+}
+
 #ifdef CONFIG_PM
 
 static int cmos_suspend(struct device *dev)
@@ -1094,8 +1143,11 @@ static void cmos_pnp_shutdown(struct pnp_dev *pnp)
 	struct device *dev = &pnp->dev;
 	struct cmos_rtc	*cmos = dev_get_drvdata(dev);
 
-	if (system_state == SYSTEM_POWER_OFF && !cmos_poweroff(dev))
-		return;
+	if (system_state == SYSTEM_POWER_OFF) {
+		cmos_poweroff(dev);
+		if (cmos_aie_poweroff(dev) < 0)
+			return;
+	}
 
 	cmos_do_shutdown(cmos->irq);
 }
@@ -1200,8 +1252,11 @@ static void cmos_platform_shutdown(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct cmos_rtc	*cmos = dev_get_drvdata(dev);
 
-	if (system_state == SYSTEM_POWER_OFF && !cmos_poweroff(dev))
-		return;
+	if (system_state == SYSTEM_POWER_OFF) {
+		cmos_poweroff(dev);
+		if (cmos_aie_poweroff(dev) < 0)
+			return;
+	}
 
 	cmos_do_shutdown(cmos->irq);
 }
-- 
1.9.1

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] [RFC PATCH v2 2/2] rtc-cmos: Revert "rtc-cmos: Add an alarm disable quirk"

[Adrian Huang]

Commit d5a1c7e3fc38 ("rtc-cmos: Add an alarm disable quirk") that
added a special quirk is not needed because [PATCH 1/2] of this
patchset makes the kernel more robust:
rtc-cmos: Clear interrupt flag if alarm time is less equal to
now+1 seconds.

Signed-off-by: Adrian Huang <ahuang12@lenovo.com>
---
 drivers/rtc/rtc-cmos.c | 50 --------------------------------------------------
 1 file changed, 50 deletions(-)

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 4e60ac8..6064f29 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -41,7 +41,6 @@
 #include <linux/pm.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
-#include <linux/dmi.h>
 
 /* this is for "generic access to PC-style RTC" using CMOS_READ/CMOS_WRITE */
 #include <asm-generic/rtc.h>
@@ -383,50 +382,6 @@ static int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 	return 0;
 }
 
-/*
- * Do not disable RTC alarm on shutdown - workaround for b0rked BIOSes.
- */
-static bool alarm_disable_quirk;
-
-static int __init set_alarm_disable_quirk(const struct dmi_system_id *id)
-{
-	alarm_disable_quirk = true;
-	pr_info("BIOS has alarm-disable quirk - RTC alarms disabled\n");
-	return 0;
-}
-
-static const struct dmi_system_id rtc_quirks[] __initconst = {
-	/* https://bugzilla.novell.com/show_bug.cgi?id=805740 */
-	{
-		.callback = set_alarm_disable_quirk,
-		.ident    = "IBM Truman",
-		.matches  = {
-			DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
-			DMI_MATCH(DMI_PRODUCT_NAME, "4852570"),
-		},
-	},
-	/* https://bugzilla.novell.com/show_bug.cgi?id=812592 */
-	{
-		.callback = set_alarm_disable_quirk,
-		.ident    = "Gigabyte GA-990XA-UD3",
-		.matches  = {
-			DMI_MATCH(DMI_SYS_VENDOR,
-					"Gigabyte Technology Co., Ltd."),
-			DMI_MATCH(DMI_PRODUCT_NAME, "GA-990XA-UD3"),
-		},
-	},
-	/* http://permalink.gmane.org/gmane.linux.kernel/1604474 */
-	{
-		.callback = set_alarm_disable_quirk,
-		.ident    = "Toshiba Satellite L300",
-		.matches  = {
-			DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
-			DMI_MATCH(DMI_PRODUCT_NAME, "Satellite L300"),
-		},
-	},
-	{}
-};
-
 static int cmos_alarm_irq_enable(struct device *dev, unsigned int enabled)
 {
 	struct cmos_rtc	*cmos = dev_get_drvdata(dev);
@@ -435,9 +390,6 @@ static int cmos_alarm_irq_enable(struct device *dev, unsigned int enabled)
 	if (!is_valid_irq(cmos->irq))
 		return -EINVAL;
 
-	if (alarm_disable_quirk)
-		return 0;
-
 	spin_lock_irqsave(&rtc_lock, flags);
 
 	if (enabled)
@@ -1298,8 +1250,6 @@ static int __init cmos_init(void)
 			platform_driver_registered = true;
 	}
 
-	dmi_check_system(rtc_quirks);
-
 	if (retval == 0)
 		return 0;
 
-- 
1.9.1

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [RFC PATCH v2 1/2] rtc-cmos: Clear interrupt flag if alarm time is

[Huang Adrian]

Hi Alexandre,

This reworked patch is based on your suggestion. Could you kindly
review it? If it looks good to you, I'll ask Borislav, Diego and
Egbert a favor for testing.

Thanks in advance.

-- Adrian

> ---
>  drivers/rtc/rtc-cmos.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 60 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
> index a82556a..4e60ac8 100644
> --- a/drivers/rtc/rtc-cmos.c
> +++ b/drivers/rtc/rtc-cmos.c
> @@ -51,6 +51,7 @@ struct cmos_rtc {
>         struct device           *dev;
>         int                     irq;
>         struct resource         *iomem;
> +       time64_t                alarm_expires;
>
>         void                    (*wake_on)(struct device *);
>         void                    (*wake_off)(struct device *);
> @@ -377,6 +378,8 @@ static int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
>
>         spin_unlock_irq(&rtc_lock);
>
> +       cmos->alarm_expires = rtc_tm_to_time64(&t->time);
> +
>         return 0;
>  }
>
> @@ -588,7 +591,7 @@ static struct bin_attribute nvram = {
>
>  /*----------------------------------------------------------------*/
>
> -static struct cmos_rtc cmos_rtc;
> +static struct cmos_rtc cmos_rtc = { .alarm_expires = 0 };
>
>  static irqreturn_t cmos_interrupt(int irq, void *p)
>  {
> @@ -860,6 +863,52 @@ static void __exit cmos_do_remove(struct device *dev)
>         cmos->dev = NULL;
>  }
>
> +static int cmos_aie_poweroff(struct device *dev)
> +{
> +       struct cmos_rtc *cmos = dev_get_drvdata(dev);
> +       struct rtc_time now;
> +       int err;
> +       time64_t t_now;
> +       unsigned char rtc_control;
> +
> +       if (!cmos->alarm_expires)
> +               return -EINVAL;
> +
> +       spin_lock_irq(&rtc_lock);
> +       rtc_control = CMOS_READ(RTC_CONTROL);
> +       spin_unlock_irq(&rtc_lock);
> +
> +       /* We only care about the situation where AIE is disabled. */
> +       if (rtc_control & RTC_AIE)
> +               return -EBUSY;
> +
> +       err = cmos_read_time(dev, &now);
> +       if (err < 0)
> +               return err;
> +       t_now = rtc_tm_to_time64(&now);
> +
> +       /*
> +        * When enabling "RTC wake-up" in BIOS setup, the machine reboots
> +        * automatically right after shutdown on some buggy boxes.
> +        * This automatic rebooting issue won't happen when the alarm
> +        * time is larger than now+1 seconds.
> +        *
> +        * If the alarm time is equal to now+1 seconds, we need to wait
> +        * for 1 second so that AF is set by CMOS hardware. Therefore, we
> +        * can get a chance to clear AF in cmos_do_shutdown().
> +        *
> +        * If the alarm time is less equal to 'now', the function simply
> +        * returns '0' so that we can get a chance to clear AF in
> +        * cmos_do_shutdown().
> +        */
> +       if (cmos->alarm_expires > (t_now + 1))
> +               return -EBUSY;
> +       else if (cmos->alarm_expires == (t_now + 1))
> +               msleep(1000);
> +
> +       return 0;
> +}
> +
>  #ifdef CONFIG_PM
>
>  static int cmos_suspend(struct device *dev)
> @@ -1094,8 +1143,11 @@ static void cmos_pnp_shutdown(struct pnp_dev *pnp)
>         struct device *dev = &pnp->dev;
>         struct cmos_rtc *cmos = dev_get_drvdata(dev);
>
> -       if (system_state == SYSTEM_POWER_OFF && !cmos_poweroff(dev))
> -               return;
> +       if (system_state == SYSTEM_POWER_OFF) {
> +               cmos_poweroff(dev);
> +               if (cmos_aie_poweroff(dev) < 0)
> +                       return;
> +       }
>
>         cmos_do_shutdown(cmos->irq);
>  }
> @@ -1200,8 +1252,11 @@ static void cmos_platform_shutdown(struct platform_device *pdev)
>         struct device *dev = &pdev->dev;
>         struct cmos_rtc *cmos = dev_get_drvdata(dev);
>
> -       if (system_state == SYSTEM_POWER_OFF && !cmos_poweroff(dev))
> -               return;
> +       if (system_state == SYSTEM_POWER_OFF) {
> +               cmos_poweroff(dev);
> +               if (cmos_aie_poweroff(dev) < 0)
> +                       return;
> +       }
>
>         cmos_do_shutdown(cmos->irq);
>  }
> --
> 1.9.1
>

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [RFC PATCH v2 1/2] rtc-cmos: Clear interrupt flag if alarm time is

[Diego Ercolani]

[-- Attachment #1: Type: text/plain, Size: 5158 bytes --]

No problem to me to test, please give a build for openSUSE_13.2 so I can
test on my system
Thank you

2015-06-10 7:51 GMT+02:00 Huang Adrian <adrianhuang0701@gmail.com>:

> Hi Alexandre,
>
> This reworked patch is based on your suggestion. Could you kindly
> review it? If it looks good to you, I'll ask Borislav, Diego and
> Egbert a favor for testing.
>
> Thanks in advance.
>
> -- Adrian
>
> > ---
> >  drivers/rtc/rtc-cmos.c | 65
> ++++++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 60 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
> > index a82556a..4e60ac8 100644
> > --- a/drivers/rtc/rtc-cmos.c
> > +++ b/drivers/rtc/rtc-cmos.c
> > @@ -51,6 +51,7 @@ struct cmos_rtc {
> >         struct device           *dev;
> >         int                     irq;
> >         struct resource         *iomem;
> > +       time64_t                alarm_expires;
> >
> >         void                    (*wake_on)(struct device *);
> >         void                    (*wake_off)(struct device *);
> > @@ -377,6 +378,8 @@ static int cmos_set_alarm(struct device *dev, struct
> rtc_wkalrm *t)
> >
> >         spin_unlock_irq(&rtc_lock);
> >
> > +       cmos->alarm_expires = rtc_tm_to_time64(&t->time);
> > +
> >         return 0;
> >  }
> >
> > @@ -588,7 +591,7 @@ static struct bin_attribute nvram = {
> >
> >  /*----------------------------------------------------------------*/
> >
> > -static struct cmos_rtc cmos_rtc;
> > +static struct cmos_rtc cmos_rtc = { .alarm_expires = 0 };
> >
> >  static irqreturn_t cmos_interrupt(int irq, void *p)
> >  {
> > @@ -860,6 +863,52 @@ static void __exit cmos_do_remove(struct device
> *dev)
> >         cmos->dev = NULL;
> >  }
> >
> > +static int cmos_aie_poweroff(struct device *dev)
> > +{
> > +       struct cmos_rtc *cmos = dev_get_drvdata(dev);
> > +       struct rtc_time now;
> > +       int err;
> > +       time64_t t_now;
> > +       unsigned char rtc_control;
> > +
> > +       if (!cmos->alarm_expires)
> > +               return -EINVAL;
> > +
> > +       spin_lock_irq(&rtc_lock);
> > +       rtc_control = CMOS_READ(RTC_CONTROL);
> > +       spin_unlock_irq(&rtc_lock);
> > +
> > +       /* We only care about the situation where AIE is disabled. */
> > +       if (rtc_control & RTC_AIE)
> > +               return -EBUSY;
> > +
> > +       err = cmos_read_time(dev, &now);
> > +       if (err < 0)
> > +               return err;
> > +       t_now = rtc_tm_to_time64(&now);
> > +
> > +       /*
> > +        * When enabling "RTC wake-up" in BIOS setup, the machine reboots
> > +        * automatically right after shutdown on some buggy boxes.
> > +        * This automatic rebooting issue won't happen when the alarm
> > +        * time is larger than now+1 seconds.
> > +        *
> > +        * If the alarm time is equal to now+1 seconds, we need to wait
> > +        * for 1 second so that AF is set by CMOS hardware. Therefore, we
> > +        * can get a chance to clear AF in cmos_do_shutdown().
> > +        *
> > +        * If the alarm time is less equal to 'now', the function simply
> > +        * returns '0' so that we can get a chance to clear AF in
> > +        * cmos_do_shutdown().
> > +        */
> > +       if (cmos->alarm_expires > (t_now + 1))
> > +               return -EBUSY;
> > +       else if (cmos->alarm_expires == (t_now + 1))
> > +               msleep(1000);
> > +
> > +       return 0;
> > +}
> > +
> >  #ifdef CONFIG_PM
> >
> >  static int cmos_suspend(struct device *dev)
> > @@ -1094,8 +1143,11 @@ static void cmos_pnp_shutdown(struct pnp_dev *pnp)
> >         struct device *dev = &pnp->dev;
> >         struct cmos_rtc *cmos = dev_get_drvdata(dev);
> >
> > -       if (system_state == SYSTEM_POWER_OFF && !cmos_poweroff(dev))
> > -               return;
> > +       if (system_state == SYSTEM_POWER_OFF) {
> > +               cmos_poweroff(dev);
> > +               if (cmos_aie_poweroff(dev) < 0)
> > +                       return;
> > +       }
> >
> >         cmos_do_shutdown(cmos->irq);
> >  }
> > @@ -1200,8 +1252,11 @@ static void cmos_platform_shutdown(struct
> platform_device *pdev)
> >         struct device *dev = &pdev->dev;
> >         struct cmos_rtc *cmos = dev_get_drvdata(dev);
> >
> > -       if (system_state == SYSTEM_POWER_OFF && !cmos_poweroff(dev))
> > -               return;
> > +       if (system_state == SYSTEM_POWER_OFF) {
> > +               cmos_poweroff(dev);
> > +               if (cmos_aie_poweroff(dev) < 0)
> > +                       return;
> > +       }
> >
> >         cmos_do_shutdown(cmos->irq);
> >  }
> > --
> > 1.9.1
> >
>

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: Type: text/html, Size: 7175 bytes --]

[rtc-linux] Re: [RFC PATCH v2 1/2] rtc-cmos: Clear interrupt flag if alarm time is

[Alexandre Belloni]

Hi,

On 09/06/2015 at 18:23:32 +0800, Adrian Huang wrote :
> @@ -588,7 +591,7 @@ static struct bin_attribute nvram = {
>  
>  /*----------------------------------------------------------------*/
>  
> -static struct cmos_rtc	cmos_rtc;
> +static struct cmos_rtc	cmos_rtc = { .alarm_expires = 0 };
>  

That is probably unnecessary as the whole struct is already zeroed at
initialization.

>  static irqreturn_t cmos_interrupt(int irq, void *p)
>  {
> @@ -860,6 +863,52 @@ static void __exit cmos_do_remove(struct device *dev)
>  	cmos->dev = NULL;
>  }
>  
> +static int cmos_aie_poweroff(struct device *dev)
> +{
> +	struct cmos_rtc	*cmos = dev_get_drvdata(dev);
> +	struct rtc_time now;
> +	int err;
> +	time64_t t_now;
> +	unsigned char rtc_control;
> +
> +	if (!cmos->alarm_expires)
> +		return -EINVAL;
> +
> +	spin_lock_irq(&rtc_lock);
> +	rtc_control = CMOS_READ(RTC_CONTROL);
> +	spin_unlock_irq(&rtc_lock);
> +
> +	/* We only care about the situation where AIE is disabled. */
> +	if (rtc_control & RTC_AIE)
> +		return -EBUSY;
> +
> +	err = cmos_read_time(dev, &now);
> +	if (err < 0)
> +		return err;
> +	t_now = rtc_tm_to_time64(&now);
> +
> +	/*
> +	 * When enabling "RTC wake-up" in BIOS setup, the machine reboots
> +	 * automatically right after shutdown on some buggy boxes.
> +	 * This automatic rebooting issue won't happen when the alarm
> +	 * time is larger than now+1 seconds.
> +	 *
> +	 * If the alarm time is equal to now+1 seconds, we need to wait
> +	 * for 1 second so that AF is set by CMOS hardware. Therefore, we
> +	 * can get a chance to clear AF in cmos_do_shutdown().
> +	 *
> +	 * If the alarm time is less equal to 'now', the function simply
> +	 * returns '0' so that we can get a chance to clear AF in
> +	 * cmos_do_shutdown().
> +	 */
> +	if (cmos->alarm_expires > (t_now + 1))
> +		return -EBUSY;
> +	else if (cmos->alarm_expires == (t_now + 1))
> +		msleep(1000);

I'm not too happy about that one second wait. Isn't the idea of
updating the alarm to a value in the past to cancel it working?

> +
> +	return 0;
> +}
> +
>  #ifdef CONFIG_PM
>  
>  static int cmos_suspend(struct device *dev)
> @@ -1094,8 +1143,11 @@ static void cmos_pnp_shutdown(struct pnp_dev *pnp)
>  	struct device *dev = &pnp->dev;
>  	struct cmos_rtc	*cmos = dev_get_drvdata(dev);
>  
> -	if (system_state == SYSTEM_POWER_OFF && !cmos_poweroff(dev))
> -		return;
> +	if (system_state == SYSTEM_POWER_OFF) {
> +		cmos_poweroff(dev);

You should test the return value of cmos_poweroff() at some point as the
return is depending on it in the original code.

However, I'm not sure why one would want to not execute
cmos_do_shutdown() only when CONFIG_PM is not defined.

> +		if (cmos_aie_poweroff(dev) < 0)
> +			return;
> +	}
>  
>  	cmos_do_shutdown(cmos->irq);
>  }
> @@ -1200,8 +1252,11 @@ static void cmos_platform_shutdown(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct cmos_rtc	*cmos = dev_get_drvdata(dev);
>  
> -	if (system_state == SYSTEM_POWER_OFF && !cmos_poweroff(dev))
> -		return;
> +	if (system_state == SYSTEM_POWER_OFF) {
> +		cmos_poweroff(dev);

Same comment here.

> +		if (cmos_aie_poweroff(dev) < 0)
> +			return;
> +	}
>  
>  	cmos_do_shutdown(cmos->irq);
>  }

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [RFC PATCH v2 1/2] rtc-cmos: Clear interrupt flag if alarm time is

[Huang Adrian]

Hi,

>>
>> -static struct cmos_rtc       cmos_rtc;
>> +static struct cmos_rtc       cmos_rtc = { .alarm_expires = 0 };
>>
>
> That is probably unnecessary as the whole struct is already zeroed at
> initialization.
>

Yes. Good catch. Will keep the original code.

>> +     if (cmos->alarm_expires > (t_now + 1))
>> +             return -EBUSY;
>> +     else if (cmos->alarm_expires == (t_now + 1))
>> +             msleep(1000);
>
> I'm not too happy about that one second wait. Isn't the idea of
> updating the alarm to a value in the past to cancel it working?
>

Updating the alarm to the past value can cancel it working.
I've verified it on my local machine and the issue goes away.
Thanks for the suggestion.

>>  static int cmos_suspend(struct device *dev)
>> @@ -1094,8 +1143,11 @@ static void cmos_pnp_shutdown(struct pnp_dev *pnp)
>>       struct device *dev = &pnp->dev;
>>       struct cmos_rtc *cmos = dev_get_drvdata(dev);
>>
>> -     if (system_state == SYSTEM_POWER_OFF && !cmos_poweroff(dev))
>> -             return;
>> +     if (system_state == SYSTEM_POWER_OFF) {
>> +             cmos_poweroff(dev);
>
> You should test the return value of cmos_poweroff() at some point as the
> return is depending on it in the original code.
>

Yes, agreed.

> However, I'm not sure why one would want to not execute
> cmos_do_shutdown() only when CONFIG_PM is not defined.
>

I have the same question about this one. BTW, cmos_do_shutdown()
is not executed when CONFIG_PM is defined.


I created the following patch based on your suggestion. Does it looks good
to you? If it does, I'll prepare v3 for testing. Thanks in advance.

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index a82556a..f8900aa 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -51,6 +51,7 @@ struct cmos_rtc {
        struct device           *dev;
        int                     irq;
        struct resource         *iomem;
+       time64_t                alarm_expires;

        void                    (*wake_on)(struct device *);
        void                    (*wake_off)(struct device *);
@@ -377,6 +378,8 @@ static int cmos_set_alarm(struct device *dev,
struct rtc_wkalrm *t)

        spin_unlock_irq(&rtc_lock);

+       cmos->alarm_expires = rtc_tm_to_time64(&t->time);
+
        return 0;
 }

@@ -860,6 +863,52 @@ static void __exit cmos_do_remove(struct device *dev)
        cmos->dev = NULL;
 }

+static int cmos_aie_poweroff(struct device *dev)
+{
+       struct cmos_rtc *cmos = dev_get_drvdata(dev);
+       struct rtc_time now;
+       time64_t t_now;
+       int retval = 0;
+       unsigned char rtc_control;
+
+       if (!cmos->alarm_expires)
+               return -EINVAL;
+
+       spin_lock_irq(&rtc_lock);
+       rtc_control = CMOS_READ(RTC_CONTROL);
+       spin_unlock_irq(&rtc_lock);
+
+       /* We only care about the situation where AIE is disabled. */
+       if (rtc_control & RTC_AIE)
+               return -EBUSY;
+
+       cmos_read_time(dev, &now);
+       t_now = rtc_tm_to_time64(&now);
+
+       /*
+        * When enabling "RTC wake-up" in BIOS setup, the machine reboots
+        * automatically right after shutdown on some buggy boxes.
+        * This automatic rebooting issue won't happen when the alarm
+        * time is larger than now+1 seconds.
+        *
+        * If the alarm time is equal to now+1 seconds, the issue can be
+        * prevented by cancelling the alarm.
+        */
+       if (cmos->alarm_expires == t_now + 1) {
+               struct rtc_wkalrm alarm;
+               int err;
+
+               /* Cancel the AIE timer by configuring the past time. */
+               rtc_time64_to_tm(t_now - 1, &alarm.time);
+               alarm.enabled = 0;
+               retval = cmos_set_alarm(dev, &alarm);
+       } else if (cmos->alarm_expires > t_now + 1) {
+               retval = -EBUSY;
+       }
+
+       return retval;
+}
+
 #ifdef CONFIG_PM

 static int cmos_suspend(struct device *dev)
@@ -1094,8 +1143,12 @@ static void cmos_pnp_shutdown(struct pnp_dev *pnp)
        struct device *dev = &pnp->dev;
        struct cmos_rtc *cmos = dev_get_drvdata(dev);

-       if (system_state == SYSTEM_POWER_OFF && !cmos_poweroff(dev))
-               return;
+       if (system_state == SYSTEM_POWER_OFF) {
+               int retval = cmos_poweroff(dev);
+
+               if (cmos_aie_poweroff(dev) < 0 && !retval)
+                       return;
+       }

        cmos_do_shutdown(cmos->irq);
 }
@@ -1200,8 +1253,12 @@ static void cmos_platform_shutdown(struct
platform_device *pdev)
        struct device *dev = &pdev->dev;
        struct cmos_rtc *cmos = dev_get_drvdata(dev);

-       if (system_state == SYSTEM_POWER_OFF && !cmos_poweroff(dev))
-               return;
+       if (system_state == SYSTEM_POWER_OFF) {
+               int retval = cmos_poweroff(dev);
+
+               if (cmos_aie_poweroff(dev) < 0 && !retval)
+                       return;
+       }

        cmos_do_shutdown(cmos->irq);
 }
--
1.9.1

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [RFC PATCH v2 1/2] rtc-cmos: Clear interrupt flag if alarm time is

[Alexandre Belloni]

On 26/06/2015 at 14:01:48 +0800, Huang Adrian wrote :
> I created the following patch based on your suggestion. Does it looks good
> to you? If it does, I'll prepare v3 for testing. Thanks in advance.
> 

Seems good to me.

> diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
> index a82556a..f8900aa 100644
> --- a/drivers/rtc/rtc-cmos.c
> +++ b/drivers/rtc/rtc-cmos.c
> @@ -51,6 +51,7 @@ struct cmos_rtc {
>         struct device           *dev;
>         int                     irq;
>         struct resource         *iomem;
> +       time64_t                alarm_expires;
> 
>         void                    (*wake_on)(struct device *);
>         void                    (*wake_off)(struct device *);
> @@ -377,6 +378,8 @@ static int cmos_set_alarm(struct device *dev,
> struct rtc_wkalrm *t)
> 
>         spin_unlock_irq(&rtc_lock);
> 
> +       cmos->alarm_expires = rtc_tm_to_time64(&t->time);
> +
>         return 0;
>  }
> 
> @@ -860,6 +863,52 @@ static void __exit cmos_do_remove(struct device *dev)
>         cmos->dev = NULL;
>  }
> 
> +static int cmos_aie_poweroff(struct device *dev)
> +{
> +       struct cmos_rtc *cmos = dev_get_drvdata(dev);
> +       struct rtc_time now;
> +       time64_t t_now;
> +       int retval = 0;
> +       unsigned char rtc_control;
> +
> +       if (!cmos->alarm_expires)
> +               return -EINVAL;
> +
> +       spin_lock_irq(&rtc_lock);
> +       rtc_control = CMOS_READ(RTC_CONTROL);
> +       spin_unlock_irq(&rtc_lock);
> +
> +       /* We only care about the situation where AIE is disabled. */
> +       if (rtc_control & RTC_AIE)
> +               return -EBUSY;
> +
> +       cmos_read_time(dev, &now);
> +       t_now = rtc_tm_to_time64(&now);
> +
> +       /*
> +        * When enabling "RTC wake-up" in BIOS setup, the machine reboots
> +        * automatically right after shutdown on some buggy boxes.
> +        * This automatic rebooting issue won't happen when the alarm
> +        * time is larger than now+1 seconds.
> +        *
> +        * If the alarm time is equal to now+1 seconds, the issue can be
> +        * prevented by cancelling the alarm.
> +        */
> +       if (cmos->alarm_expires == t_now + 1) {
> +               struct rtc_wkalrm alarm;
> +               int err;
> +
> +               /* Cancel the AIE timer by configuring the past time. */
> +               rtc_time64_to_tm(t_now - 1, &alarm.time);
> +               alarm.enabled = 0;
> +               retval = cmos_set_alarm(dev, &alarm);
> +       } else if (cmos->alarm_expires > t_now + 1) {
> +               retval = -EBUSY;
> +       }
> +
> +       return retval;
> +}
> +
>  #ifdef CONFIG_PM
> 
>  static int cmos_suspend(struct device *dev)
> @@ -1094,8 +1143,12 @@ static void cmos_pnp_shutdown(struct pnp_dev *pnp)
>         struct device *dev = &pnp->dev;
>         struct cmos_rtc *cmos = dev_get_drvdata(dev);
> 
> -       if (system_state == SYSTEM_POWER_OFF && !cmos_poweroff(dev))
> -               return;
> +       if (system_state == SYSTEM_POWER_OFF) {
> +               int retval = cmos_poweroff(dev);
> +
> +               if (cmos_aie_poweroff(dev) < 0 && !retval)
> +                       return;
> +       }
> 
>         cmos_do_shutdown(cmos->irq);
>  }
> @@ -1200,8 +1253,12 @@ static void cmos_platform_shutdown(struct
> platform_device *pdev)
>         struct device *dev = &pdev->dev;
>         struct cmos_rtc *cmos = dev_get_drvdata(dev);
> 
> -       if (system_state == SYSTEM_POWER_OFF && !cmos_poweroff(dev))
> -               return;
> +       if (system_state == SYSTEM_POWER_OFF) {
> +               int retval = cmos_poweroff(dev);
> +
> +               if (cmos_aie_poweroff(dev) < 0 && !retval)
> +                       return;
> +       }
> 
>         cmos_do_shutdown(cmos->irq);
>  }
> --
> 1.9.1

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.