[i2c] [PATCH] i2c: improve lost-arbitration handling in i2c-algo-pcf

[Jean Delvare]

Hi Eric,

When sending a patch, please set the attachment type to something more
friendly than "application/octet-stream".

On Thu, 19 Jul 2007 22:26:08 -0700, Eric Brower wrote:
> Attached is a patch that improves multi-master support for PCF8584.
> The current implementation of multi-master support (mine) in
> i2c-algo-pcf does not properly handle a collision detected during wait_for_bb()
> and thus causes an error that is only recoverable by reloading the
> relevant device drivers.  I'd appreciate your consideration of this
> patch.
> 
> The only affected driver is i2c-envctrl.c-- currently out-of-kernel,
> in development; you
> can find it here if you are interested:
> 
>    http://www.david-web.co.uk/index.php?p=envctrl
> 
> This change has been tested by a few individuals (on Sun Microsystems
> E250 servers) over the course of several months, and rectifies
> occasional errors seen without the patch applied.
> 
> I'll be happy to provide further clarification if you are interested.
> Please cc me on responses-- I am not a member of this list.

Note that I can't test your patch, as I don't have supported hardware.

> Commit text:
> 
> Improve lost-arbitration handling of PCF8584
> Signed-off-by: Eric Brower <ebrower at gmail.com>

Review:

> diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
> index ecb2c2d..2cc0302 100644
> --- a/drivers/i2c/algos/i2c-algo-pcf.c
> +++ b/drivers/i2c/algos/i2c-algo-pcf.c
> @@ -78,6 +78,31 @@ static void i2c_stop(struct i2c_algo_pcf
>  	set_pcf(adap, 1, I2C_PCF_STOP);
>  }
>  
> +static inline void handle_lab(struct i2c_algo_pcf_data *adap, const int *status)
> +{
> +	DEB2(printk(KERN_INFO 
> +		"i2c-algo-pcf.o: lost arbitration (CSR 0x%02x)\n",
> +		 *status));
> +	/* Cleanup from LAB-- reset and enable ESO.
> +	 * This resets the PCF8584; since we've lost the bus, no
> +	 * further attempts should be made by callers to clean up 
> +	 * (no i2c_stop() etc.)
> +	 */
> +	set_pcf(adap, 1, I2C_PCF_PIN);
> +	set_pcf(adap, 1, I2C_PCF_ESO);
> +	/* We pause for a time period sufficient for any running 
> +	 * I2C transaction to complete-- the arbitration logic won't 
> +	 * work properly until the next START is seen.
> +	 * It is assumed the bus driver or client has set a proper value.
> +	 */
> +	if (adap->lab_mdelay) {
> +		mdelay(adap->lab_mdelay);
> +	}

Out of curiosity: on what basis will the value of lab_mdelay be chosen
by the driver author? This seems to be pretty arbitrary, as you don't
know what kind of transactions the other master is doing. The
underlying question being: is it worth having a per-adapter parameter
for this, as opposed to a hard-coded value?

Also, it seems to me that you could sleep here. If you sleep a bit
longer than expected, it shouldn't be a problem, right?

> +	DEB2(printk(KERN_INFO 
> +		"i2c-algo-pcf.o: reset LAB condition (CSR 0x%02x)\n", 
> +		get_pcf(adap,1)));

Coding style: space after comma.

> +}

This function is too big to be inlined, especially given that it should
rarely be called, so speeding it up isn't very interesting.

> +
>  static int wait_for_bb(struct i2c_algo_pcf_data *adap) {
>  
>  	int timeout = DEF_TIMEOUT;
> @@ -86,6 +111,10 @@ static int wait_for_bb(struct i2c_algo_p
>  	status = get_pcf(adap, 1);
>  #ifndef STUB_I2C
>  	while (timeout-- && !(status & I2C_PCF_BB)) {
> +		if (status & I2C_PCF_LAB) {
> +			handle_lab(adap, &status);
> +			return -EINTR;
> +		}

How can this happen? We check for busy bus before we start
transmitting, and arbitration can't be lost until we at least try to
transmit.

>  		udelay(100); /* wait for 100 us */
>  		status = get_pcf(adap, 1);
>  	}

> @@ -109,23 +139,7 @@ static int wait_for_pin(struct i2c_algo_
>  		*status = get_pcf(adap, 1);
>  	}
>  	if (*status & I2C_PCF_LAB) {
> -		DEB2(printk(KERN_INFO 
> -			"i2c-algo-pcf.o: lost arbitration (CSR 0x%02x)\n",
> -			 *status));
> -		/* Cleanup from LAB-- reset and enable ESO.
> -		 * This resets the PCF8584; since we've lost the bus, no
> -		 * further attempts should be made by callers to clean up 
> -		 * (no i2c_stop() etc.)
> -		 */
> -		set_pcf(adap, 1, I2C_PCF_PIN);
> -		set_pcf(adap, 1, I2C_PCF_ESO);
> -		/* TODO: we should pause for a time period sufficient for any
> -		 * running I2C transaction to complete-- the arbitration
> -		 * logic won't work properly until the next START is seen.
> -		 */
> -		DEB2(printk(KERN_INFO 
> -			"i2c-algo-pcf.o: reset LAB condition (CSR 0x%02x)\n", 
> -			get_pcf(adap,1)));
> +		handle_lab(adap, status);
>  		return(-EINTR);
>  	}
>  #endif
> @@ -218,9 +232,8 @@ static inline int try_address(struct i2c
>  				break;	/* success! */
>  			}
>  		}
> -		if (wfp == -EINTR) {
> +		else if (wfp == -EINTR) {
>  			/* arbitration lost */
> -			udelay(adap->udelay);
>  			return -EINTR;
>  		}
>  		i2c_stop(adap);
> @@ -378,6 +393,10 @@ static int pcf_xfer(struct i2c_adapter *
>  	/* Check for bus busy */
>  	timeout = wait_for_bb(adap);
>  	if (timeout) {
> +		if (timeout == -EINTR) {
> +			/* arbitration lost */
> +			return -EINTR;
> +		}
>  		DEB2(printk(KERN_ERR "i2c-algo-pcf.o: "
>  		            "Timeout waiting for BB in pcf_xfer\n");)
>  		return -EIO;
> diff --git a/include/linux/i2c-algo-pcf.h b/include/linux/i2c-algo-pcf.h
> index 994eb86..d11c141 100644
> --- a/include/linux/i2c-algo-pcf.h
> +++ b/include/linux/i2c-algo-pcf.h
> @@ -36,6 +36,12 @@ struct i2c_algo_pcf_data {
>  	/* local settings */
>  	int udelay;
>  	int timeout;
> +
> +	/* Multi-master lost arbitration back-off delay (msecs)
> +	 * This should be set by the bus adapter or knowledgable client 
> +	 * if bus is multi-mastered, else zero
> +	 */
> +	unsigned long lab_mdelay;
>  };
>  
>  int i2c_pcf_add_bus(struct i2c_adapter *);

As a side question, shouldn't i2c-algo-pcf retry, rather than fail, on
arbitration lost?

-- 
Jean Delvare