From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030276AbcHaQJn (ORCPT ); Wed, 31 Aug 2016 12:09:43 -0400 Received: from down.free-electrons.com ([37.187.137.238]:40579 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S934855AbcHaQJm (ORCPT ); Wed, 31 Aug 2016 12:09:42 -0400 Date: Wed, 31 Aug 2016 18:09:40 +0200 From: Alexandre Belloni To: Oleksij Rempel Cc: Alessandro Zummo , rtc-linux@googlegroups.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] rtc: asm9260: rework locking Message-ID: <20160831160940.eb3ip4t3jfrga3lv@piout.net> References: <20160831001805.8367-1-alexandre.belloni@free-electrons.com> <20160831001805.8367-2-alexandre.belloni@free-electrons.com> <5e6bfec7-d5b6-6311-9cab-76d9aa932408@rempel-privat.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5e6bfec7-d5b6-6311-9cab-76d9aa932408@rempel-privat.de> User-Agent: Mutt/1.6.2-neo (2016-07-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > --- > > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Sender: rtc-linux@googlegroups.com Received: from mail.free-electrons.com (down.free-electrons.com. [37.187.137.238]) by gmr-mx.google.com with ESMTP id u3si93037wmf.1.2016.08.31.09.09.40 for ; Wed, 31 Aug 2016 09:09:40 -0700 (PDT) Date: Wed, 31 Aug 2016 18:09:40 +0200 From: Alexandre Belloni To: Oleksij Rempel Cc: Alessandro Zummo , rtc-linux@googlegroups.com, linux-kernel@vger.kernel.org Subject: [rtc-linux] Re: [PATCH 2/2] rtc: asm9260: rework locking Message-ID: <20160831160940.eb3ip4t3jfrga3lv@piout.net> References: <20160831001805.8367-1-alexandre.belloni@free-electrons.com> <20160831001805.8367-2-alexandre.belloni@free-electrons.com> <5e6bfec7-d5b6-6311-9cab-76d9aa932408@rempel-privat.de> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 In-Reply-To: <5e6bfec7-d5b6-6311-9cab-76d9aa932408@rempel-privat.de> Reply-To: rtc-linux@googlegroups.com List-ID: List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , 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 > > --- > > 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 -- You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. --- You received this message because you are subscribed to the Google Groups "rtc-linux" group. To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout.