Patchwork [V2] i2c: designware: fix race between subsequent xfers

login
register
mail settings
Submitter Christian Ruppert
Date June 7, 2013, 8:51 a.m.
Message ID <1370595083-801-1-git-send-email-christian.ruppert@abilis.com>
Download mbox | patch
Permalink /patch/249622/
State Accepted
Headers show

Comments

Christian Ruppert - June 7, 2013, 8:51 a.m.
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(-)
Andy Shevchenko - June 7, 2013, 9:19 a.m.
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
Wolfram Sang - June 14, 2013, 2:37 p.m.
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?
Christian Ruppert - June 17, 2013, 8:19 a.m.
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
Jean Delvare - June 17, 2013, 8:33 a.m.
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.
Mika Westerberg - June 17, 2013, 8:34 a.m.
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
Christian Ruppert - June 17, 2013, 9:01 a.m.
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

Patch

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;
 	}