| Message ID | 1370630768-4077-3-git-send-email-grygorii.strashko@ti.com |
|---|---|
| State | Rejected |
| Headers | show |
Hi, On Fri, Jun 07, 2013 at 09:46:05PM +0300, Grygorii Strashko wrote: > Add runtime check at the beginning of omap_i2c_isr/omap_i2c_isr_thread > to be sure that i2c is enabled, before performing IRQ handling and accessing > I2C IP registers: > if (pm_runtime_suspended(dev->dev)) { > WARN_ONCE(true, "We should never be here!\n"); > return IRQ_NONE; > } > > Produce warning in case if ISR called when i2c is disabled > > CC: Kevin Hilman <khilman@linaro.org> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > --- > drivers/i2c/busses/i2c-omap.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index 97844ff..2dac598 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -885,6 +885,11 @@ omap_i2c_isr(int irq, void *dev_id) > u16 stat; > > spin_lock(&dev->lock); > + if (pm_runtime_suspended(dev->dev)) { > + WARN_ONCE(true, "We should never be here!\n"); > + return IRQ_NONE; > + } returning IRQ_NONE is not what you want to do in this case. You want to setup a flag so that your runtime_resume() knows that there are pending events to be handled and you handle those in runtime_resume time. But to be frank, I don't see how this can trigger since we're calling pm_runtime_get_sync() from omap_i2c_xfer() which means by the time pm_runtime_get_sync() returns, assuming no errors, i2c module should be fully resumed and ready to go. Perhaps you have found a bug somewhere else ? Also, your 'We should never be here' message isn't helpfull at all. > @@ -905,6 +910,11 @@ omap_i2c_isr_thread(int this_irq, void *dev_id) > u16 stat; > int err = 0, count = 0; > > + if (pm_runtime_suspended(dev->dev)) { > + WARN_ONCE(true, "We should never be here!\n"); > + return IRQ_NONE; > + } because of IRQF_ONESHOT I can't see how this would *ever* be a valid check.
Hi Felipe, On 06/07/2013 10:02 PM, Felipe Balbi wrote: > Hi, > > On Fri, Jun 07, 2013 at 09:46:05PM +0300, Grygorii Strashko wrote: >> Add runtime check at the beginning of omap_i2c_isr/omap_i2c_isr_thread >> to be sure that i2c is enabled, before performing IRQ handling and accessing >> I2C IP registers: >> if (pm_runtime_suspended(dev->dev)) { >> WARN_ONCE(true, "We should never be here!\n"); >> return IRQ_NONE; >> } >> >> Produce warning in case if ISR called when i2c is disabled >> >> CC: Kevin Hilman <khilman@linaro.org> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >> --- >> drivers/i2c/busses/i2c-omap.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c >> index 97844ff..2dac598 100644 >> --- a/drivers/i2c/busses/i2c-omap.c >> +++ b/drivers/i2c/busses/i2c-omap.c >> @@ -885,6 +885,11 @@ omap_i2c_isr(int irq, void *dev_id) >> u16 stat; >> >> spin_lock(&dev->lock); >> + if (pm_runtime_suspended(dev->dev)) { >> + WARN_ONCE(true, "We should never be here!\n"); >> + return IRQ_NONE; >> + } > returning IRQ_NONE is not what you want to do in this case. You want to > setup a flag so that your runtime_resume() knows that there are pending > events to be handled and you handle those in runtime_resume time. I don't want to handle this IRQ - we should never be here. Will be changed to IRQ_HANDLED. > > But to be frank, I don't see how this can trigger since we're calling > pm_runtime_get_sync() from omap_i2c_xfer() which means by the time > pm_runtime_get_sync() returns, assuming no errors, i2c module should be > fully resumed and ready to go. Perhaps you have found a bug somewhere > else ? May be it's better to revert this patch: e3a36b207f76364c281aeecaf14c1b22a7247278 i2c: omap: remove unnecessary pm_runtime_suspended check which doesn't cover case when transfer is *finished*. Please, see https://patchwork.kernel.org/patch/2689211/ and cover-latter. > > Also, your 'We should never be here' message isn't helpfull at all. > >> @@ -905,6 +910,11 @@ omap_i2c_isr_thread(int this_irq, void *dev_id) >> u16 stat; >> int err = 0, count = 0; >> >> + if (pm_runtime_suspended(dev->dev)) { >> + WARN_ONCE(true, "We should never be here!\n"); >> + return IRQ_NONE; >> + } > because of IRQF_ONESHOT I can't see how this would *ever* be a valid > check. > Please, see https://patchwork.kernel.org/patch/2689211/ and cover-latter. Sorry, for delayed reply - I've had problems with my e-mail. - grygorii -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 19, 2013 at 09:42:25PM +0300, Grygorii Strashko wrote: > Hi Felipe, > On 06/07/2013 10:02 PM, Felipe Balbi wrote: > >Hi, > > > >On Fri, Jun 07, 2013 at 09:46:05PM +0300, Grygorii Strashko wrote: > >>Add runtime check at the beginning of omap_i2c_isr/omap_i2c_isr_thread > >>to be sure that i2c is enabled, before performing IRQ handling and accessing > >>I2C IP registers: > >> if (pm_runtime_suspended(dev->dev)) { > >> WARN_ONCE(true, "We should never be here!\n"); > >> return IRQ_NONE; > >> } > >> > >>Produce warning in case if ISR called when i2c is disabled > >> > >>CC: Kevin Hilman <khilman@linaro.org> > >>Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > >>--- > >> drivers/i2c/busses/i2c-omap.c | 10 ++++++++++ > >> 1 file changed, 10 insertions(+) > >> > >>diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > >>index 97844ff..2dac598 100644 > >>--- a/drivers/i2c/busses/i2c-omap.c > >>+++ b/drivers/i2c/busses/i2c-omap.c > >>@@ -885,6 +885,11 @@ omap_i2c_isr(int irq, void *dev_id) > >> u16 stat; > >> spin_lock(&dev->lock); > >>+ if (pm_runtime_suspended(dev->dev)) { > >>+ WARN_ONCE(true, "We should never be here!\n"); > >>+ return IRQ_NONE; > >>+ } > >returning IRQ_NONE is not what you want to do in this case. You want to > >setup a flag so that your runtime_resume() knows that there are pending > >events to be handled and you handle those in runtime_resume time. > I don't want to handle this IRQ - we should never be here. > Will be changed to IRQ_HANDLED. blindly returning IRQ_HANDLED won't do you any good either. Your line will be re-enabled anyway and you'll get another IRQ even being fired. If you have found a bug in the driver, fix it, don't try to mask it. > >But to be frank, I don't see how this can trigger since we're calling > >pm_runtime_get_sync() from omap_i2c_xfer() which means by the time > >pm_runtime_get_sync() returns, assuming no errors, i2c module should be > >fully resumed and ready to go. Perhaps you have found a bug somewhere > >else ? > May be it's better to revert this patch: > e3a36b207f76364c281aeecaf14c1b22a7247278 > i2c: omap: remove unnecessary pm_runtime_suspended check > > which doesn't cover case when transfer is *finished*. what happens when transfer is finished ? After we come out of the loop in omap_i2c_xfer() we will mark last busy and pm_runtime_put_autosuspend() 633 static int 634 omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) 635 { 636 struct omap_i2c_dev *dev = i2c_get_adapdata(adap); 637 int i; 638 int r; 639 640 r = pm_runtime_get_sync(dev->dev); 641 if (IS_ERR_VALUE(r)) 642 goto out; 643 644 r = omap_i2c_wait_for_bb(dev); 645 if (r < 0) 646 goto out; 647 648 if (dev->set_mpu_wkup_lat != NULL) 649 dev->set_mpu_wkup_lat(dev->dev, dev->latency); 650 651 for (i = 0; i < num; i++) { 652 r = omap_i2c_xfer_msg(adap, &msgs[i], (i == (num - 1))); 653 if (r != 0) 654 break; 655 } 656 657 if (r == 0) 658 r = num; 659 660 omap_i2c_wait_for_bb(dev); 661 662 if (dev->set_mpu_wkup_lat != NULL) 663 dev->set_mpu_wkup_lat(dev->dev, -1); 664 665 out: 666 pm_runtime_mark_last_busy(dev->dev); 667 pm_runtime_put_autosuspend(dev->dev); 668 return r; 669 } When that timer expires, we will mask the controller's IRQs on our ->runtime_suspend(). Now, if another IRQ comes before we ->runtime_suspend(), then we need to figure out what's generating that event, we don't want to be "fixing" what's not broken in this driver. > Please, see https://patchwork.kernel.org/patch/2689211/ and > cover-latter. that patch is fine, but doesn't seem like has nothing to do with what you're talking about here. > >Also, your 'We should never be here' message isn't helpfull at all. nor is your explanation of the problem. It's not sufficient. I'm telling you that we should never reach this case because that's the assumption that the driver makes. It assumes that no IRQs will be fired unless a transfer is started and by the time that for loop ends, no transfer will be started. > >>@@ -905,6 +910,11 @@ omap_i2c_isr_thread(int this_irq, void *dev_id) > >> u16 stat; > >> int err = 0, count = 0; > >>+ if (pm_runtime_suspended(dev->dev)) { > >>+ WARN_ONCE(true, "We should never be here!\n"); > >>+ return IRQ_NONE; > >>+ } > >because of IRQF_ONESHOT I can't see how this would *ever* be a valid > >check. > > > Please, see https://patchwork.kernel.org/patch/2689211/ and > cover-latter. explain to me how would we get to this point, meaning the IRQ thread handler, with the device disabled.
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index 97844ff..2dac598 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -885,6 +885,11 @@ omap_i2c_isr(int irq, void *dev_id) u16 stat; spin_lock(&dev->lock); + if (pm_runtime_suspended(dev->dev)) { + WARN_ONCE(true, "We should never be here!\n"); + return IRQ_NONE; + } + mask = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG); stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG); @@ -905,6 +910,11 @@ omap_i2c_isr_thread(int this_irq, void *dev_id) u16 stat; int err = 0, count = 0; + if (pm_runtime_suspended(dev->dev)) { + WARN_ONCE(true, "We should never be here!\n"); + return IRQ_NONE; + } + spin_lock_irqsave(&dev->lock, flags); do { bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
Add runtime check at the beginning of omap_i2c_isr/omap_i2c_isr_thread to be sure that i2c is enabled, before performing IRQ handling and accessing I2C IP registers: if (pm_runtime_suspended(dev->dev)) { WARN_ONCE(true, "We should never be here!\n"); return IRQ_NONE; } Produce warning in case if ISR called when i2c is disabled CC: Kevin Hilman <khilman@linaro.org> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> --- drivers/i2c/busses/i2c-omap.c | 10 ++++++++++ 1 file changed, 10 insertions(+)