Re: [PATCH 2/5] i2c: omap: add runtime check in isr to be sure that i2c is enabled
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 khil...@linaro.org
Signed-off-by: Grygorii Strashko grygorii.stras...@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-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] i2c: omap: add runtime check in isr to be sure that i2c is enabled
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 khil...@linaro.org
Signed-off-by: Grygorii Strashko grygorii.stras...@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.
--
balbi
signature.asc
Description: Digital signature
[PATCH 2/5] i2c: omap: add runtime check in isr to be sure that i2c is enabled
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 khil...@linaro.org
Signed-off-by: Grygorii Strashko grygorii.stras...@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;
+ }
+
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);
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] i2c: omap: add runtime check in isr to be sure that i2c is enabled
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 khil...@linaro.org
Signed-off-by: Grygorii Strashko grygorii.stras...@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.
--
balbi
signature.asc
Description: Digital signature
