Re: [PATCH 2/5] i2c: omap: add runtime check in isr to be sure thati2c is enabled

[Felipe Balbi]

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@xxxxxxxxxx>
> >>Signed-off-by: Grygorii Strashko <grygorii.strashko@xxxxxx>
> >>---
> >>  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.

-- 
balbi

Attachment: signature.asc
Description: Digital signature