[PATCH RFC] i2c: octeon: allow receiving of signals.

[PATCH RFC] i2c: octeon: allow receiving of signals.

[Nicholas Mc Guire]

Change wait_event_timeout() to wait_event_interruptible_timeout() allowing
signals to be received while waiting on event (task state set to 
TASK_INTERRUPTIBLE) as the dev_dbg() string indicates that signals
are expected to be possible. While at it fix the type to long (as returned
by schedule_timeout(). 

Signed-off-by: Nicholas Mc Guire <hofrat-Q945KHDl0DbYtjvyW6yDsg@public.gmane.org>
---

>From the dev_dbg string "wait interrupted" it seems that interrupts are
expected to be on - so most likely this should be
wait_event_interruptible_timeout() and not wait_event_timeout() as the
later return >= 0 always and sets the task state to TASK_UNINTERRUPTIBLE.
Someone that knows the details of this driver needs to confirm this though.

Patch was compile tested with cavium_octeon_defconfig
(implies CONFIG_I2C_OCTEON=y)

Patch is against 4.1-rc7 (localversion-next is -next-20150611)

 drivers/i2c/busses/i2c-octeon.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon.c b/drivers/i2c/busses/i2c-octeon.c
index 6e75e01..e25e2b8 100644
--- a/drivers/i2c/busses/i2c-octeon.c
+++ b/drivers/i2c/busses/i2c-octeon.c
@@ -200,19 +200,19 @@ static int octeon_i2c_test_iflg(struct octeon_i2c *i2c)
  */
 static int octeon_i2c_wait(struct octeon_i2c *i2c)
 {
-	int result;
+	long result;
 
 	octeon_i2c_int_enable(i2c);
 
-	result = wait_event_timeout(i2c->queue,
-					octeon_i2c_test_iflg(i2c),
-					i2c->adap.timeout);
+	result = wait_event_interruptible_timeout(i2c->queue,
+						  octeon_i2c_test_iflg(i2c),
+						  i2c->adap.timeout);
 
 	octeon_i2c_int_disable(i2c);
 
 	if (result < 0) {
 		dev_dbg(i2c->dev, "%s: wait interrupted\n", __func__);
-		return result;
+		return -ERESTARTSYS;
 	} else if (result == 0) {
 		dev_dbg(i2c->dev, "%s: timeout\n", __func__);
 		return -ETIMEDOUT;
-- 
1.7.10.4

Re: [PATCH RFC] i2c: octeon: allow receiving of signals.

[Wolfram Sang]

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

On Thu, Jun 11, 2015 at 03:23:52PM +0200, Nicholas Mc Guire wrote:
> Change wait_event_timeout() to wait_event_interruptible_timeout() allowing
> signals to be received while waiting on event (task state set to 
> TASK_INTERRUPTIBLE) as the dev_dbg() string indicates that signals
> are expected to be possible. While at it fix the type to long (as returned
> by schedule_timeout(). 
> 
> Signed-off-by: Nicholas Mc Guire <hofrat-Q945KHDl0DbYtjvyW6yDsg@public.gmane.org>
> ---
> 
> From the dev_dbg string "wait interrupted" it seems that interrupts are
> expected to be on - so most likely this should be
> wait_event_interruptible_timeout() and not wait_event_timeout() as the
> later return >= 0 always and sets the task state to TASK_UNINTERRUPTIBLE.
> Someone that knows the details of this driver needs to confirm this though.

Use 'git log drivers/i2c/busses/i2c-octeon.c' and you will find commit
2637e5fd232d421b680757944f613d4b1a36ae26 which disabled the signals.
For further digging, you could do
'git log --grep="interruptible" drivers/i2c/busses' to find that signals
cause a lot of problems with i2c transactions.

Summary: The code still handling signal code simply needs to go.

Thanks,

   Wolfram


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

Re: [PATCH RFC] i2c: octeon: allow receiving of signals.

[Nicholas Mc Guire]

On Thu, 11 Jun 2015, Wolfram Sang wrote:

> On Thu, Jun 11, 2015 at 03:23:52PM +0200, Nicholas Mc Guire wrote:
> > Change wait_event_timeout() to wait_event_interruptible_timeout() allowing
> > signals to be received while waiting on event (task state set to 
> > TASK_INTERRUPTIBLE) as the dev_dbg() string indicates that signals
> > are expected to be possible. While at it fix the type to long (as returned
> > by schedule_timeout(). 
> > 
> > Signed-off-by: Nicholas Mc Guire <hofrat-Q945KHDl0DbYtjvyW6yDsg@public.gmane.org>
> > ---
> > 
> > From the dev_dbg string "wait interrupted" it seems that interrupts are
> > expected to be on - so most likely this should be
> > wait_event_interruptible_timeout() and not wait_event_timeout() as the
> > later return >= 0 always and sets the task state to TASK_UNINTERRUPTIBLE.
> > Someone that knows the details of this driver needs to confirm this though.
> 
> Use 'git log drivers/i2c/busses/i2c-octeon.c' and you will find commit
> 2637e5fd232d421b680757944f613d4b1a36ae26 which disabled the signals.
> For further digging, you could do
> 'git log --grep="interruptible" drivers/i2c/busses' to find that signals
> cause a lot of problems with i2c transactions.
> 
> Summary: The code still handling signal code simply needs to go.
>
thanks - the output of 'git log --grep="interruptible" drivers/i2c/busses' 
really make it very clear - sorry for the noise - I had only been looking at
the code of this driver to figure out how to resolve the obvious API
inconsistency but did not check the broader scope.

will fix it up and resend.

thx!
hofrat