Message ID | 1433845413-13960-2-git-send-email-adrianhuang0701@gmail.com |
---|---|
State | Superseded |
Headers | show |
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 >
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 > > >
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); > }
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); }