| Message ID | 1399386481-18643-1-git-send-email-boris.brezillon@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
Boris, > -----Original Message----- > From: Boris BREZILLON [mailto:boris.brezillon@free-electrons.com] > Sent: Tuesday, May 06, 2014 10:28 AM > To: Bryan Evenson > Cc: Andrew Victor; Nicolas Ferre; Jean-Christophe Plagniol-Villard; linux-arm- > kernel@lists.infradead.org; Alessandro Zummo; rtc- > linux@googlegroups.com; linux-kernel@vger.kernel.org; Boris BREZILLON > Subject: [PATCH] rtc: rtc-at91rm9200: fix infinite wait for ACKUPD irq > > The rtc user must wait at least 1 sec between each time/calandar update > (see atmel's datasheet chapter "Updating Time/Calendar"). > > Use the 1Hz interrupt to update the at91_rtc_upd_rdy flag and wait for > the at91_rtc_wait_upd_rdy event if the rtc is not ready. > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > Reported-by: Bryan Evenson <bevenson@melinkcorp.com> > --- > Hello Bryan, > > I reproduced your bug (using your script) and this patch seems to fix the > problem. > > Could you try it and let me know if it works for you ? Looks good to me. I modified the test script as follows: ---------- #!/bin/sh i=0 while [ 1 ]; do hwclock -w -u > /dev/null 2>&1 echo $$ $i $? : $((i++)) done ---------- This version then attempts to write to the RTC as often as possible (script change was due to a suggestion on the Busybox mailing list). I ran two instances of the script, which each looped through about 60,000 times over a 30 minute run. At no point has access to the RTC been permanently locked out on my system. I'd call this fixed. I'd assume this patch would be backported to the longterm releases? Regards, Bryan > > Best Regards, > > Boris > > drivers/rtc/rtc-at91rm9200.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c > index 3281c90..e3fe54c 100644 > --- a/drivers/rtc/rtc-at91rm9200.c > +++ b/drivers/rtc/rtc-at91rm9200.c > @@ -48,11 +48,13 @@ struct at91_rtc_config { > > static const struct at91_rtc_config *at91_rtc_config; > static DECLARE_COMPLETION(at91_rtc_updated); > +static DECLARE_COMPLETION(at91_rtc_wait_upd_rdy); > static unsigned int at91_alarm_year = AT91_RTC_EPOCH; > static void __iomem *at91_rtc_regs; > static int irq; > static DEFINE_SPINLOCK(at91_rtc_lock); > static u32 at91_rtc_shadow_imr; > +static bool at91_rtc_upd_rdy; > > static void at91_rtc_write_ier(u32 mask) > { > @@ -161,6 +163,8 @@ static int at91_rtc_settime(struct device *dev, struct > rtc_time *tm) > 1900 + tm->tm_year, tm->tm_mon, tm->tm_mday, > tm->tm_hour, tm->tm_min, tm->tm_sec); > > + wait_for_completion(&at91_rtc_wait_upd_rdy); > + > /* Stop Time/Calendar from counting */ > cr = at91_rtc_read(AT91_RTC_CR); > at91_rtc_write(AT91_RTC_CR, cr | AT91_RTC_UPDCAL | > AT91_RTC_UPDTIM); > @@ -183,6 +187,7 @@ static int at91_rtc_settime(struct device *dev, struct > rtc_time *tm) > > /* Restart Time/Calendar */ > cr = at91_rtc_read(AT91_RTC_CR); > + at91_rtc_upd_rdy = 0; > at91_rtc_write(AT91_RTC_CR, cr & ~(AT91_RTC_UPDCAL | > AT91_RTC_UPDTIM)); > > return 0; > @@ -290,8 +295,13 @@ static irqreturn_t at91_rtc_interrupt(int irq, void > *dev_id) > if (rtsr) { /* this interrupt is shared! Is it ours? */ > if (rtsr & AT91_RTC_ALARM) > events |= (RTC_AF | RTC_IRQF); > - if (rtsr & AT91_RTC_SECEV) > + if (rtsr & AT91_RTC_SECEV) { > events |= (RTC_UF | RTC_IRQF); > + if (!at91_rtc_upd_rdy) { > + at91_rtc_upd_rdy = 1; > + complete(&at91_rtc_wait_upd_rdy); > + } > + } > if (rtsr & AT91_RTC_ACKUPD) > complete(&at91_rtc_updated); > > @@ -413,6 +423,8 @@ static int __init at91_rtc_probe(struct > platform_device *pdev) > return PTR_ERR(rtc); > platform_set_drvdata(pdev, rtc); > > + /* Enable 1Hz events */ > + at91_rtc_write_ier(AT91_RTC_SECEV); > dev_info(&pdev->dev, "AT91 Real Time Clock driver.\n"); > return 0; > } > -- > 1.8.3.2
Hi, On 06/05/2014 21:06, Bryan Evenson wrote: > Boris, > >> -----Original Message----- >> From: Boris BREZILLON [mailto:boris.brezillon@free-electrons.com] >> Sent: Tuesday, May 06, 2014 10:28 AM >> To: Bryan Evenson >> Cc: Andrew Victor; Nicolas Ferre; Jean-Christophe Plagniol-Villard; linux-arm- >> kernel@lists.infradead.org; Alessandro Zummo; rtc- >> linux@googlegroups.com; linux-kernel@vger.kernel.org; Boris BREZILLON >> Subject: [PATCH] rtc: rtc-at91rm9200: fix infinite wait for ACKUPD irq >> >> The rtc user must wait at least 1 sec between each time/calandar update >> (see atmel's datasheet chapter "Updating Time/Calendar"). >> >> Use the 1Hz interrupt to update the at91_rtc_upd_rdy flag and wait for >> the at91_rtc_wait_upd_rdy event if the rtc is not ready. >> >> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> >> Reported-by: Bryan Evenson <bevenson@melinkcorp.com> >> --- >> Hello Bryan, >> >> I reproduced your bug (using your script) and this patch seems to fix the >> problem. >> >> Could you try it and let me know if it works for you ? > Looks good to me. I modified the test script as follows: > > ---------- > #!/bin/sh > i=0 > while [ 1 ]; do > hwclock -w -u > /dev/null 2>&1 > echo $$ $i $? > : $((i++)) > done > ---------- > > This version then attempts to write to the RTC as often as possible (script change was due to a suggestion on the Busybox mailing list). I ran two instances of the script, which each looped through about 60,000 times over a 30 minute run. At no point has access to the RTC been permanently locked out on my system. I'd call this fixed. Great! > I'd assume this patch would be backported to the longterm releases? I'd like to wait for Nicolas' ack before asking for a backport to stable releases. Best Regards, Boris
Hi, On 06/05/2014 at 19:06:56 +0000, Bryan Evenson wrote : > > I'd assume this patch would be backported to the longterm releases? > If by longterm, you mean the linux4sam tree, 3.10 branch, it is up to Nicolas to take it. I believe it will be pretty easy to convince him ;)
On 06/05/2014 23:10, Alexandre Belloni : > Hi, > > On 06/05/2014 at 19:06:56 +0000, Bryan Evenson wrote : >> >> I'd assume this patch would be backported to the longterm releases? >> > > If by longterm, you mean the linux4sam tree, 3.10 branch, it is up to > Nicolas to take it. I believe it will be pretty easy to convince him ;) Absolutely, it is always easy to convince me with: - good commit message - script to reproduce the issue - follow-up of the patch plus testing - dialogue between skilled kernel developers So, yes, I will "ack" the patch and immediately integrate it in our linux4sam 3.10 branch. On the other hand, I let Boris add the "stable" tag to his original patch. Thanks to all of you for this fix. Bye,
diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c index 3281c90..e3fe54c 100644 --- a/drivers/rtc/rtc-at91rm9200.c +++ b/drivers/rtc/rtc-at91rm9200.c @@ -48,11 +48,13 @@ struct at91_rtc_config { static const struct at91_rtc_config *at91_rtc_config; static DECLARE_COMPLETION(at91_rtc_updated); +static DECLARE_COMPLETION(at91_rtc_wait_upd_rdy); static unsigned int at91_alarm_year = AT91_RTC_EPOCH; static void __iomem *at91_rtc_regs; static int irq; static DEFINE_SPINLOCK(at91_rtc_lock); static u32 at91_rtc_shadow_imr; +static bool at91_rtc_upd_rdy; static void at91_rtc_write_ier(u32 mask) { @@ -161,6 +163,8 @@ static int at91_rtc_settime(struct device *dev, struct rtc_time *tm) 1900 + tm->tm_year, tm->tm_mon, tm->tm_mday, tm->tm_hour, tm->tm_min, tm->tm_sec); + wait_for_completion(&at91_rtc_wait_upd_rdy); + /* Stop Time/Calendar from counting */ cr = at91_rtc_read(AT91_RTC_CR); at91_rtc_write(AT91_RTC_CR, cr | AT91_RTC_UPDCAL | AT91_RTC_UPDTIM); @@ -183,6 +187,7 @@ static int at91_rtc_settime(struct device *dev, struct rtc_time *tm) /* Restart Time/Calendar */ cr = at91_rtc_read(AT91_RTC_CR); + at91_rtc_upd_rdy = 0; at91_rtc_write(AT91_RTC_CR, cr & ~(AT91_RTC_UPDCAL | AT91_RTC_UPDTIM)); return 0; @@ -290,8 +295,13 @@ static irqreturn_t at91_rtc_interrupt(int irq, void *dev_id) if (rtsr) { /* this interrupt is shared! Is it ours? */ if (rtsr & AT91_RTC_ALARM) events |= (RTC_AF | RTC_IRQF); - if (rtsr & AT91_RTC_SECEV) + if (rtsr & AT91_RTC_SECEV) { events |= (RTC_UF | RTC_IRQF); + if (!at91_rtc_upd_rdy) { + at91_rtc_upd_rdy = 1; + complete(&at91_rtc_wait_upd_rdy); + } + } if (rtsr & AT91_RTC_ACKUPD) complete(&at91_rtc_updated); @@ -413,6 +423,8 @@ static int __init at91_rtc_probe(struct platform_device *pdev) return PTR_ERR(rtc); platform_set_drvdata(pdev, rtc); + /* Enable 1Hz events */ + at91_rtc_write_ier(AT91_RTC_SECEV); dev_info(&pdev->dev, "AT91 Real Time Clock driver.\n"); return 0; }
The rtc user must wait at least 1 sec between each time/calandar update (see atmel's datasheet chapter "Updating Time/Calendar"). Use the 1Hz interrupt to update the at91_rtc_upd_rdy flag and wait for the at91_rtc_wait_upd_rdy event if the rtc is not ready. Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> Reported-by: Bryan Evenson <bevenson@melinkcorp.com> --- Hello Bryan, I reproduced your bug (using your script) and this patch seems to fix the problem. Could you try it and let me know if it works for you ? Best Regards, Boris drivers/rtc/rtc-at91rm9200.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)