* [PATCH 1/2] rtc: asm9260: allow COMPILE_TEST @ 2016-08-31 0:18 Alexandre Belloni 2016-08-31 0:18 ` [PATCH 2/2] rtc: asm9260: rework locking Alexandre Belloni 0 siblings, 1 reply; 4+ messages in thread From: Alexandre Belloni @ 2016-08-31 0:18 UTC (permalink / raw) To: Alessandro Zummo Cc: Oleksij Rempel, rtc-linux, linux-kernel, Alexandre Belloni The rtc-asm9260 driver compiles correctly on other architectures, add COMPILE_TEST to improve code coverage. Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> --- drivers/rtc/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig index e215f50794b6..5be1d9261fa9 100644 --- a/drivers/rtc/Kconfig +++ b/drivers/rtc/Kconfig @@ -1191,7 +1191,7 @@ comment "on-CPU RTC drivers" config RTC_DRV_ASM9260 tristate "Alphascale asm9260 RTC" - depends on MACH_ASM9260 + depends on MACH_ASM9260 || COMPILE_TEST help If you say yes here you get support for the RTC on the Alphascale asm9260 SoC. -- 2.9.3 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 2/2] rtc: asm9260: rework locking 2016-08-31 0:18 [PATCH 1/2] rtc: asm9260: allow COMPILE_TEST Alexandre Belloni @ 2016-08-31 0:18 ` Alexandre Belloni 2016-08-31 15:14 ` Oleksij Rempel 0 siblings, 1 reply; 4+ messages in thread From: Alexandre Belloni @ 2016-08-31 0:18 UTC (permalink / raw) To: Alessandro Zummo Cc: Oleksij Rempel, rtc-linux, linux-kernel, Alexandre Belloni The rtc-asm9260 driver uses a discrete spinlock (wrongly uninitialized). Use the rtc mutex to lock mmio accesses instead. Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> --- drivers/rtc/rtc-asm9260.c | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/drivers/rtc/rtc-asm9260.c b/drivers/rtc/rtc-asm9260.c index 5219916ce11d..812f131da694 100644 --- a/drivers/rtc/rtc-asm9260.c +++ b/drivers/rtc/rtc-asm9260.c @@ -112,8 +112,6 @@ struct asm9260_rtc_priv { void __iomem *iobase; struct rtc_device *rtc; struct clk *clk; - /* io lock */ - spinlock_t lock; }; static irqreturn_t asm9260_rtc_irq(int irq, void *dev_id) @@ -122,11 +120,13 @@ static irqreturn_t asm9260_rtc_irq(int irq, void *dev_id) u32 isr; unsigned long events = 0; + mutex_lock(&priv->rtc->ops_lock); isr = ioread32(priv->iobase + HW_CIIR); if (!isr) return IRQ_NONE; iowrite32(0, priv->iobase + HW_CIIR); + mutex_unlock(&priv->rtc->ops_lock); events |= RTC_AF | RTC_IRQF; @@ -139,9 +139,7 @@ static int asm9260_rtc_read_time(struct device *dev, struct rtc_time *tm) { struct asm9260_rtc_priv *priv = dev_get_drvdata(dev); u32 ctime0, ctime1, ctime2; - unsigned long irq_flags; - spin_lock_irqsave(&priv->lock, irq_flags); ctime0 = ioread32(priv->iobase + HW_CTIME0); ctime1 = ioread32(priv->iobase + HW_CTIME1); ctime2 = ioread32(priv->iobase + HW_CTIME2); @@ -155,7 +153,6 @@ static int asm9260_rtc_read_time(struct device *dev, struct rtc_time *tm) ctime1 = ioread32(priv->iobase + HW_CTIME1); ctime2 = ioread32(priv->iobase + HW_CTIME2); } - spin_unlock_irqrestore(&priv->lock, irq_flags); tm->tm_sec = (ctime0 >> BM_CTIME0_SEC_S) & BM_CTIME0_SEC_M; tm->tm_min = (ctime0 >> BM_CTIME0_MIN_S) & BM_CTIME0_MIN_M; @@ -174,9 +171,7 @@ static int asm9260_rtc_read_time(struct device *dev, struct rtc_time *tm) static int asm9260_rtc_set_time(struct device *dev, struct rtc_time *tm) { struct asm9260_rtc_priv *priv = dev_get_drvdata(dev); - unsigned long irq_flags; - spin_lock_irqsave(&priv->lock, irq_flags); /* * make sure SEC counter will not flip other counter on write time, * real value will be written at the enf of sequence. @@ -191,7 +186,6 @@ static int asm9260_rtc_set_time(struct device *dev, struct rtc_time *tm) iowrite32(tm->tm_hour, priv->iobase + HW_HOUR); iowrite32(tm->tm_min, priv->iobase + HW_MIN); iowrite32(tm->tm_sec, priv->iobase + HW_SEC); - spin_unlock_irqrestore(&priv->lock, irq_flags); return 0; } @@ -199,9 +193,7 @@ static int asm9260_rtc_set_time(struct device *dev, struct rtc_time *tm) static int asm9260_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) { struct asm9260_rtc_priv *priv = dev_get_drvdata(dev); - unsigned long irq_flags; - spin_lock_irqsave(&priv->lock, irq_flags); alrm->time.tm_year = ioread32(priv->iobase + HW_ALYEAR); alrm->time.tm_mon = ioread32(priv->iobase + HW_ALMON); alrm->time.tm_mday = ioread32(priv->iobase + HW_ALDOM); @@ -213,7 +205,6 @@ static int asm9260_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) alrm->enabled = ioread32(priv->iobase + HW_AMR) ? 1 : 0; alrm->pending = ioread32(priv->iobase + HW_CIIR) ? 1 : 0; - spin_unlock_irqrestore(&priv->lock, irq_flags); return rtc_valid_tm(&alrm->time); } @@ -221,9 +212,7 @@ static int asm9260_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) static int asm9260_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) { struct asm9260_rtc_priv *priv = dev_get_drvdata(dev); - unsigned long irq_flags; - spin_lock_irqsave(&priv->lock, irq_flags); iowrite32(alrm->time.tm_year, priv->iobase + HW_ALYEAR); iowrite32(alrm->time.tm_mon, priv->iobase + HW_ALMON); iowrite32(alrm->time.tm_mday, priv->iobase + HW_ALDOM); @@ -234,7 +223,6 @@ static int asm9260_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) iowrite32(alrm->time.tm_sec, priv->iobase + HW_ALSEC); iowrite32(alrm->enabled ? 0 : BM_AMR_OFF, priv->iobase + HW_AMR); - spin_unlock_irqrestore(&priv->lock, irq_flags); return 0; } -- 2.9.3 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] rtc: asm9260: rework locking 2016-08-31 0:18 ` [PATCH 2/2] rtc: asm9260: rework locking Alexandre Belloni @ 2016-08-31 15:14 ` Oleksij Rempel 2016-08-31 16:09 ` Alexandre Belloni 0 siblings, 1 reply; 4+ messages in thread From: Oleksij Rempel @ 2016-08-31 15:14 UTC (permalink / raw) To: Alexandre Belloni, Alessandro Zummo; +Cc: rtc-linux, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 4612 bytes --] Hi Am 31.08.2016 um 02:18 schrieb Alexandre Belloni: > The rtc-asm9260 driver uses a discrete spinlock (wrongly uninitialized). > Use the rtc mutex to lock mmio accesses instead. > > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> > --- > drivers/rtc/rtc-asm9260.c | 16 ++-------------- > 1 file changed, 2 insertions(+), 14 deletions(-) > > diff --git a/drivers/rtc/rtc-asm9260.c b/drivers/rtc/rtc-asm9260.c > index 5219916ce11d..812f131da694 100644 > --- a/drivers/rtc/rtc-asm9260.c > +++ b/drivers/rtc/rtc-asm9260.c > @@ -112,8 +112,6 @@ struct asm9260_rtc_priv { > void __iomem *iobase; > struct rtc_device *rtc; > struct clk *clk; > - /* io lock */ > - spinlock_t lock; > }; > > static irqreturn_t asm9260_rtc_irq(int irq, void *dev_id) > @@ -122,11 +120,13 @@ static irqreturn_t asm9260_rtc_irq(int irq, void *dev_id) > u32 isr; > unsigned long events = 0; > > + mutex_lock(&priv->rtc->ops_lock); > isr = ioread32(priv->iobase + HW_CIIR); > if (!isr) > return IRQ_NONE; > > iowrite32(0, priv->iobase + HW_CIIR); > + mutex_unlock(&priv->rtc->ops_lock); This means if we have shared or spurious interrupt we will get a deadlock here? > events |= RTC_AF | RTC_IRQF; > > @@ -139,9 +139,7 @@ static int asm9260_rtc_read_time(struct device *dev, struct rtc_time *tm) > { > struct asm9260_rtc_priv *priv = dev_get_drvdata(dev); > u32 ctime0, ctime1, ctime2; > - unsigned long irq_flags; > > - spin_lock_irqsave(&priv->lock, irq_flags); > ctime0 = ioread32(priv->iobase + HW_CTIME0); > ctime1 = ioread32(priv->iobase + HW_CTIME1); > ctime2 = ioread32(priv->iobase + HW_CTIME2); > @@ -155,7 +153,6 @@ static int asm9260_rtc_read_time(struct device *dev, struct rtc_time *tm) > ctime1 = ioread32(priv->iobase + HW_CTIME1); > ctime2 = ioread32(priv->iobase + HW_CTIME2); > } > - spin_unlock_irqrestore(&priv->lock, irq_flags); > > tm->tm_sec = (ctime0 >> BM_CTIME0_SEC_S) & BM_CTIME0_SEC_M; > tm->tm_min = (ctime0 >> BM_CTIME0_MIN_S) & BM_CTIME0_MIN_M; > @@ -174,9 +171,7 @@ static int asm9260_rtc_read_time(struct device *dev, struct rtc_time *tm) > static int asm9260_rtc_set_time(struct device *dev, struct rtc_time *tm) > { > struct asm9260_rtc_priv *priv = dev_get_drvdata(dev); > - unsigned long irq_flags; > > - spin_lock_irqsave(&priv->lock, irq_flags); > /* > * make sure SEC counter will not flip other counter on write time, > * real value will be written at the enf of sequence. > @@ -191,7 +186,6 @@ static int asm9260_rtc_set_time(struct device *dev, struct rtc_time *tm) > iowrite32(tm->tm_hour, priv->iobase + HW_HOUR); > iowrite32(tm->tm_min, priv->iobase + HW_MIN); > iowrite32(tm->tm_sec, priv->iobase + HW_SEC); > - spin_unlock_irqrestore(&priv->lock, irq_flags); > > return 0; > } > @@ -199,9 +193,7 @@ static int asm9260_rtc_set_time(struct device *dev, struct rtc_time *tm) > static int asm9260_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) > { > struct asm9260_rtc_priv *priv = dev_get_drvdata(dev); > - unsigned long irq_flags; > > - spin_lock_irqsave(&priv->lock, irq_flags); > alrm->time.tm_year = ioread32(priv->iobase + HW_ALYEAR); > alrm->time.tm_mon = ioread32(priv->iobase + HW_ALMON); > alrm->time.tm_mday = ioread32(priv->iobase + HW_ALDOM); > @@ -213,7 +205,6 @@ static int asm9260_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) > > alrm->enabled = ioread32(priv->iobase + HW_AMR) ? 1 : 0; > alrm->pending = ioread32(priv->iobase + HW_CIIR) ? 1 : 0; > - spin_unlock_irqrestore(&priv->lock, irq_flags); > > return rtc_valid_tm(&alrm->time); > } > @@ -221,9 +212,7 @@ static int asm9260_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) > static int asm9260_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) > { > struct asm9260_rtc_priv *priv = dev_get_drvdata(dev); > - unsigned long irq_flags; > > - spin_lock_irqsave(&priv->lock, irq_flags); > iowrite32(alrm->time.tm_year, priv->iobase + HW_ALYEAR); > iowrite32(alrm->time.tm_mon, priv->iobase + HW_ALMON); > iowrite32(alrm->time.tm_mday, priv->iobase + HW_ALDOM); > @@ -234,7 +223,6 @@ static int asm9260_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) > iowrite32(alrm->time.tm_sec, priv->iobase + HW_ALSEC); > > iowrite32(alrm->enabled ? 0 : BM_AMR_OFF, priv->iobase + HW_AMR); > - spin_unlock_irqrestore(&priv->lock, irq_flags); > > return 0; > } > -- Regards, Oleksij [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 213 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] rtc: asm9260: rework locking 2016-08-31 15:14 ` Oleksij Rempel @ 2016-08-31 16:09 ` Alexandre Belloni 0 siblings, 0 replies; 4+ messages in thread From: Alexandre Belloni @ 2016-08-31 16:09 UTC (permalink / raw) To: Oleksij Rempel; +Cc: Alessandro Zummo, rtc-linux, linux-kernel On 31/08/2016 at 17:14:27 +0200, Oleksij Rempel wrote : > Hi > > Am 31.08.2016 um 02:18 schrieb Alexandre Belloni: > > The rtc-asm9260 driver uses a discrete spinlock (wrongly uninitialized). > > Use the rtc mutex to lock mmio accesses instead. > > > > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> > > --- > > drivers/rtc/rtc-asm9260.c | 16 ++-------------- > > 1 file changed, 2 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/rtc/rtc-asm9260.c b/drivers/rtc/rtc-asm9260.c > > index 5219916ce11d..812f131da694 100644 > > --- a/drivers/rtc/rtc-asm9260.c > > +++ b/drivers/rtc/rtc-asm9260.c > > @@ -112,8 +112,6 @@ struct asm9260_rtc_priv { > > void __iomem *iobase; > > struct rtc_device *rtc; > > struct clk *clk; > > - /* io lock */ > > - spinlock_t lock; > > }; > > > > static irqreturn_t asm9260_rtc_irq(int irq, void *dev_id) > > @@ -122,11 +120,13 @@ static irqreturn_t asm9260_rtc_irq(int irq, void *dev_id) > > u32 isr; > > unsigned long events = 0; > > > > + mutex_lock(&priv->rtc->ops_lock); > > isr = ioread32(priv->iobase + HW_CIIR); > > if (!isr) > > return IRQ_NONE; > > > > iowrite32(0, priv->iobase + HW_CIIR); > > + mutex_unlock(&priv->rtc->ops_lock); > > This means if we have shared or spurious interrupt we will get a > deadlock here? > True, I'll fix that. I actually got it right at first and then I toyed with sparse/smatch to see whether it was properly checking the locking. Seems like my setup was not correct. Thanks for the review ! > > > events |= RTC_AF | RTC_IRQF; > > > > @@ -139,9 +139,7 @@ static int asm9260_rtc_read_time(struct device *dev, struct rtc_time *tm) > > { > > struct asm9260_rtc_priv *priv = dev_get_drvdata(dev); > > u32 ctime0, ctime1, ctime2; > > - unsigned long irq_flags; > > > > - spin_lock_irqsave(&priv->lock, irq_flags); > > ctime0 = ioread32(priv->iobase + HW_CTIME0); > > ctime1 = ioread32(priv->iobase + HW_CTIME1); > > ctime2 = ioread32(priv->iobase + HW_CTIME2); > > @@ -155,7 +153,6 @@ static int asm9260_rtc_read_time(struct device *dev, struct rtc_time *tm) > > ctime1 = ioread32(priv->iobase + HW_CTIME1); > > ctime2 = ioread32(priv->iobase + HW_CTIME2); > > } > > - spin_unlock_irqrestore(&priv->lock, irq_flags); > > > > tm->tm_sec = (ctime0 >> BM_CTIME0_SEC_S) & BM_CTIME0_SEC_M; > > tm->tm_min = (ctime0 >> BM_CTIME0_MIN_S) & BM_CTIME0_MIN_M; > > @@ -174,9 +171,7 @@ static int asm9260_rtc_read_time(struct device *dev, struct rtc_time *tm) > > static int asm9260_rtc_set_time(struct device *dev, struct rtc_time *tm) > > { > > struct asm9260_rtc_priv *priv = dev_get_drvdata(dev); > > - unsigned long irq_flags; > > > > - spin_lock_irqsave(&priv->lock, irq_flags); > > /* > > * make sure SEC counter will not flip other counter on write time, > > * real value will be written at the enf of sequence. > > @@ -191,7 +186,6 @@ static int asm9260_rtc_set_time(struct device *dev, struct rtc_time *tm) > > iowrite32(tm->tm_hour, priv->iobase + HW_HOUR); > > iowrite32(tm->tm_min, priv->iobase + HW_MIN); > > iowrite32(tm->tm_sec, priv->iobase + HW_SEC); > > - spin_unlock_irqrestore(&priv->lock, irq_flags); > > > > return 0; > > } > > @@ -199,9 +193,7 @@ static int asm9260_rtc_set_time(struct device *dev, struct rtc_time *tm) > > static int asm9260_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) > > { > > struct asm9260_rtc_priv *priv = dev_get_drvdata(dev); > > - unsigned long irq_flags; > > > > - spin_lock_irqsave(&priv->lock, irq_flags); > > alrm->time.tm_year = ioread32(priv->iobase + HW_ALYEAR); > > alrm->time.tm_mon = ioread32(priv->iobase + HW_ALMON); > > alrm->time.tm_mday = ioread32(priv->iobase + HW_ALDOM); > > @@ -213,7 +205,6 @@ static int asm9260_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) > > > > alrm->enabled = ioread32(priv->iobase + HW_AMR) ? 1 : 0; > > alrm->pending = ioread32(priv->iobase + HW_CIIR) ? 1 : 0; > > - spin_unlock_irqrestore(&priv->lock, irq_flags); > > > > return rtc_valid_tm(&alrm->time); > > } > > @@ -221,9 +212,7 @@ static int asm9260_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) > > static int asm9260_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) > > { > > struct asm9260_rtc_priv *priv = dev_get_drvdata(dev); > > - unsigned long irq_flags; > > > > - spin_lock_irqsave(&priv->lock, irq_flags); > > iowrite32(alrm->time.tm_year, priv->iobase + HW_ALYEAR); > > iowrite32(alrm->time.tm_mon, priv->iobase + HW_ALMON); > > iowrite32(alrm->time.tm_mday, priv->iobase + HW_ALDOM); > > @@ -234,7 +223,6 @@ static int asm9260_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) > > iowrite32(alrm->time.tm_sec, priv->iobase + HW_ALSEC); > > > > iowrite32(alrm->enabled ? 0 : BM_AMR_OFF, priv->iobase + HW_AMR); > > - spin_unlock_irqrestore(&priv->lock, irq_flags); > > > > return 0; > > } > > > > > -- > Regards, > Oleksij > -- Alexandre Belloni, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-08-31 16:09 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-31 0:18 [PATCH 1/2] rtc: asm9260: allow COMPILE_TEST Alexandre Belloni 2016-08-31 0:18 ` [PATCH 2/2] rtc: asm9260: rework locking Alexandre Belloni 2016-08-31 15:14 ` Oleksij Rempel 2016-08-31 16:09 ` Alexandre Belloni
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®