[PATCH 0/9] Fix Marvell mv63xxx I2C driver

[PATCH 0/9] Fix Marvell mv63xxx I2C driver

[Russell King - ARM Linux]

This patch series fixes a whole chunk of problems with this driver,
discovered with the Marvell Armada 510 chip on the Solid-run cubox.

Most notable of these is the I2C driver aborting a transaction
because a signal is pending - which might be SIGPIPE or SIGALRM
for the process.  Meanwhile, the calling driver may be in the
middle of a critical read-modify-write cycle on a device register,
causing it to fail.

Other problems are race conditions in the handling of multi-part
messages, where we end up sending multiple start conditions -
sometimes more times than we have i2c_msg's to send.

Lastly is the rather poor probe error handling, which ranges from
non-existent to lacking propagating the provided error code.

[PATCH 1/9] I2C: mv64xxx: work around signals causing I2C transactions to be aborted

[Russell King]

Do not use interruptible waits in an I2C driver; if a process uses
signals (eg, Xorg uses SIGALRM and SIGPIPE) then these signals can
cause the I2C driver to abort a transaction in progress by another
driver, which can cause that driver to fail.  I2C drivers are not
expected to abort transactions on signals.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/i2c/busses/i2c-mv64xxx.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 8b20ef8..8d4c0a0 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -252,7 +252,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 		writel(drv_data->cntl_bits,
 			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
 		drv_data->block = 0;
-		wake_up_interruptible(&drv_data->waitq);
+		wake_up(&drv_data->waitq);
 		break;
 
 	case MV64XXX_I2C_ACTION_CONTINUE:
@@ -300,7 +300,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
 			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
 		drv_data->block = 0;
-		wake_up_interruptible(&drv_data->waitq);
+		wake_up(&drv_data->waitq);
 		break;
 
 	case MV64XXX_I2C_ACTION_INVALID:
@@ -315,7 +315,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
 			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
 		drv_data->block = 0;
-		wake_up_interruptible(&drv_data->waitq);
+		wake_up(&drv_data->waitq);
 		break;
 	}
 }
@@ -381,7 +381,7 @@ mv64xxx_i2c_wait_for_completion(struct mv64xxx_i2c_data *drv_data)
 	unsigned long	flags;
 	char		abort = 0;
 
-	time_left = wait_event_interruptible_timeout(drv_data->waitq,
+	time_left = wait_event_timeout(drv_data->waitq,
 		!drv_data->block, drv_data->adapter.timeout);
 
 	spin_lock_irqsave(&drv_data->lock, flags);
-- 
1.7.4.4

[PATCH 1/9] I2C: mv64xxx: work around signals causing I2C transactions to be aborted

[Jean-Francois Moine]

On Thu, 16 May 2013 21:30:59 +0100
Russell King <rmk+kernel@arm.linux.org.uk> wrote:

> Do not use interruptible waits in an I2C driver; if a process uses
> signals (eg, Xorg uses SIGALRM and SIGPIPE) then these signals can
> cause the I2C driver to abort a transaction in progress by another
> driver, which can cause that driver to fail.  I2C drivers are not
> expected to abort transactions on signals.

Hi Russell,

I had the same problem with my dove drm driver, but I don't fully agree
with your solution.

Using wait_event_timeout() stops the system, and reading the EDID from
an external screen may take some time.

Instead, as the problem occurs with the X server on HDMI exchanges,
I acted on the tda998x driver, simply masking the SIGALRM and SIGPIPE
signals on each drm driver request.

This mechanism works fine for me. Patch follows.

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCH 1/9] I2C: mv64xxx: work around signals causing I2C transactions to be aborted

[Russell King - ARM Linux]

On Fri, May 17, 2013 at 08:37:43AM +0200, Jean-Francois Moine wrote:
> On Thu, 16 May 2013 21:30:59 +0100
> Russell King <rmk+kernel@arm.linux.org.uk> wrote:
> 
> > Do not use interruptible waits in an I2C driver; if a process uses
> > signals (eg, Xorg uses SIGALRM and SIGPIPE) then these signals can
> > cause the I2C driver to abort a transaction in progress by another
> > driver, which can cause that driver to fail.  I2C drivers are not
> > expected to abort transactions on signals.
> 
> Hi Russell,
> 
> I had the same problem with my dove drm driver, but I don't fully agree
> with your solution.
> 
> Using wait_event_timeout() stops the system, and reading the EDID from
> an external screen may take some time.

It doesn't stop the system.  It stops the process until that has completed.
Using the DRM TDA19988 driver, things are nice and quick while reading
the EDID, because it does reads an entire EDID block using one message.

Blocking the signals like you do has exactly the same effect - you
prevent the I2C driver being interrupted by signals.

[PATCH 1/9] I2C: mv64xxx: work around signals causing I2C transactions to be aborted

[Wolfram Sang]

On Thu, May 16, 2013 at 09:30:59PM +0100, Russell King wrote:
> Do not use interruptible waits in an I2C driver; if a process uses
> signals (eg, Xorg uses SIGALRM and SIGPIPE) then these signals can
> cause the I2C driver to abort a transaction in progress by another
> driver, which can cause that driver to fail.  I2C drivers are not
> expected to abort transactions on signals.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

Applied to for-current, thanks!

Rest will be reviewed later...

[PATCH 1/9] I2C: mv64xxx: work around signals causing I2C transactions to be aborted

[Mark A. Greer]

On Thu, May 16, 2013 at 09:30:59PM +0100, Russell King wrote:
> Do not use interruptible waits in an I2C driver; if a process uses
> signals (eg, Xorg uses SIGALRM and SIGPIPE) then these signals can
> cause the I2C driver to abort a transaction in progress by another
> driver, which can cause that driver to fail.  I2C drivers are not
> expected to abort transactions on signals.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---

I don't have hardware to test but I have no issues with these patches so,
FWIW:

Acked-by: Mark A. Greer <mgreer@animalcreek.com>

for the entire series.

[PATCH 2/9] I2C: mv64xxx: use return value from mv64xxx_i2c_map_regs()

[Russell King]

mv64xxx_i2c_map_regs() already returns an error code, so lets
propagate that to mv64xxx_i2c_probe()'s caller.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/i2c/busses/i2c-mv64xxx.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 8d4c0a0..0339cd8 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -619,10 +619,9 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 	if (!drv_data)
 		return -ENOMEM;
 
-	if (mv64xxx_i2c_map_regs(pd, drv_data)) {
-		rc = -ENODEV;
+	rc = mv64xxx_i2c_map_regs(pd, drv_data);
+	if (rc)
 		goto exit_kfree;
-	}
 
 	strlcpy(drv_data->adapter.name, MV64XXX_I2C_CTLR_NAME " adapter",
 		sizeof(drv_data->adapter.name));
-- 
1.7.4.4

[PATCH 3/9] I2C: mv64xxx: use devm_ioremap_resource()

[Russell King]

Eliminate reg_base_p and reg_size, mv64xxx_i2c_unmap_regs() and an
unchecked ioremap() return from this driver by using the devm_*
API for requesting and ioremapping resources.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/i2c/busses/i2c-mv64xxx.c |   46 +++++---------------------------------
 1 files changed, 6 insertions(+), 40 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 0339cd8..19cc9bf 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -92,8 +92,6 @@ struct mv64xxx_i2c_data {
 	u32			aborting;
 	u32			cntl_bits;
 	void __iomem		*reg_base;
-	u32			reg_base_p;
-	u32			reg_size;
 	u32			addr1;
 	u32			addr2;
 	u32			bytes_left;
@@ -495,40 +493,6 @@ static const struct i2c_algorithm mv64xxx_i2c_algo = {
  *
  *****************************************************************************
  */
-static int
-mv64xxx_i2c_map_regs(struct platform_device *pd,
-	struct mv64xxx_i2c_data *drv_data)
-{
-	int size;
-	struct resource	*r = platform_get_resource(pd, IORESOURCE_MEM, 0);
-
-	if (!r)
-		return -ENODEV;
-
-	size = resource_size(r);
-
-	if (!request_mem_region(r->start, size, drv_data->adapter.name))
-		return -EBUSY;
-
-	drv_data->reg_base = ioremap(r->start, size);
-	drv_data->reg_base_p = r->start;
-	drv_data->reg_size = size;
-
-	return 0;
-}
-
-static void
-mv64xxx_i2c_unmap_regs(struct mv64xxx_i2c_data *drv_data)
-{
-	if (drv_data->reg_base) {
-		iounmap(drv_data->reg_base);
-		release_mem_region(drv_data->reg_base_p, drv_data->reg_size);
-	}
-
-	drv_data->reg_base = NULL;
-	drv_data->reg_base_p = 0;
-}
-
 #ifdef CONFIG_OF
 static int
 mv64xxx_calc_freq(const int tclk, const int n, const int m)
@@ -610,6 +574,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 {
 	struct mv64xxx_i2c_data		*drv_data;
 	struct mv64xxx_i2c_pdata	*pdata = pd->dev.platform_data;
+	struct resource	*r;
 	int	rc;
 
 	if ((!pdata && !pd->dev.of_node))
@@ -619,9 +584,12 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 	if (!drv_data)
 		return -ENOMEM;
 
-	rc = mv64xxx_i2c_map_regs(pd, drv_data);
-	if (rc)
+	r = platform_get_resource(pd, IORESOURCE_MEM, 0);
+	drv_data->reg_base = devm_ioremap_resource(&pd->dev, r);
+	if (IS_ERR(drv_data->reg_base)) {
+		rc = PTR_ERR(drv_data->reg_base);
 		goto exit_kfree;
+	}
 
 	strlcpy(drv_data->adapter.name, MV64XXX_I2C_CTLR_NAME " adapter",
 		sizeof(drv_data->adapter.name));
@@ -690,7 +658,6 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 		clk_unprepare(drv_data->clk);
 	}
 #endif
-		mv64xxx_i2c_unmap_regs(drv_data);
 	exit_kfree:
 		kfree(drv_data);
 	return rc;
@@ -704,7 +671,6 @@ mv64xxx_i2c_remove(struct platform_device *dev)
 
 	rc = i2c_del_adapter(&drv_data->adapter);
 	free_irq(drv_data->irq, drv_data);
-	mv64xxx_i2c_unmap_regs(drv_data);
 #if defined(CONFIG_HAVE_CLK)
 	/* Not all platforms have a clk */
 	if (!IS_ERR(drv_data->clk)) {
-- 
1.7.4.4

[PATCH 3/9] I2C: mv64xxx: use devm_ioremap_resource()

[Jean-Francois Moine]

On Thu, 16 May 2013 21:33:09 +0100
Russell King <rmk+kernel@arm.linux.org.uk> wrote:

> Eliminate reg_base_p and reg_size, mv64xxx_i2c_unmap_regs() and an
> unchecked ioremap() return from this driver by using the devm_*
> API for requesting and ioremapping resources.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/i2c/busses/i2c-mv64xxx.c |   46 +++++---------------------------------
>  1 files changed, 6 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index 0339cd8..19cc9bf 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
	[snip]
> @@ -704,7 +671,6 @@ mv64xxx_i2c_remove(struct platform_device *dev)
>  
>  	rc = i2c_del_adapter(&drv_data->adapter);
>  	free_irq(drv_data->irq, drv_data);
> -	mv64xxx_i2c_unmap_regs(drv_data);
>  #if defined(CONFIG_HAVE_CLK)
>  	/* Not all platforms have a clk */
>  	if (!IS_ERR(drv_data->clk)) {

The patch does not apply: it seems it lacks:

+	int rc;

-	i2c_del_adapter(&drv_data->adapter);
+	rc = i2c_del_adapter(&drv_data->adapter);

-	return 0;
+	return rc;

BTW, is this return code useful?

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCH 3/9] I2C: mv64xxx: use devm_ioremap_resource()

[Russell King - ARM Linux]

On Fri, May 17, 2013 at 11:23:16AM +0200, Jean-Francois Moine wrote:
> On Thu, 16 May 2013 21:33:09 +0100
> Russell King <rmk+kernel@arm.linux.org.uk> wrote:
> 
> > Eliminate reg_base_p and reg_size, mv64xxx_i2c_unmap_regs() and an
> > unchecked ioremap() return from this driver by using the devm_*
> > API for requesting and ioremapping resources.
> > 
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >  drivers/i2c/busses/i2c-mv64xxx.c |   46 +++++---------------------------------
> >  1 files changed, 6 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> > index 0339cd8..19cc9bf 100644
> > --- a/drivers/i2c/busses/i2c-mv64xxx.c
> > +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> 	[snip]
> > @@ -704,7 +671,6 @@ mv64xxx_i2c_remove(struct platform_device *dev)
> >  
> >  	rc = i2c_del_adapter(&drv_data->adapter);
> >  	free_irq(drv_data->irq, drv_data);
> > -	mv64xxx_i2c_unmap_regs(drv_data);
> >  #if defined(CONFIG_HAVE_CLK)
> >  	/* Not all platforms have a clk */
> >  	if (!IS_ERR(drv_data->clk)) {
> 
> The patch does not apply: it seems it lacks:

I should've mentioned that the patches are against v3.9, not v3.10-rc1.

> +	int rc;
> 
> -	i2c_del_adapter(&drv_data->adapter);
> +	rc = i2c_del_adapter(&drv_data->adapter);
> 
> -	return 0;
> +	return rc;
> 
> BTW, is this return code useful?

No it isn't.  Removals *can't* fail.  Once the remove function is called,
the device _will_ be unbound no matter what the return value of that
function is.  The module will also be unloaded (if that's why it's being
unbound) whether or not you return an error.

So, removals must always succeed.  Why their return type hasn't been void
from the start I've no idea - but that's a question for Patrick Mochel
who implemented the original driver model design.

[PATCH 4/9] I2C: mv64xxx: use devm_clk_get() to avoid missing clk_put()

[Russell King]

This driver forgets to use clk_put().  Rather than adding clk_put(),
lets instead use devm_clk_get() to obtain this clock so that it's
automatically handled on cleanup.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/i2c/busses/i2c-mv64xxx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 19cc9bf..0b4da77 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -599,7 +599,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 
 #if defined(CONFIG_HAVE_CLK)
 	/* Not all platforms have a clk */
-	drv_data->clk = clk_get(&pd->dev, NULL);
+	drv_data->clk = devm_clk_get(&pd->dev, NULL);
 	if (!IS_ERR(drv_data->clk)) {
 		clk_prepare(drv_data->clk);
 		clk_enable(drv_data->clk);
-- 
1.7.4.4

[PATCH 5/9] I2C: mv64xxx: use devm_kzalloc()

[Russell King]

As we're changing to using devm_* APIs to fix various problems
in this driver, lets also do devm_kzalloc() while we're here too.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/i2c/busses/i2c-mv64xxx.c |   24 ++++++++++--------------
 1 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 0b4da77..841792f 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -580,16 +580,15 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 	if ((!pdata && !pd->dev.of_node))
 		return -ENODEV;
 
-	drv_data = kzalloc(sizeof(struct mv64xxx_i2c_data), GFP_KERNEL);
+	drv_data = devm_kzalloc(&pd->dev, sizeof(struct mv64xxx_i2c_data),
+				GFP_KERNEL);
 	if (!drv_data)
 		return -ENOMEM;
 
 	r = platform_get_resource(pd, IORESOURCE_MEM, 0);
 	drv_data->reg_base = devm_ioremap_resource(&pd->dev, r);
-	if (IS_ERR(drv_data->reg_base)) {
-		rc = PTR_ERR(drv_data->reg_base);
-		goto exit_kfree;
-	}
+	if (IS_ERR(drv_data->reg_base))
+		return PTR_ERR(drv_data->reg_base);
 
 	strlcpy(drv_data->adapter.name, MV64XXX_I2C_CTLR_NAME " adapter",
 		sizeof(drv_data->adapter.name));
@@ -613,11 +612,11 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 	} else if (pd->dev.of_node) {
 		rc = mv64xxx_of_config(drv_data, pd->dev.of_node);
 		if (rc)
-			goto exit_unmap_regs;
+			goto exit_clk;
 	}
 	if (drv_data->irq < 0) {
 		rc = -ENXIO;
-		goto exit_unmap_regs;
+		goto exit_clk;
 	}
 
 	drv_data->adapter.dev.parent = &pd->dev;
@@ -637,7 +636,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 			"mv64xxx: Can't register intr handler irq: %d\n",
 			drv_data->irq);
 		rc = -EINVAL;
-		goto exit_unmap_regs;
+		goto exit_clk;
 	} else if ((rc = i2c_add_numbered_adapter(&drv_data->adapter)) != 0) {
 		dev_err(&drv_data->adapter.dev,
 			"mv64xxx: Can't add i2c adapter, rc: %d\n", -rc);
@@ -648,9 +647,9 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 
 	return 0;
 
-	exit_free_irq:
-		free_irq(drv_data->irq, drv_data);
-	exit_unmap_regs:
+exit_free_irq:
+	free_irq(drv_data->irq, drv_data);
+exit_clk:
 #if defined(CONFIG_HAVE_CLK)
 	/* Not all platforms have a clk */
 	if (!IS_ERR(drv_data->clk)) {
@@ -658,8 +657,6 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 		clk_unprepare(drv_data->clk);
 	}
 #endif
-	exit_kfree:
-		kfree(drv_data);
 	return rc;
 }
 
@@ -678,7 +675,6 @@ mv64xxx_i2c_remove(struct platform_device *dev)
 		clk_unprepare(drv_data->clk);
 	}
 #endif
-	kfree(drv_data);
 
 	return rc;
 }
-- 
1.7.4.4

[PATCH 6/9] I2C: mv64xxx: fix error handling for request_irq()

[Russell King]

Propagate the error code from request_irq() rather than ignoring it
entirely.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/i2c/busses/i2c-mv64xxx.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 841792f..a82ab25 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -630,12 +630,12 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 
 	mv64xxx_i2c_hw_init(drv_data);
 
-	if (request_irq(drv_data->irq, mv64xxx_i2c_intr, 0,
-			MV64XXX_I2C_CTLR_NAME, drv_data)) {
+	rc = request_irq(drv_data->irq, mv64xxx_i2c_intr, 0,
+			 MV64XXX_I2C_CTLR_NAME, drv_data);
+	if (rc) {
 		dev_err(&drv_data->adapter.dev,
-			"mv64xxx: Can't register intr handler irq: %d\n",
-			drv_data->irq);
-		rc = -EINVAL;
+			"mv64xxx: Can't register intr handler irq%d: %d\n",
+			drv_data->irq, rc);
 		goto exit_clk;
 	} else if ((rc = i2c_add_numbered_adapter(&drv_data->adapter)) != 0) {
 		dev_err(&drv_data->adapter.dev,
-- 
1.7.4.4

[PATCH 7/9] I2C: mv64xxx: remove I2C_M_NOSTART code

[Russell King]

As this driver does not advertise protocol mangling support
(I2C_FUNC_PROTOCOL_MANGLING is not set), having code to act on
I2C_M_NOSTART is illogical, and in any case isn't supportable on
anything but the first message - which makes no sense.  Remove
the I2C_M_NOSTART code.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/i2c/busses/i2c-mv64xxx.c |   26 +++++---------------------
 1 files changed, 5 insertions(+), 21 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index a82ab25..d160f8c 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -419,28 +419,12 @@ mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg,
 	spin_lock_irqsave(&drv_data->lock, flags);
 	mv64xxx_i2c_prepare_for_io(drv_data, msg);
 
-	if (unlikely(msg->flags & I2C_M_NOSTART)) { /* Skip start/addr phases */
-		if (drv_data->msg->flags & I2C_M_RD) {
-			/* No action to do, wait for slave to send a byte */
-			drv_data->action = MV64XXX_I2C_ACTION_CONTINUE;
-			drv_data->state =
-				MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_DATA;
-		} else {
-			drv_data->action = MV64XXX_I2C_ACTION_SEND_DATA;
-			drv_data->state =
-				MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_ACK;
-			drv_data->bytes_left--;
-		}
+	if (is_first) {
+		drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
 	} else {
-		if (is_first) {
-			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
-			drv_data->state =
-				MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
-		} else {
-			drv_data->action = MV64XXX_I2C_ACTION_SEND_ADDR_1;
-			drv_data->state =
-				MV64XXX_I2C_STATE_WAITING_FOR_ADDR_1_ACK;
-		}
+		drv_data->action = MV64XXX_I2C_ACTION_SEND_ADDR_1;
+		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_ADDR_1_ACK;
 	}
 
 	drv_data->send_stop = is_last;
-- 
1.7.4.4

[PATCH 7/9] I2C: mv64xxx: remove I2C_M_NOSTART code

[Mark Brown]

On Thu, May 16, 2013 at 09:37:11PM +0100, Russell King wrote:

> As this driver does not advertise protocol mangling support
> (I2C_FUNC_PROTOCOL_MANGLING is not set), having code to act on
> I2C_M_NOSTART is illogical, and in any case isn't supportable on
> anything but the first message - which makes no sense.  Remove
> the I2C_M_NOSTART code.

There is I2C_FUNC_NOSTART, though if it's only possible to use it on the
first message that is pretty much useless as you say.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130522/ffffb826/attachment.sig>

[PATCH 8/9] I2C: mv64xxx: move mv64xxx_i2c_prepare_for_io()

[Russell King]

Move mv64xxx_i2c_prepare_for_io() higher up in the driver to avoid a
future forward declaration for this function.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/i2c/busses/i2c-mv64xxx.c |   52 +++++++++++++++++++-------------------
 1 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index d160f8c..7457ef5 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -110,6 +110,32 @@ struct mv64xxx_i2c_data {
 	struct i2c_adapter	adapter;
 };
 
+static void
+mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
+	struct i2c_msg *msg)
+{
+	u32	dir = 0;
+
+	drv_data->msg = msg;
+	drv_data->byte_posn = 0;
+	drv_data->bytes_left = msg->len;
+	drv_data->aborting = 0;
+	drv_data->rc = 0;
+	drv_data->cntl_bits = MV64XXX_I2C_REG_CONTROL_ACK |
+		MV64XXX_I2C_REG_CONTROL_INTEN | MV64XXX_I2C_REG_CONTROL_TWSIEN;
+
+	if (msg->flags & I2C_M_RD)
+		dir = 1;
+
+	if (msg->flags & I2C_M_TEN) {
+		drv_data->addr1 = 0xf0 | (((u32)msg->addr & 0x300) >> 7) | dir;
+		drv_data->addr2 = (u32)msg->addr & 0xff;
+	} else {
+		drv_data->addr1 = ((u32)msg->addr & 0x7f) << 1 | dir;
+		drv_data->addr2 = 0;
+	}
+}
+
 /*
  *****************************************************************************
  *
@@ -347,32 +373,6 @@ mv64xxx_i2c_intr(int irq, void *dev_id)
  *****************************************************************************
  */
 static void
-mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
-	struct i2c_msg *msg)
-{
-	u32	dir = 0;
-
-	drv_data->msg = msg;
-	drv_data->byte_posn = 0;
-	drv_data->bytes_left = msg->len;
-	drv_data->aborting = 0;
-	drv_data->rc = 0;
-	drv_data->cntl_bits = MV64XXX_I2C_REG_CONTROL_ACK |
-		MV64XXX_I2C_REG_CONTROL_INTEN | MV64XXX_I2C_REG_CONTROL_TWSIEN;
-
-	if (msg->flags & I2C_M_RD)
-		dir = 1;
-
-	if (msg->flags & I2C_M_TEN) {
-		drv_data->addr1 = 0xf0 | (((u32)msg->addr & 0x300) >> 7) | dir;
-		drv_data->addr2 = (u32)msg->addr & 0xff;
-	} else {
-		drv_data->addr1 = ((u32)msg->addr & 0x7f) << 1 | dir;
-		drv_data->addr2 = 0;
-	}
-}
-
-static void
 mv64xxx_i2c_wait_for_completion(struct mv64xxx_i2c_data *drv_data)
 {
 	long		time_left;
-- 
1.7.4.4

[PATCH 9/9] I2C: mv64xxx: fix race between FSM/interrupt and process context

[Russell King]

Asking for a multi-part message to be handled by this driver is racy; it
has been observed that the following sequence is possible with this
driver:

	- send start
	- send address + write
	- send data
	- send (repeated) start
	- send address + write
	- send (repeated) start
	- send address + read
	- unrecoverable bus hang (except by system reset)

The problem is that the interrupt handling sees the next event after the
first repeated start is sent - the IFLG bit is set in the register even
though INTEN is disabled.

Let's fix this by moving all of the message processing into interrupt
context, rather than having it partly in IRQ and partly in process
context.  This allows us to move immediately to the next message in the
interrupt handler and get on with the transfer, rather than incuring a
couple of scheduling switches to get the next message.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/i2c/busses/i2c-mv64xxx.c |   54 ++++++++++++++++++++++++--------------
 1 files changed, 34 insertions(+), 20 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 7457ef5..a2e4633 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -86,6 +86,8 @@ enum {
 };
 
 struct mv64xxx_i2c_data {
+	struct i2c_msg		*msgs;
+	int			num_msgs;
 	int			irq;
 	u32			state;
 	u32			action;
@@ -194,7 +196,7 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
 		if ((drv_data->bytes_left == 0)
 				|| (drv_data->aborting
 					&& (drv_data->byte_posn != 0))) {
-			if (drv_data->send_stop) {
+			if (drv_data->send_stop || drv_data->aborting) {
 				drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP;
 				drv_data->state = MV64XXX_I2C_STATE_IDLE;
 			} else {
@@ -271,12 +273,25 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 {
 	switch(drv_data->action) {
 	case MV64XXX_I2C_ACTION_SEND_RESTART:
+		/* We should only get here if we have further messages */
+		BUG_ON(drv_data->num_msgs == 0);
+
 		drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
-		drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
 		writel(drv_data->cntl_bits,
 			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
-		drv_data->block = 0;
-		wake_up(&drv_data->waitq);
+
+		drv_data->msgs++;
+		drv_data->num_msgs--;
+
+		/* Setup for the next message */
+		mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
+
+		/*
+		 * We're never at the start of the message here, and by this
+		 * time it's already too late to do any protocol mangling.
+		 * Thankfully, do not advertise support for that feature.
+		 */
+		drv_data->send_stop = drv_data->num_msgs == 1;
 		break;
 
 	case MV64XXX_I2C_ACTION_CONTINUE:
@@ -412,20 +427,15 @@ mv64xxx_i2c_wait_for_completion(struct mv64xxx_i2c_data *drv_data)
 
 static int
 mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg,
-				int is_first, int is_last)
+				int is_last)
 {
 	unsigned long	flags;
 
 	spin_lock_irqsave(&drv_data->lock, flags);
 	mv64xxx_i2c_prepare_for_io(drv_data, msg);
 
-	if (is_first) {
-		drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
-		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
-	} else {
-		drv_data->action = MV64XXX_I2C_ACTION_SEND_ADDR_1;
-		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_ADDR_1_ACK;
-	}
+	drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+	drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
 
 	drv_data->send_stop = is_last;
 	drv_data->block = 1;
@@ -453,16 +463,20 @@ static int
 mv64xxx_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 {
 	struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);
-	int	i, rc;
+	int rc, ret = num;
 
-	for (i = 0; i < num; i++) {
-		rc = mv64xxx_i2c_execute_msg(drv_data, &msgs[i],
-						i == 0, i + 1 == num);
-		if (rc < 0)
-			return rc;
-	}
+	BUG_ON(drv_data->msgs != NULL);
+	drv_data->msgs = msgs;
+	drv_data->num_msgs = num;
+
+	rc = mv64xxx_i2c_execute_msg(drv_data, &msgs[0], num == 1);
+	if (rc < 0)
+		ret = rc;
+
+	drv_data->num_msgs = 0;
+	drv_data->msgs = NULL;
 
-	return num;
+	return ret;
 }
 
 static const struct i2c_algorithm mv64xxx_i2c_algo = {
-- 
1.7.4.4

[PATCH 9/9] I2C: mv64xxx: fix race between FSM/interrupt and process context

[Wolfram Sang]

> @@ -271,12 +273,25 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  {
>  	switch(drv_data->action) {
>  	case MV64XXX_I2C_ACTION_SEND_RESTART:
> +		/* We should only get here if we have further messages */
> +		BUG_ON(drv_data->num_msgs == 0);
> +

...

> @@ -453,16 +463,20 @@ static int
>  mv64xxx_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  {
>  	struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);
> -	int	i, rc;
> +	int rc, ret = num;
>  
> -	for (i = 0; i < num; i++) {
> -		rc = mv64xxx_i2c_execute_msg(drv_data, &msgs[i],
> -						i == 0, i + 1 == num);
> -		if (rc < 0)
> -			return rc;
> -	}
> +	BUG_ON(drv_data->msgs != NULL);

Can't we handle this more gracefully than to halt the kernel? Like
WARN_ON and resetting the controller or disabling the bus or...

[PATCH 9/9] I2C: mv64xxx: fix race between FSM/interrupt and process context

[Russell King - ARM Linux]

On Fri, May 17, 2013 at 11:51:51AM +0200, Wolfram Sang wrote:
> >  {
> >  	switch(drv_data->action) {
> >  	case MV64XXX_I2C_ACTION_SEND_RESTART:
> > +		/* We should only get here if we have further messages */
> > +		BUG_ON(drv_data->num_msgs == 0);
> > +
> 
> ...
> 
> > @@ -453,16 +463,20 @@ static int
> >  mv64xxx_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> >  {
> >  	struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);
> > -	int	i, rc;
> > +	int rc, ret = num;
> >  
> > -	for (i = 0; i < num; i++) {
> > -		rc = mv64xxx_i2c_execute_msg(drv_data, &msgs[i],
> > -						i == 0, i + 1 == num);
> > -		if (rc < 0)
> > -			return rc;
> > -	}
> > +	BUG_ON(drv_data->msgs != NULL);
> 
> Can't we handle this more gracefully than to halt the kernel? Like
> WARN_ON and resetting the controller or disabling the bus or...

Well, the latter really is something which should never ever happen,
and if it does happen it can only really be because something's
screwed up in the locking in the I2C layer.

The former is more probable, and I thought about that, but I don't
have any good alternative solution.  Given the problems I've seen,
I don't think resetting the controller is really an option, because
that'll likely cause the bus to lock (that's the original problem
which I'm trying to solve in this patch.)  The thing really does
have to work according to the I2C protocol otherwise stuff goes
irrecoverably wrong to the point of needing an entire system reset.

[PATCH 9/9] I2C: mv64xxx: fix race between FSM/interrupt and process context

[Wolfram Sang]

On Fri, May 17, 2013 at 11:00:16AM +0100, Russell King - ARM Linux wrote:
> On Fri, May 17, 2013 at 11:51:51AM +0200, Wolfram Sang wrote:
> > >  {
> > >  	switch(drv_data->action) {
> > >  	case MV64XXX_I2C_ACTION_SEND_RESTART:
> > > +		/* We should only get here if we have further messages */
> > > +		BUG_ON(drv_data->num_msgs == 0);
> > > +
> > 
> > ...
> > 
> > > @@ -453,16 +463,20 @@ static int
> > >  mv64xxx_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> > >  {
> > >  	struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);
> > > -	int	i, rc;
> > > +	int rc, ret = num;
> > >  
> > > -	for (i = 0; i < num; i++) {
> > > -		rc = mv64xxx_i2c_execute_msg(drv_data, &msgs[i],
> > > -						i == 0, i + 1 == num);
> > > -		if (rc < 0)
> > > -			return rc;
> > > -	}
> > > +	BUG_ON(drv_data->msgs != NULL);
> > 
> > Can't we handle this more gracefully than to halt the kernel? Like
> > WARN_ON and resetting the controller or disabling the bus or...
> 
> Well, the latter really is something which should never ever happen,
> and if it does happen it can only really be because something's
> screwed up in the locking in the I2C layer.

I'd think we should trust the layer here.

> The former is more probable, and I thought about that, but I don't
> have any good alternative solution.  Given the problems I've seen,
> I don't think resetting the controller is really an option, because
> that'll likely cause the bus to lock (that's the original problem
> which I'm trying to solve in this patch.)  The thing really does
> have to work according to the I2C protocol otherwise stuff goes
> irrecoverably wrong to the point of needing an entire system reset.

Fine with me for now. If somebody later has a setup where I2C slaves can
be reset (e.g. via GPIO), so a complete bus reset is possible, we might
need another solution, then.

Thanks,

   Wolfram

[PATCH 0/9] Fix Marvell mv63xxx I2C driver

[Wolfram Sang]

On Thu, May 16, 2013 at 09:29:21PM +0100, Russell King - ARM Linux wrote:
> This patch series fixes a whole chunk of problems with this driver,
> discovered with the Marvell Armada 510 chip on the Solid-run cubox.
> 
> Most notable of these is the I2C driver aborting a transaction
> because a signal is pending - which might be SIGPIPE or SIGALRM
> for the process.  Meanwhile, the calling driver may be in the
> middle of a critical read-modify-write cycle on a device register,
> causing it to fail.
> 
> Other problems are race conditions in the handling of multi-part
> messages, where we end up sending multiple start conditions -
> sometimes more times than we have i2c_msg's to send.
> 
> Lastly is the rather poor probe error handling, which ranges from
> non-existent to lacking propagating the provided error code.

Applied rest of the series to for-next, thanks!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130605/32e7753b/attachment.sig>