Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion

[Jan Glauber <jan.glauber@caviumnetworks.com>]

On Mon, Nov 07, 2016 at 08:09:21PM +0000, Paul Burton wrote:
> Commit 1bb1ff3e7c74 ("i2c: octeon: Improve performance if interrupt is
> early") modified octeon_i2c_wait() & octeon_i2c_hlc_wait() to attempt to
> check for a valid bit being clear & if not to sleep for a while then try
> again before waiting on a waitqueue which may time out. However it does
> so by sleeping within a function called as the condition provided to
> wait_event_timeout() which seems to cause strange behaviour, with the
> system hanging during boot with the condition being checked constantly &
> the timeout not seeming to have any effect.
> 
> Fix this by instead checking for the valid bit being clear in the
> octeon_i2c(_hlc)_wait() functions & sleeping there if that condition is
> not met, then calling the wait_event_timeout with a condition that does
> not sleep.
> 
> Tested on a Rhino Labs UTM-8 with Octeon CN7130.

This patch breaks ipmi on ThunderX (CN88xx). We also have not seen the
problems you mention, although I must admit that I don't like the
complicated nested waits.

--Jan

> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Cc: David Daney <david.daney@cavium.com>
> Cc: Jan Glauber <jglauber@cavium.com>
> Cc: Peter Swain <pswain@cavium.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: linux-i2c@vger.kernel.org
> ---
> 
>  drivers/i2c/busses/i2c-octeon-core.c | 58 ++++++++++--------------------------
>  1 file changed, 15 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
> index 419b54b..2e270a1 100644
> --- a/drivers/i2c/busses/i2c-octeon-core.c
> +++ b/drivers/i2c/busses/i2c-octeon-core.c
> @@ -36,24 +36,6 @@ static bool octeon_i2c_test_iflg(struct octeon_i2c *i2c)
>  	return (octeon_i2c_ctl_read(i2c) & TWSI_CTL_IFLG);
>  }
>  
> -static bool octeon_i2c_test_ready(struct octeon_i2c *i2c, bool *first)
> -{
> -	if (octeon_i2c_test_iflg(i2c))
> -		return true;
> -
> -	if (*first) {
> -		*first = false;
> -		return false;
> -	}
> -
> -	/*
> -	 * IRQ has signaled an event but IFLG hasn't changed.
> -	 * Sleep and retry once.
> -	 */
> -	usleep_range(I2C_OCTEON_EVENT_WAIT, 2 * I2C_OCTEON_EVENT_WAIT);
> -	return octeon_i2c_test_iflg(i2c);
> -}
> -
>  /**
>   * octeon_i2c_wait - wait for the IFLG to be set
>   * @i2c: The struct octeon_i2c
> @@ -80,8 +62,13 @@ static int octeon_i2c_wait(struct octeon_i2c *i2c)
>  	}
>  
>  	i2c->int_enable(i2c);
> -	time_left = wait_event_timeout(i2c->queue, octeon_i2c_test_ready(i2c, &first),
> -				       i2c->adap.timeout);
> +	time_left = i2c->adap.timeout;
> +	if (!octeon_i2c_test_iflg(i2c)) {
> +		usleep_range(I2C_OCTEON_EVENT_WAIT, 2 * I2C_OCTEON_EVENT_WAIT);
> +		time_left = wait_event_timeout(i2c->queue,
> +					       octeon_i2c_test_iflg(i2c),
> +					       time_left);
> +	}
>  	i2c->int_disable(i2c);
>  
>  	if (i2c->broken_irq_check && !time_left &&
> @@ -99,26 +86,8 @@ static int octeon_i2c_wait(struct octeon_i2c *i2c)
>  
>  static bool octeon_i2c_hlc_test_valid(struct octeon_i2c *i2c)
>  {
> -	return (__raw_readq(i2c->twsi_base + SW_TWSI(i2c)) & SW_TWSI_V) == 0;
> -}
> -
> -static bool octeon_i2c_hlc_test_ready(struct octeon_i2c *i2c, bool *first)
> -{
>  	/* check if valid bit is cleared */
> -	if (octeon_i2c_hlc_test_valid(i2c))
> -		return true;
> -
> -	if (*first) {
> -		*first = false;
> -		return false;
> -	}
> -
> -	/*
> -	 * IRQ has signaled an event but valid bit isn't cleared.
> -	 * Sleep and retry once.
> -	 */
> -	usleep_range(I2C_OCTEON_EVENT_WAIT, 2 * I2C_OCTEON_EVENT_WAIT);
> -	return octeon_i2c_hlc_test_valid(i2c);
> +	return (__raw_readq(i2c->twsi_base + SW_TWSI(i2c)) & SW_TWSI_V) == 0;
>  }
>  
>  static void octeon_i2c_hlc_int_clear(struct octeon_i2c *i2c)
> @@ -176,7 +145,6 @@ static void octeon_i2c_hlc_disable(struct octeon_i2c *i2c)
>   */
>  static int octeon_i2c_hlc_wait(struct octeon_i2c *i2c)
>  {
> -	bool first = true;
>  	int time_left;
>  
>  	/*
> @@ -194,9 +162,13 @@ static int octeon_i2c_hlc_wait(struct octeon_i2c *i2c)
>  	}
>  
>  	i2c->hlc_int_enable(i2c);
> -	time_left = wait_event_timeout(i2c->queue,
> -				       octeon_i2c_hlc_test_ready(i2c, &first),
> -				       i2c->adap.timeout);
> +	time_left = i2c->adap.timeout;
> +	if (!octeon_i2c_hlc_test_valid(i2c)) {
> +		usleep_range(I2C_OCTEON_EVENT_WAIT, 2 * I2C_OCTEON_EVENT_WAIT);
> +		time_left = wait_event_timeout(i2c->queue,
> +					       octeon_i2c_hlc_test_valid(i2c),
> +					       time_left);
> +	}
>  	i2c->hlc_int_disable(i2c);
>  	if (!time_left)
>  		octeon_i2c_hlc_int_clear(i2c);
> -- 
> 2.10.2