[PATCH] i2c-mpc: do not allow interruptions when waiting for I2C to complete

[PATCH] i2c-mpc: do not allow interruptions when waiting for I2C to complete

[Timur Tabi]

The i2c_wait() function is using wait_event_interruptible_timeout() to wait for
the I2C controller to signal that it has completed an I2C bus operation.  If
the process that causes the I2C operation terminated abruptly, the wait will
be interrupted, returning an error.  It is better to let the I2C operation
finished before the process exits.

It is safe to use wait_event_timeout() instead, because the timeout will allow
the process to exit if the I2C bus hangs.  It's also better to allow the
I2C operation to finish, because unacknowledged I2C operations can cause the
I2C bus to hang.

Signed-off-by: Timur Tabi <timur@freescale.com>
---

A similar change should probably be done to i2c-cpm.c, and maybe all other
I2C drivers.  Not many use wait_event_interruptible_timeout().

 drivers/i2c/busses/i2c-mpc.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index a9a45fc..c0ace48 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -70,7 +70,7 @@ static irqreturn_t mpc_i2c_isr(int irq, void *dev_id)
 		/* Read again to allow register to stabilise */
 		i2c->interrupt = readb(i2c->base + MPC_I2C_SR);
 		writeb(0, i2c->base + MPC_I2C_SR);
-		wake_up_interruptible(&i2c->queue);
+		wake_up(&i2c->queue);
 	}
 	return IRQ_HANDLED;
 }
@@ -115,13 +115,10 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
 		writeb(0, i2c->base + MPC_I2C_SR);
 	} else {
 		/* Interrupt mode */
-		result = wait_event_interruptible_timeout(i2c->queue,
+		result = wait_event_timeout(i2c->queue,
 			(i2c->interrupt & CSR_MIF), timeout * HZ);
 
-		if (unlikely(result < 0)) {
-			pr_debug("I2C: wait interrupted\n");
-			writeccr(i2c, 0);
-		} else if (unlikely(!(i2c->interrupt & CSR_MIF))) {
+		if (unlikely(!(i2c->interrupt & CSR_MIF))) {
 			pr_debug("I2C: wait timeout\n");
 			writeccr(i2c, 0);
 			result = -ETIMEDOUT;
-- 
1.5.5

Re: [PATCH] i2c-mpc: do not allow interruptions when waiting for I2C to complete

[Timur Tabi]

On Fri, Feb 6, 2009 at 8:00 AM, Timur Tabi <timur@freescale.com> wrote:
> The i2c_wait() function is using wait_event_interruptible_timeout() to wait for
> the I2C controller to signal that it has completed an I2C bus operation.  If
> the process that causes the I2C operation terminated abruptly, the wait will
> be interrupted, returning an error.  It is better to let the I2C operation
> finished before the process exits.
>
> It is safe to use wait_event_timeout() instead, because the timeout will allow
> the process to exit if the I2C bus hangs.  It's also better to allow the
> I2C operation to finish, because unacknowledged I2C operations can cause the
> I2C bus to hang.
>
> Signed-off-by: Timur Tabi <timur@freescale.com>

Jean,

Could you pick up this patch for 2.6.30?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] i2c-mpc: do not allow interruptions when waiting for I2C to complete

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]

On Fri, Feb 06, 2009 at 08:00:37AM -0600, Timur Tabi wrote:
> The i2c_wait() function is using wait_event_interruptible_timeout() to wait for
> the I2C controller to signal that it has completed an I2C bus operation.  If
> the process that causes the I2C operation terminated abruptly, the wait will
> be interrupted, returning an error.  It is better to let the I2C operation
> finished before the process exits.
> 
> It is safe to use wait_event_timeout() instead, because the timeout will allow
> the process to exit if the I2C bus hangs.  It's also better to allow the
> I2C operation to finish, because unacknowledged I2C operations can cause the
> I2C bus to hang.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>

Acked-by: Wolfram Sang <w.sang@pengutronix.de>

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] i2c-mpc: do not allow interruptions when waiting for I2C to complete

[Jean Delvare]

On Tue, 10 Feb 2009 08:59:57 -0600, Timur Tabi wrote:
> On Fri, Feb 6, 2009 at 8:00 AM, Timur Tabi <timur@freescale.com> wrote:
> > The i2c_wait() function is using wait_event_interruptible_timeout() to wait for
> > the I2C controller to signal that it has completed an I2C bus operation.  If
> > the process that causes the I2C operation terminated abruptly, the wait will
> > be interrupted, returning an error.  It is better to let the I2C operation
> > finished before the process exits.
> >
> > It is safe to use wait_event_timeout() instead, because the timeout will allow
> > the process to exit if the I2C bus hangs.  It's also better to allow the
> > I2C operation to finish, because unacknowledged I2C operations can cause the
> > I2C bus to hang.
> >
> > Signed-off-by: Timur Tabi <timur@freescale.com>
> 
> Jean,
> 
> Could you pick up this patch for 2.6.30?

No, that's something for either Ben Dooks (Cc'd) or the powerpc tree.

-- 
Jean Delvare

Re: [PATCH] i2c-mpc: do not allow interruptions when waiting for I2C to complete

[Timur Tabi]

Jean Delvare wrote:

> No, that's something for either Ben Dooks (Cc'd) or the powerpc tree.

This patch has nothing to do with ARM, so Kumar will pick it up, if you
ACK it.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] i2c-mpc: do not allow interruptions when waiting for I2C to complete

[Jean Delvare]

On Tue, 10 Feb 2009 10:01:54 -0600, Timur Tabi wrote:
> Jean Delvare wrote:
>=20
> > No, that's something for either Ben Dooks (Cc'd) or the powerpc tree.
>=20
> This patch has nothing to do with ARM, so Kumar will pick it up, if you
> ACK it.

Why are you mentioning ARM?

=46rom MAINTAINERS:

I2C SUBSYSTEM
P:      Jean Delvare (PC drivers, core)
M:      khali@linux-fr.org
P:      Ben Dooks (embedded platforms)
M:      ben-linux@fluff.org

To the best of my knowledge i2c-mpc falls into the "embedded platforms"
category and is thus under Ben's responsibility.

Thanks,
--=20
Jean Delvare

Re: [PATCH] i2c-mpc: do not allow interruptions when waiting for I2C to complete

[Timur Tabi]

Jean Delvare wrote:

> To the best of my knowledge i2c-mpc falls into the "embedded platforms"
> category and is thus under Ben's responsibility.

I don't see his Signed-off-by on any patch to i2c-mpc.c for the past
three years (at least).  I do see yours, however.  You have directly
applied similar patches in the recent past, so of course I will assume
that you would apply this one.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] i2c-mpc: do not allow interruptions when waiting for I2C to complete

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 477 bytes --]

> I don't see his Signed-off-by on any patch to i2c-mpc.c for the past
> three years (at least).  I do see yours, however.  You have directly

Ben is the I2C-"embedded"-maintainer. This job was created less than
a year ago, this is why you don't see sob from him regarding this
driver.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] i2c-mpc: do not allow interruptions when waiting for I2C to complete

[Ben Dooks]

On Tue, Feb 10, 2009 at 10:21:04AM -0600, Timur Tabi wrote:
> Jean Delvare wrote:
> 
> > To the best of my knowledge i2c-mpc falls into the "embedded platforms"
> > category and is thus under Ben's responsibility.
> 
> I don't see his Signed-off-by on any patch to i2c-mpc.c for the past
> three years (at least).  I do see yours, however.  You have directly
> applied similar patches in the recent past, so of course I will assume
> that you would apply this one.

My appointment as an embedded maintainer is relatively recent.

I do try and review patches, and sometimes these patches get sent
to another maintainer.

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [PATCH] i2c-mpc: do not allow interruptions when waiting for I2C to complete

[Ben Dooks]

On Fri, Feb 06, 2009 at 08:00:37AM -0600, Timur Tabi wrote:
> The i2c_wait() function is using wait_event_interruptible_timeout() to wait for
> the I2C controller to signal that it has completed an I2C bus operation.  If
> the process that causes the I2C operation terminated abruptly, the wait will
> be interrupted, returning an error.  It is better to let the I2C operation
> finished before the process exits.
> 
> It is safe to use wait_event_timeout() instead, because the timeout will allow
> the process to exit if the I2C bus hangs.  It's also better to allow the
> I2C operation to finish, because unacknowledged I2C operations can cause the
> I2C bus to hang.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
Acked-by: Ben Dooks <ben-linux@fluff.org>
> ---
> 
> A similar change should probably be done to i2c-cpm.c, and maybe all other
> I2C drivers.  Not many use wait_event_interruptible_timeout().
> 
>  drivers/i2c/busses/i2c-mpc.c |    9 +++------
>  1 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index a9a45fc..c0ace48 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -70,7 +70,7 @@ static irqreturn_t mpc_i2c_isr(int irq, void *dev_id)
>  		/* Read again to allow register to stabilise */
>  		i2c->interrupt = readb(i2c->base + MPC_I2C_SR);
>  		writeb(0, i2c->base + MPC_I2C_SR);
> -		wake_up_interruptible(&i2c->queue);
> +		wake_up(&i2c->queue);
>  	}
>  	return IRQ_HANDLED;
>  }
> @@ -115,13 +115,10 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
>  		writeb(0, i2c->base + MPC_I2C_SR);
>  	} else {
>  		/* Interrupt mode */
> -		result = wait_event_interruptible_timeout(i2c->queue,
> +		result = wait_event_timeout(i2c->queue,
>  			(i2c->interrupt & CSR_MIF), timeout * HZ);
>  
> -		if (unlikely(result < 0)) {
> -			pr_debug("I2C: wait interrupted\n");
> -			writeccr(i2c, 0);
> -		} else if (unlikely(!(i2c->interrupt & CSR_MIF))) {
> +		if (unlikely(!(i2c->interrupt & CSR_MIF))) {
>  			pr_debug("I2C: wait timeout\n");
>  			writeccr(i2c, 0);
>  			result = -ETIMEDOUT;
> -- 
> 1.5.5
> 
> --
> 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

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [PATCH] i2c-mpc: do not allow interruptions when waiting for I2C to complete

[Kumar Gala]

On Feb 6, 2009, at 8:00 AM, Timur Tabi wrote:

> The i2c_wait() function is using wait_event_interruptible_timeout()  
> to wait for
> the I2C controller to signal that it has completed an I2C bus  
> operation.  If
> the process that causes the I2C operation terminated abruptly, the  
> wait will
> be interrupted, returning an error.  It is better to let the I2C  
> operation
> finished before the process exits.
>
> It is safe to use wait_event_timeout() instead, because the timeout  
> will allow
> the process to exit if the I2C bus hangs.  It's also better to allow  
> the
> I2C operation to finish, because unacknowledged I2C operations can  
> cause the
> I2C bus to hang.
>
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
>
> A similar change should probably be done to i2c-cpm.c, and maybe all  
> other
> I2C drivers.  Not many use wait_event_interruptible_timeout().
>
> drivers/i2c/busses/i2c-mpc.c |    9 +++------
> 1 files changed, 3 insertions(+), 6 deletions(-)

applied to next

- k