Skip to contentSkip to conversation

Fix Marvell mv63xxx I2C driver

PATCH New Russell King - ARM Linux 6 participants 21 messages May 16, 2013 – Jun 5, 2013

Subsystems (2)
I2C subsystem host drivers I2C MV64XXX Marvell and Allwinner driver
lore
Share & tools
lore

Russell King - ARM Linux

author
#0 lore raw

cover Fix Marvell mv63xxx I2C driver

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.

#1 lore raw

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

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 <[email protected]>
---
 drivers/i2c/busses/i2c-mv64xxx.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-mv64xxx.c +4 -4
index 8b20ef8..8d4c0a0 100644
@@ -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);
Signature · 1 line
1.7.4.4
#2 lore raw

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

Quoted 5 lines · On Thu, 16 May 2013 21:30:59 +0100 Russell King <[email protected]>…

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.

Signature · 2 lines
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
author
#3 lore raw

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

Quoted 9 lines · Jean-Francois Moine
Quoted 5 lines · On Thu, 16 May 2013 21:30:59 +0100 Russell King <[email protected]>…

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.

Quoted 7 lines

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.

Applied
#4 lore raw

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

In reply to Russell King
Quoted 7 lines · 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 <[email protected]>

Applied to for-current, thanks!

Rest will be reviewed later...

#5 lore raw

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

In reply to Russell King
Quoted 8 lines · 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 <[email protected]>

---

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

Acked-by Mark A. Greer <[email protected]>

for the entire series.

#6 lore raw

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

In reply to Russell King - ARM Linux

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

Signed-off-by Russell King <[email protected]>
---
 drivers/i2c/busses/i2c-mv64xxx.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)
drivers/i2c/busses/i2c-mv64xxx.c +2 -3
index 8d4c0a0..0339cd8 100644
@@ -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));
Signature · 1 line
1.7.4.4
#7 lore raw

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

In reply to Russell King - ARM Linux

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 <[email protected]>
---
 drivers/i2c/busses/i2c-mv64xxx.c |   46 +++++---------------------------------
 1 files changed, 6 insertions(+), 40 deletions(-)
drivers/i2c/busses/i2c-mv64xxx.c +6 -40
index 0339cd8..19cc9bf 100644
@@ -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)) {
Signature · 1 line
1.7.4.4
#8 lore raw

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

drivers/i2c/busses/i2c-mv64xxx.c

Quoted 13 lines · On Thu, 16 May 2013 21:33:09 +0100 Russell King <[email protected]>…

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 <[email protected]>
---
 drivers/i2c/busses/i2c-mv64xxx.c |   46 +++++---------------------------------
 1 files changed, 6 insertions(+), 40 deletions(-)
drivers/i2c/busses/i2c-mv64xxx.c +0 -0
index 0339cd8..19cc9bf 100644

	[snip]

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_remove(struct platform_device *dev) · line 676

Quoted 8 lines
drivers/i2c/busses/i2c-mv64xxx.c +0 -1
@@ -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?

Signature · 2 lines
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
author
#9 lore raw

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

Quoted 19 lines · Jean-Francois Moine

drivers/i2c/busses/i2c-mv64xxx.c

Quoted 13 lines · On Thu, 16 May 2013 21:33:09 +0100 Russell King <[email protected]>…

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 <[email protected]>
---
 drivers/i2c/busses/i2c-mv64xxx.c |   46 +++++---------------------------------
 1 files changed, 6 insertions(+), 40 deletions(-)
drivers/i2c/busses/i2c-mv64xxx.c +0 -0
index 0339cd8..19cc9bf 100644

	[snip]
Quoted 2 lines
drivers/i2c/busses/i2c-mv64xxx.c +0 -0
@@ -704,7 +671,6 @@ mv64xxx_i2c_remove(struct platform_device *dev)
Quoted 8 lines

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_remove(struct platform_device *dev) · line 676

Quoted 6 lines
drivers/i2c/busses/i2c-mv64xxx.c +0 -1
 	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.

Quoted 9 lines
(quoted diff) +3 -2
+	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.

#10 lore raw

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

In reply to Russell King - ARM Linux

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 <[email protected]>
---
 drivers/i2c/busses/i2c-mv64xxx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/i2c/busses/i2c-mv64xxx.c +1 -1
index 19cc9bf..0b4da77 100644
@@ -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);
Signature · 1 line
1.7.4.4
#11 lore raw

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

In reply to Russell King - ARM Linux

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 <[email protected]>
---
 drivers/i2c/busses/i2c-mv64xxx.c |   24 ++++++++++--------------
 1 files changed, 10 insertions(+), 14 deletions(-)
drivers/i2c/busses/i2c-mv64xxx.c +10 -14
index 0b4da77..841792f 100644
@@ -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;
 }
Signature · 1 line
1.7.4.4
#12 lore raw

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

In reply to Russell King - ARM Linux

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

Signed-off-by Russell King <[email protected]>
---
 drivers/i2c/busses/i2c-mv64xxx.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)
drivers/i2c/busses/i2c-mv64xxx.c +5 -5
index 841792f..a82ab25 100644
@@ -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,
Signature · 1 line
1.7.4.4
#13 lore raw

PATCH 7/9 I2C: mv64xxx: remove I2C_M_NOSTART code

In reply to Russell King - ARM Linux

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 <[email protected]>
---
 drivers/i2c/busses/i2c-mv64xxx.c |   26 +++++---------------------
 1 files changed, 5 insertions(+), 21 deletions(-)
drivers/i2c/busses/i2c-mv64xxx.c +5 -21
index a82ab25..d160f8c 100644
@@ -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;
Signature · 1 line
1.7.4.4
#14 lore raw

PATCH 7/9 I2C: mv64xxx: remove I2C_M_NOSTART code

Quoted 5 lines · 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.

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>

#15 lore raw

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

In reply to Russell King - ARM Linux

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 <[email protected]>
---
 drivers/i2c/busses/i2c-mv64xxx.c |   52 +++++++++++++++++++-------------------
 1 files changed, 26 insertions(+), 26 deletions(-)
drivers/i2c/busses/i2c-mv64xxx.c +26 -26
index d160f8c..7457ef5 100644
@@ -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;
Signature · 1 line
1.7.4.4
#16 lore raw

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

In reply to Russell King - ARM Linux

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 <[email protected]>
---
 drivers/i2c/busses/i2c-mv64xxx.c |   54 ++++++++++++++++++++++++--------------
 1 files changed, 34 insertions(+), 20 deletions(-)
drivers/i2c/busses/i2c-mv64xxx.c +34 -20
index 7457ef5..a2e4633 100644
@@ -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 = {
Signature · 1 line
1.7.4.4
#17 lore raw

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

Quoted 8 lines
drivers/i2c/busses/i2c-mv64xxx.c +3 -0
@@ -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);
+

...

Quoted 4 lines
drivers/i2c/busses/i2c-mv64xxx.c +0 -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);

drivers/i2c/busses/i2c-mv64xxx.c static int · line 468

Quoted 10 lines
drivers/i2c/busses/i2c-mv64xxx.c +2 -7
-	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...

author
#18 lore raw

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

Quoted 16 lines · Wolfram Sang
Quoted 6 lines

{

drivers/i2c/busses/i2c-mv64xxx.c +3 -0
 	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);
+

...

Quoted 7 lines
drivers/i2c/busses/i2c-mv64xxx.c +1 -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;
Quoted 10 lines

drivers/i2c/busses/i2c-mv64xxx.c static int · line 468

Quoted 7 lines
drivers/i2c/busses/i2c-mv64xxx.c +1 -6
-	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.

#19 lore raw

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

Quoted 28 lines · Russell King - ARM Linux
Quoted 16 lines · Wolfram Sang
Quoted 6 lines

{

drivers/i2c/busses/i2c-mv64xxx.c +3 -0
 	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);
+

...

Quoted 7 lines
drivers/i2c/busses/i2c-mv64xxx.c +1 -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;
Quoted 10 lines

drivers/i2c/busses/i2c-mv64xxx.c static int · line 468

Quoted 7 lines
drivers/i2c/busses/i2c-mv64xxx.c +1 -6
-	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...

Quoted 3 lines

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.

Quoted 7 lines

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

#20 lore raw

cover Fix Marvell mv63xxx I2C driver

In reply to Russell King - ARM Linux
Quoted 9 lines · 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.

Quoted 6 lines

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>