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@xxxxxxxxxx>
> Cc: David Daney <david.daney@xxxxxxxxxx>
> Cc: Jan Glauber <jglauber@xxxxxxxxxx>
> Cc: Peter Swain <pswain@xxxxxxxxxx>
> Cc: Wolfram Sang <wsa@xxxxxxxxxxxxx>
> Cc: linux-i2c@xxxxxxxxxxxxxxx
> ---
>
> 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
