| Message ID | 1412881594-25678-9-git-send-email-johan@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
Hi, On Thu, Oct 09, 2014 at 09:06:30PM +0200, Johan Hovold wrote: > Make sure to restore local irq state when reading the timer/calendar > (TC) registers, so that omap_rtc_read_time() can be called with > interrupts disabled. > > Signed-off-by: Johan Hovold <johan@kernel.org> > --- > drivers/rtc/rtc-omap.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c > index 0ef016553a97..62e2e9a9887a 100644 > --- a/drivers/rtc/rtc-omap.c > +++ b/drivers/rtc/rtc-omap.c > @@ -239,8 +239,10 @@ static void bcd2tm(struct rtc_time *tm) > > static int omap_rtc_read_time(struct device *dev, struct rtc_time *tm) > { > + unsigned long flags; > + > /* we don't report wday/yday/isdst ... */ > - local_irq_disable(); > + local_irq_save(flags); you should really convert these to a real spin_lock_irq*(), that's because local_irq* do not get re-written with RT patchset, so this pretty much "breaks" RT.
On Fri, Oct 10, 2014 at 01:02:31PM -0500, Felipe Balbi wrote: > Hi, > > On Thu, Oct 09, 2014 at 09:06:30PM +0200, Johan Hovold wrote: > > Make sure to restore local irq state when reading the timer/calendar > > (TC) registers, so that omap_rtc_read_time() can be called with > > interrupts disabled. > > > > Signed-off-by: Johan Hovold <johan@kernel.org> > > --- > > drivers/rtc/rtc-omap.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c > > index 0ef016553a97..62e2e9a9887a 100644 > > --- a/drivers/rtc/rtc-omap.c > > +++ b/drivers/rtc/rtc-omap.c > > @@ -239,8 +239,10 @@ static void bcd2tm(struct rtc_time *tm) > > > > static int omap_rtc_read_time(struct device *dev, struct rtc_time *tm) > > { > > + unsigned long flags; > > + > > /* we don't report wday/yday/isdst ... */ > > - local_irq_disable(); > > + local_irq_save(flags); > > you should really convert these to a real spin_lock_irq*(), that's > because local_irq* do not get re-written with RT patchset, so this > pretty much "breaks" RT. The driver uses local_irq* throughout to guarantee registers are not read or written during an update event. In fact, at least on AM33xx, this is not even necessary when reading (as opposed to writing) the TC registers, but I did not dare change that without knowing how the legacy platforms work in this respect. Do you suggest doing this conversion as part of, or on top of, this series? Johan
On Sat, Oct 11, 2014 at 12:12:01PM +0200, Johan Hovold wrote: > On Fri, Oct 10, 2014 at 01:02:31PM -0500, Felipe Balbi wrote: > > Hi, > > > > On Thu, Oct 09, 2014 at 09:06:30PM +0200, Johan Hovold wrote: > > > Make sure to restore local irq state when reading the timer/calendar > > > (TC) registers, so that omap_rtc_read_time() can be called with > > > interrupts disabled. > > > > > > Signed-off-by: Johan Hovold <johan@kernel.org> > > > --- > > > drivers/rtc/rtc-omap.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c > > > index 0ef016553a97..62e2e9a9887a 100644 > > > --- a/drivers/rtc/rtc-omap.c > > > +++ b/drivers/rtc/rtc-omap.c > > > @@ -239,8 +239,10 @@ static void bcd2tm(struct rtc_time *tm) > > > > > > static int omap_rtc_read_time(struct device *dev, struct rtc_time *tm) > > > { > > > + unsigned long flags; > > > + > > > /* we don't report wday/yday/isdst ... */ > > > - local_irq_disable(); > > > + local_irq_save(flags); > > > > you should really convert these to a real spin_lock_irq*(), that's > > because local_irq* do not get re-written with RT patchset, so this > > pretty much "breaks" RT. > > The driver uses local_irq* throughout to guarantee registers are not > read or written during an update event. > > In fact, at least on AM33xx, this is not even necessary when reading (as > opposed to writing) the TC registers, but I did not dare change that > without knowing how the legacy platforms work in this respect. > > Do you suggest doing this conversion as part of, or on top of, this > series? probably on top of is. Safer that way.
On Sat, Oct 11, 2014 at 07:47:58PM -0500, Felipe Balbi wrote: > On Sat, Oct 11, 2014 at 12:12:01PM +0200, Johan Hovold wrote: > > On Fri, Oct 10, 2014 at 01:02:31PM -0500, Felipe Balbi wrote: > > > Hi, > > > > > > On Thu, Oct 09, 2014 at 09:06:30PM +0200, Johan Hovold wrote: > > > > Make sure to restore local irq state when reading the timer/calendar > > > > (TC) registers, so that omap_rtc_read_time() can be called with > > > > interrupts disabled. > > > > > > > > Signed-off-by: Johan Hovold <johan@kernel.org> > > > > --- > > > > drivers/rtc/rtc-omap.c | 6 ++++-- > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c > > > > index 0ef016553a97..62e2e9a9887a 100644 > > > > --- a/drivers/rtc/rtc-omap.c > > > > +++ b/drivers/rtc/rtc-omap.c > > > > @@ -239,8 +239,10 @@ static void bcd2tm(struct rtc_time *tm) > > > > > > > > static int omap_rtc_read_time(struct device *dev, struct rtc_time *tm) > > > > { > > > > + unsigned long flags; > > > > + > > > > /* we don't report wday/yday/isdst ... */ > > > > - local_irq_disable(); > > > > + local_irq_save(flags); > > > > > > you should really convert these to a real spin_lock_irq*(), that's > > > because local_irq* do not get re-written with RT patchset, so this > > > pretty much "breaks" RT. > > > > The driver uses local_irq* throughout to guarantee registers are not > > read or written during an update event. > > > > In fact, at least on AM33xx, this is not even necessary when reading (as > > opposed to writing) the TC registers, but I did not dare change that > > without knowing how the legacy platforms work in this respect. > > > > Do you suggest doing this conversion as part of, or on top of, this > > series? > > probably on top of is. Safer that way. I didn't include this conversion in v2 as you may have noticed. In fact, I'm not even sure it should be done. Using local_irq* "breaks" RT, but if we change it to spin_lock_irq* and RT preempts us, we may miss the 15us access window which would break the driver: "If the ARM accesses the TC registers outside of the access period, then the access is not guaranteed." On am33xx this only affects updating the TC (timer/calendar) or alarm registers, but the driver currently disables interrupts also on reading the time (possibly because the early IP revisions did not have the shadow read registers). Johan
On Wed, Oct 22, 2014 at 12:50:11PM +0200, Johan Hovold wrote: > On Sat, Oct 11, 2014 at 07:47:58PM -0500, Felipe Balbi wrote: > > On Sat, Oct 11, 2014 at 12:12:01PM +0200, Johan Hovold wrote: > > > On Fri, Oct 10, 2014 at 01:02:31PM -0500, Felipe Balbi wrote: > > > > Hi, > > > > > > > > On Thu, Oct 09, 2014 at 09:06:30PM +0200, Johan Hovold wrote: > > > > > Make sure to restore local irq state when reading the timer/calendar > > > > > (TC) registers, so that omap_rtc_read_time() can be called with > > > > > interrupts disabled. > > > > > > > > > > Signed-off-by: Johan Hovold <johan@kernel.org> > > > > > --- > > > > > drivers/rtc/rtc-omap.c | 6 ++++-- > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c > > > > > index 0ef016553a97..62e2e9a9887a 100644 > > > > > --- a/drivers/rtc/rtc-omap.c > > > > > +++ b/drivers/rtc/rtc-omap.c > > > > > @@ -239,8 +239,10 @@ static void bcd2tm(struct rtc_time *tm) > > > > > > > > > > static int omap_rtc_read_time(struct device *dev, struct rtc_time *tm) > > > > > { > > > > > + unsigned long flags; > > > > > + > > > > > /* we don't report wday/yday/isdst ... */ > > > > > - local_irq_disable(); > > > > > + local_irq_save(flags); > > > > > > > > you should really convert these to a real spin_lock_irq*(), that's > > > > because local_irq* do not get re-written with RT patchset, so this > > > > pretty much "breaks" RT. > > > > > > The driver uses local_irq* throughout to guarantee registers are not > > > read or written during an update event. > > > > > > In fact, at least on AM33xx, this is not even necessary when reading (as > > > opposed to writing) the TC registers, but I did not dare change that > > > without knowing how the legacy platforms work in this respect. > > > > > > Do you suggest doing this conversion as part of, or on top of, this > > > series? > > > > probably on top of is. Safer that way. > > I didn't include this conversion in v2 as you may have noticed. In fact, > I'm not even sure it should be done. > > Using local_irq* "breaks" RT, but if we change it to spin_lock_irq* and > RT preempts us, we may miss the 15us access window which would break the > driver: > > "If the ARM accesses the TC registers outside of the access > period, then the access is not guaranteed." > > On am33xx this only affects updating the TC (timer/calendar) or alarm > registers, but the driver currently disables interrupts also on reading > the time (possibly because the early IP revisions did not have the > shadow read registers). that's alright, better not to change the behavior in this case. But as a future patch, you might want to convert this to a raw_spinlock. That won't get preempted.
diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c index 0ef016553a97..62e2e9a9887a 100644 --- a/drivers/rtc/rtc-omap.c +++ b/drivers/rtc/rtc-omap.c @@ -239,8 +239,10 @@ static void bcd2tm(struct rtc_time *tm) static int omap_rtc_read_time(struct device *dev, struct rtc_time *tm) { + unsigned long flags; + /* we don't report wday/yday/isdst ... */ - local_irq_disable(); + local_irq_save(flags); rtc_wait_not_busy(); tm->tm_sec = rtc_read(OMAP_RTC_SECONDS_REG); @@ -250,7 +252,7 @@ static int omap_rtc_read_time(struct device *dev, struct rtc_time *tm) tm->tm_mon = rtc_read(OMAP_RTC_MONTHS_REG); tm->tm_year = rtc_read(OMAP_RTC_YEARS_REG); - local_irq_enable(); + local_irq_restore(flags); bcd2tm(tm); return 0;
Make sure to restore local irq state when reading the timer/calendar (TC) registers, so that omap_rtc_read_time() can be called with interrupts disabled. Signed-off-by: Johan Hovold <johan@kernel.org> --- drivers/rtc/rtc-omap.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)