| Message ID | 1370595083-801-1-git-send-email-christian.ruppert@abilis.com |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, Jun 7, 2013 at 11:51 AM, Christian Ruppert <christian.ruppert@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 07, 2013 at 10:51:23AM +0200, Christian Ruppert 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. These last two lines should be below "---". > > Signed-off-by: Christian Ruppert <christian.ruppert@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. 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 Applied to for-next, thanks! BTW since I am currently here: i2c-designware-core should be in the 'algos' directory, no?
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
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.
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: > > 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. > > These last two lines should be below "---". > > > > > Signed-off-by: Christian Ruppert <christian.ruppert@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. > > 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 > > Applied to for-next, thanks! Sorry for the late response (was on vacation). This patch looks good to me as well. Thanks for applying it! -- 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 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
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; }
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.ruppert@abilis.com> --- drivers/i2c/busses/i2c-designware-core.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)