| Message ID | 1456758832-10590-7-git-send-email-akinobu.mita@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
2016-03-01 0:13 GMT+09:00 Akinobu Mita <akinobu.mita@gmail.com>: > ds3232-core requests irq with IRQF_SHARED, so irq can be shared by > several devices. But the irq handler for ds3232 unconditionally > disables the irq at first and the irq is re-enabled only when the > interrupt source was the ds3232's alarm. This behaviour breaks the > devices sharing the same irq in the various scenarios. > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > Cc: Alessandro Zummo <a.zummo@towertech.it> > Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com> > Cc: Dennis Aberilla <denzzzhome@yahoo.com> > --- > * New patch from this version > > drivers/rtc/rtc-ds3232-core.c | 14 ++------------ > 1 file changed, 2 insertions(+), 12 deletions(-) > > diff --git a/drivers/rtc/rtc-ds3232-core.c b/drivers/rtc/rtc-ds3232-core.c > index be8dfe0..6489380 100644 > --- a/drivers/rtc/rtc-ds3232-core.c > +++ b/drivers/rtc/rtc-ds3232-core.c > @@ -59,7 +59,6 @@ struct ds3232 { > */ > struct mutex mutex; > bool suspended; > - int exiting; > }; > > static int ds3232_check_rtc_status(struct device *dev) > @@ -314,8 +313,6 @@ static irqreturn_t ds3232_irq(int irq, void *dev_id) > struct device *dev = dev_id; > struct ds3232 *ds3232 = dev_get_drvdata(dev); > > - disable_irq_nosync(irq); > - > /* > * If rtc as a wakeup source, can't schedule the work > * at system resume flow, because at this time the i2c bus I found that this patch is not correct because interrupt is re-enabled after this hardirq handler finished without clearing Alarm Flag or Alarm Interrupt Enable in RTC register. Maybe we should drop IRQF_SHARED flag.
On 03/03/2016 at 23:10:35 +0900, Akinobu Mita wrote : > 2016-03-01 0:13 GMT+09:00 Akinobu Mita <akinobu.mita@gmail.com>: > > ds3232-core requests irq with IRQF_SHARED, so irq can be shared by > > several devices. But the irq handler for ds3232 unconditionally > > disables the irq at first and the irq is re-enabled only when the > > interrupt source was the ds3232's alarm. This behaviour breaks the > > devices sharing the same irq in the various scenarios. > > > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > > Cc: Alessandro Zummo <a.zummo@towertech.it> > > Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com> > > Cc: Dennis Aberilla <denzzzhome@yahoo.com> > > --- > > * New patch from this version > > > > drivers/rtc/rtc-ds3232-core.c | 14 ++------------ > > 1 file changed, 2 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/rtc/rtc-ds3232-core.c b/drivers/rtc/rtc-ds3232-core.c > > index be8dfe0..6489380 100644 > > --- a/drivers/rtc/rtc-ds3232-core.c > > +++ b/drivers/rtc/rtc-ds3232-core.c > > @@ -59,7 +59,6 @@ struct ds3232 { > > */ > > struct mutex mutex; > > bool suspended; > > - int exiting; > > }; > > > > static int ds3232_check_rtc_status(struct device *dev) > > @@ -314,8 +313,6 @@ static irqreturn_t ds3232_irq(int irq, void *dev_id) > > struct device *dev = dev_id; > > struct ds3232 *ds3232 = dev_get_drvdata(dev); > > > > - disable_irq_nosync(irq); > > - > > /* > > * If rtc as a wakeup source, can't schedule the work > > * at system resume flow, because at this time the i2c bus > > I found that this patch is not correct because interrupt is re-enabled > after this hardirq handler finished without clearing Alarm Flag or > Alarm Interrupt Enable in RTC register. > > Maybe we should drop IRQF_SHARED flag. I think the best is to completely reworke the irq handling. It dates from many many year ago and can be replace by a simple threaded irq. This will solve your issue.
2016-03-04 7:57 GMT+09:00 Alexandre Belloni <alexandre.belloni@free-electrons.com>: > On 03/03/2016 at 23:10:35 +0900, Akinobu Mita wrote : >> 2016-03-01 0:13 GMT+09:00 Akinobu Mita <akinobu.mita@gmail.com>: >> > ds3232-core requests irq with IRQF_SHARED, so irq can be shared by >> > several devices. But the irq handler for ds3232 unconditionally >> > disables the irq at first and the irq is re-enabled only when the >> > interrupt source was the ds3232's alarm. This behaviour breaks the >> > devices sharing the same irq in the various scenarios. >> > >> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> >> > Cc: Alessandro Zummo <a.zummo@towertech.it> >> > Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com> >> > Cc: Dennis Aberilla <denzzzhome@yahoo.com> >> > --- >> > * New patch from this version >> > >> > drivers/rtc/rtc-ds3232-core.c | 14 ++------------ >> > 1 file changed, 2 insertions(+), 12 deletions(-) >> > >> > diff --git a/drivers/rtc/rtc-ds3232-core.c b/drivers/rtc/rtc-ds3232-core.c >> > index be8dfe0..6489380 100644 >> > --- a/drivers/rtc/rtc-ds3232-core.c >> > +++ b/drivers/rtc/rtc-ds3232-core.c >> > @@ -59,7 +59,6 @@ struct ds3232 { >> > */ >> > struct mutex mutex; >> > bool suspended; >> > - int exiting; >> > }; >> > >> > static int ds3232_check_rtc_status(struct device *dev) >> > @@ -314,8 +313,6 @@ static irqreturn_t ds3232_irq(int irq, void *dev_id) >> > struct device *dev = dev_id; >> > struct ds3232 *ds3232 = dev_get_drvdata(dev); >> > >> > - disable_irq_nosync(irq); >> > - >> > /* >> > * If rtc as a wakeup source, can't schedule the work >> > * at system resume flow, because at this time the i2c bus >> >> I found that this patch is not correct because interrupt is re-enabled >> after this hardirq handler finished without clearing Alarm Flag or >> Alarm Interrupt Enable in RTC register. >> >> Maybe we should drop IRQF_SHARED flag. > > I think the best is to completely reworke the irq handling. It dates > from many many year ago and can be replace by a simple threaded irq. > This will solve your issue. I tried converting to threaded irq. But it needs to deal with suspend resume paths. (i.e. the following comment in ds3232_irq) /* * If rtc as a wakeup source, can't schedule the work * at system resume flow, because at this time the i2c bus * has not been resumed. */ Unfortunately, I don't have the computer that can use RTC on i2c-bus as wake up source. So I think dropping IRQF_SHARED is safe for now.
On 04/03/2016 at 22:56:01 +0900, Akinobu Mita wrote : > 2016-03-04 7:57 GMT+09:00 Alexandre Belloni > <alexandre.belloni@free-electrons.com>: > > On 03/03/2016 at 23:10:35 +0900, Akinobu Mita wrote : > >> 2016-03-01 0:13 GMT+09:00 Akinobu Mita <akinobu.mita@gmail.com>: > >> > ds3232-core requests irq with IRQF_SHARED, so irq can be shared by > >> > several devices. But the irq handler for ds3232 unconditionally > >> > disables the irq at first and the irq is re-enabled only when the > >> > interrupt source was the ds3232's alarm. This behaviour breaks the > >> > devices sharing the same irq in the various scenarios. > >> > > >> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > >> > Cc: Alessandro Zummo <a.zummo@towertech.it> > >> > Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com> > >> > Cc: Dennis Aberilla <denzzzhome@yahoo.com> > >> > --- > >> > * New patch from this version > >> > > >> > drivers/rtc/rtc-ds3232-core.c | 14 ++------------ > >> > 1 file changed, 2 insertions(+), 12 deletions(-) > >> > > >> > diff --git a/drivers/rtc/rtc-ds3232-core.c b/drivers/rtc/rtc-ds3232-core.c > >> > index be8dfe0..6489380 100644 > >> > --- a/drivers/rtc/rtc-ds3232-core.c > >> > +++ b/drivers/rtc/rtc-ds3232-core.c > >> > @@ -59,7 +59,6 @@ struct ds3232 { > >> > */ > >> > struct mutex mutex; > >> > bool suspended; > >> > - int exiting; > >> > }; > >> > > >> > static int ds3232_check_rtc_status(struct device *dev) > >> > @@ -314,8 +313,6 @@ static irqreturn_t ds3232_irq(int irq, void *dev_id) > >> > struct device *dev = dev_id; > >> > struct ds3232 *ds3232 = dev_get_drvdata(dev); > >> > > >> > - disable_irq_nosync(irq); > >> > - > >> > /* > >> > * If rtc as a wakeup source, can't schedule the work > >> > * at system resume flow, because at this time the i2c bus > >> > >> I found that this patch is not correct because interrupt is re-enabled > >> after this hardirq handler finished without clearing Alarm Flag or > >> Alarm Interrupt Enable in RTC register. > >> > >> Maybe we should drop IRQF_SHARED flag. > > > > I think the best is to completely reworke the irq handling. It dates > > from many many year ago and can be replace by a simple threaded irq. > > This will solve your issue. > > I tried converting to threaded irq. But it needs to deal with suspend > resume paths. (i.e. the following comment in ds3232_irq) > > /* > * If rtc as a wakeup source, can't schedule the work > * at system resume flow, because at this time the i2c bus > * has not been resumed. > */ > > Unfortunately, I don't have the computer that can use RTC on i2c-bus > as wake up source. So I think dropping IRQF_SHARED is safe for now. That comment is bogus, you can drop it as well. It may have been the case a few years ago but that is not true anymore (else a lot of other RTC would fail).
diff --git a/drivers/rtc/rtc-ds3232-core.c b/drivers/rtc/rtc-ds3232-core.c index be8dfe0..6489380 100644 --- a/drivers/rtc/rtc-ds3232-core.c +++ b/drivers/rtc/rtc-ds3232-core.c @@ -59,7 +59,6 @@ struct ds3232 { */ struct mutex mutex; bool suspended; - int exiting; }; static int ds3232_check_rtc_status(struct device *dev) @@ -314,8 +313,6 @@ static irqreturn_t ds3232_irq(int irq, void *dev_id) struct device *dev = dev_id; struct ds3232 *ds3232 = dev_get_drvdata(dev); - disable_irq_nosync(irq); - /* * If rtc as a wakeup source, can't schedule the work * at system resume flow, because at this time the i2c bus @@ -342,8 +339,8 @@ static void ds3232_work(struct work_struct *work) if (stat & DS3232_REG_SR_A1F) { ret = regmap_read(ds3232->regmap, DS3232_REG_CR, &control); if (ret) { - pr_warn("Read Control Register error - Disable IRQ%d\n", - ds3232->irq); + dev_warn(ds3232->dev, + "Read Control Register error %d\n", ret); } else { /* disable alarm1 interrupt */ control &= ~(DS3232_REG_CR_A1IE); @@ -367,9 +364,6 @@ static void ds3232_work(struct work_struct *work) } rtc_update_irq(ds3232->rtc, 1, RTC_AF | RTC_IRQF); - - if (!ds3232->exiting) - enable_irq(ds3232->irq); } } @@ -428,10 +422,6 @@ int ds3232_remove(struct device *dev) struct ds3232 *ds3232 = dev_get_drvdata(dev); if (ds3232->irq > 0) { - mutex_lock(&ds3232->mutex); - ds3232->exiting = 1; - mutex_unlock(&ds3232->mutex); - devm_free_irq(dev, ds3232->irq, dev); cancel_work_sync(&ds3232->work); }
ds3232-core requests irq with IRQF_SHARED, so irq can be shared by several devices. But the irq handler for ds3232 unconditionally disables the irq at first and the irq is re-enabled only when the interrupt source was the ds3232's alarm. This behaviour breaks the devices sharing the same irq in the various scenarios. Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> Cc: Alessandro Zummo <a.zummo@towertech.it> Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com> Cc: Dennis Aberilla <denzzzhome@yahoo.com> --- * New patch from this version drivers/rtc/rtc-ds3232-core.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)