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

login
register
mail settings
Submitter Russell King
Date May 16, 2013, 8:30 p.m.
Message ID <E1Ud4pH-0004JQ-Jq@rmk-PC.arm.linux.org.uk>
Download mbox | patch
Permalink /patch/244414/
State Accepted
Headers show

Comments

Russell King - May 16, 2013, 8:30 p.m.
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(-)
Jean-Francois Moine - May 17, 2013, 6:37 a.m.
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.
Russell King - ARM Linux - May 17, 2013, 8:31 a.m.
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.
--
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 - May 17, 2013, 12:17 p.m.
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...
--
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
Mark A. Greer - May 17, 2013, 5:06 p.m.
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.
--
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

Patch

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