Message ID | 20160831001805.8367-2-alexandre.belloni@free-electrons.com |
---|---|
State | Superseded |
Headers | show |
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; > } >
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 >
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; }
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(-)