| Message ID | E1Ud4pH-0004JQ-Jq@rmk-PC.arm.linux.org.uk |
|---|---|
| State | Accepted |
| Headers | show |
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.
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
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
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
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);
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(-)