Re: [PATCH V2] i2c: designware: fix race between subsequent xfers
On Fri, Jun 14, 2013 at 04:37:41PM +0200, Wolfram Sang wrote: On Fri, Jun 07, 2013 at 10:51:23AM +0200, Christian Ruppert wrote: [...] + /* +* We must disable the adapter before unlocking the dev-lock mutex +* below. Otherwise the hardware might continue generating interrupts +* which in turn causes a race condition with the following transfer. I added Needs some more investigation if the additional interrupts are a hardware bug or this driver doesn't handle them correctly yet. to the comment and Good idea, thanks. Applied to for-next, thanks! BTW since I am currently here: i2c-designware-core should be in the 'algos' directory, no? At the risk of passing for a complete moron: What exactly is the difference between I2C algos and I2C bus drivers? Greetings, Christian -- Christian Ruppert , christian.rupp...@abilis.com /| Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri _// | bilis Systems CH-1228 Plan-les-Ouates -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] i2c: designware: fix race between subsequent xfers
On Mon, 17 Jun 2013 10:19:33 +0200, Christian Ruppert wrote: On Fri, Jun 14, 2013 at 04:37:41PM +0200, Wolfram Sang wrote: BTW since I am currently here: i2c-designware-core should be in the 'algos' directory, no? At the risk of passing for a complete moron: What exactly is the difference between I2C algos and I2C bus drivers? The i2c/algos directory contains abstracted code which is common to multiple hardware implementations. The most popular of these is i2c-algo-bit which implements software-only I2C over virtually any pair of controllable pins (parallel port, GPIOs, etc.) As a general rule, i2c/algos should only contain reusable, architecture and platform independent code. All the actual hardware access should be delegated to the bus drivers, through callbacks. If this can't be done easily then i2c/algos is not the right place. -- Jean Delvare -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] i2c: designware: fix race between subsequent xfers
On Mon, Jun 17, 2013 at 10:33:36AM +0200, Jean Delvare wrote: On Mon, 17 Jun 2013 10:19:33 +0200, Christian Ruppert wrote: On Fri, Jun 14, 2013 at 04:37:41PM +0200, Wolfram Sang wrote: BTW since I am currently here: i2c-designware-core should be in the 'algos' directory, no? At the risk of passing for a complete moron: What exactly is the difference between I2C algos and I2C bus drivers? The i2c/algos directory contains abstracted code which is common to multiple hardware implementations. The most popular of these is i2c-algo-bit which implements software-only I2C over virtually any pair of controllable pins (parallel port, GPIOs, etc.) As a general rule, i2c/algos should only contain reusable, architecture and platform independent code. All the actual hardware access should be delegated to the bus drivers, through callbacks. If this can't be done easily then i2c/algos is not the right place. In this case, busses is the right place for the i2c-designware-core. This file contains the actual driver implementation (i.e. register access, interrupt handling etc.) for a dedicated I2C bus driver hardware block used in SOCs. Greetings, Christian -- Christian Ruppert , christian.rupp...@abilis.com /| Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri _// | bilis Systems CH-1228 Plan-les-Ouates -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V2] i2c: designware: fix race between subsequent xfers
The designware block is not always properly disabled in the case of
transfer errors. Interrupts from aborted transfers might be handled
after the data structures for the following transfer are initialised but
before the hardware is set up. This can corrupt the data structures to
the point that the system is stuck in an infinite interrupt loop (where
FIFOs are never emptied because dev-msg_read_idx == dev-msgs_num).
This patch cleanly disables the designware-i2c hardware at the end of
every transfer, be it successful or not.
This patch requires https://patchwork.kernel.org/patch/2601241/ to be
applied first.
Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
---
drivers/i2c/busses/i2c-designware-core.c | 10 --
1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-core.c
b/drivers/i2c/busses/i2c-designware-core.c
index b75d292..55a9991 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -588,11 +588,19 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
ret = wait_for_completion_timeout(dev-cmd_complete, HZ);
if (ret == 0) {
dev_err(dev-dev, controller timed out\n);
+ /* i2c_dw_init implicitly disables the adapter */
i2c_dw_init(dev);
ret = -ETIMEDOUT;
goto done;
}
+ /*
+* We must disable the adapter before unlocking the dev-lock mutex
+* below. Otherwise the hardware might continue generating interrupts
+* which in turn causes a race condition with the following transfer.
+*/
+ __i2c_dw_enable(dev, false);
+
if (dev-msg_err) {
ret = dev-msg_err;
goto done;
@@ -600,8 +608,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
/* no error */
if (likely(!dev-cmd_err)) {
- /* Disable the adapter */
- __i2c_dw_enable(dev, false);
ret = num;
goto done;
}
--
1.7.1
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] i2c: designware: fix race between subsequent xfers
On Fri, Jun 7, 2013 at 11:51 AM, Christian Ruppert christian.rupp...@abilis.com wrote: The designware block is not always properly disabled in the case of transfer errors. Interrupts from aborted transfers might be handled after the data structures for the following transfer are initialised but before the hardware is set up. This can corrupt the data structures to the point that the system is stuck in an infinite interrupt loop (where FIFOs are never emptied because dev-msg_read_idx == dev-msgs_num). This patch cleanly disables the designware-i2c hardware at the end of every transfer, be it successful or not. This patch requires https://patchwork.kernel.org/patch/2601241/ to be applied first. What if you just move disabling code from i2c_dw_xfer_init() to the top of i2c_dw_xfer() ? Will it help? -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
