Re: [v2,05/10] i2c-i801: Consolidate calls to i801_check_post

[Benjamin Tissoires]

On May 29 2016 or thereabouts, Corey Minyard wrote:
> From: Corey Minyard <cminyard@xxxxxxxxxx>
> 
> This was almost always called at the end of the transaction.  The
> only time it wasn't called was when a protocol violation occurred,
> and the error reporting was inconsistent there.
> 
> So have the transaction functions return positive status or a
> negative error if they detect an issue.  If a protocol violation
> does occur, always do a kill operation on the bus instead of just
> trying to finish the operation.  If there was a hardware issue,
> the code that was there to terminate the transaction could loop
> forever, and the kill seems more appropriate if something goes
> grossly wrong.
> 
> Signed-off-by: Corey Minyard <cminyard@xxxxxxxxxx>
> ---

I *think* the patch is OK. I have a few comments.

>  drivers/i2c/busses/i2c-i801.c | 51 +++++++++++++++++++++----------------------
>  1 file changed, 25 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 8794e70..56db310 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -317,7 +317,13 @@ static int i801_check_post(struct i801_priv *priv, int status)
>  	 * DEV_ERR.
>  	 */
>  	if (unlikely(status < 0)) {
> -		dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
> +		result = status;
> +		if (status == -EPROTO)
> +			dev_err(&priv->pci_dev->dev,
> +				"Illegal SMBus block read size %d\n",
> +				priv->len);

Not sure this is a good idea to map -EPROTO to "Illegal SMBus block read
size". True, it looks like it can only happens this way, but I would
rather see it mapped to "Protocol Error". Given that Protocol Error is
not very interesting in itself, I'd rather keep the error messages in
the callers only (except for ETIMEOUT where you can output an error
message).

> +		else
> +			dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
>  		/* try to stop the current command */
>  		dev_dbg(&priv->pci_dev->dev, "Terminating the current operation\n");
>  		outb_p(inb_p(SMBHSTCNT(priv)) | SMBHSTCNT_KILL,
> @@ -333,7 +339,7 @@ static int i801_check_post(struct i801_priv *priv, int status)
>  			dev_err(&priv->pci_dev->dev,
>  				"Failed terminating the transaction\n");
>  		outb_p(STATUS_FLAGS, SMBHSTSTS(priv));
> -		return -ETIMEDOUT;
> +		return result;
>  	}
>  
>  	if (status & SMBHSTSTS_FAILED) {
> @@ -414,15 +420,14 @@ static int i801_transaction(struct i801_priv *priv, int xact)
>  				 "Timeout waiting for interrupt!\n");

Here, the code already dev_warn the timeout and later i801_check_post
will dev_err the timeout too. That's one too much IMO :)

>  		}
>  		priv->status = 0;
> -		return i801_check_post(priv, status);
> +		return status;
>  	}
>  
>  	/* the current contents of SMBHSTCNT can be overwritten, since PEC,
>  	 * SMBSCMD are passed in xact */
>  	outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
>  
> -	status = i801_wait_intr(priv);
> -	return i801_check_post(priv, status);
> +	return i801_wait_intr(priv);
>  }
>  
>  static int i801_block_transaction_by_block(struct i801_priv *priv,
> @@ -444,11 +449,11 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
>  
>  	status = i801_transaction(priv, I801_BLOCK_DATA |
>  				  (hwpec ? SMBHSTCNT_PEC_EN : 0));
> -	if (status)
> +	if (status < 0 || status & STATUS_ERROR_FLAGS)
>  		return status;
>  
>  	if (read_write == I2C_SMBUS_READ) {
> -		len = inb_p(SMBHSTDAT0(priv));
> +		len = priv->len = inb_p(SMBHSTDAT0(priv));
>  		if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
>  			return -EPROTO;
>  
> @@ -456,7 +461,7 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
>  		for (i = 0; i < len; i++)
>  			data->block[i + 1] = inb_p(SMBBLKDAT(priv));
>  	}
> -	return 0;
> +	return status;
>  }
>  
>  static void i801_isr_byte_done(struct i801_priv *priv)
> @@ -590,7 +595,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  				 "Timeout waiting for interrupt!\n");
>  		}
>  		priv->status = 0;
> -		return i801_check_post(priv, status);
> +		return status;
>  	}
>  
>  	for (i = 1; i <= len; i++) {
> @@ -604,24 +609,14 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  
>  		status = i801_wait_byte_done(priv);
>  		if (status)
> -			goto exit;
> +			return status;
>  
>  		if (i == 1 && read_write == I2C_SMBUS_READ
>  		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
> -			len = inb_p(SMBHSTDAT0(priv));
> -			if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) {
> -				dev_err(&priv->pci_dev->dev,
> -					"Illegal SMBus block read size %d\n",
> -					len);
> -				/* Recover */
> -				while (inb_p(SMBHSTSTS(priv)) &
> -				       SMBHSTSTS_HOST_BUSY)
> -					outb_p(SMBHSTSTS_BYTE_DONE,
> -					       SMBHSTSTS(priv));
> -				outb_p(SMBHSTSTS_INTR, SMBHSTSTS(priv));
> +			priv->len = inb_p(SMBHSTDAT0(priv));
> +			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX)
>  				return -EPROTO;
> -			}
> -			data->block[0] = len;
> +			data->block[0] = len = priv->len;

Not entirely sure why the priv->len changes are interleaved in this
patch. I might be missing something, but it looks like it should not be
there or in a separate patch.

Cheers,
Benjamin

>  		}
>  
>  		/* Retrieve/store value in SMBBLKDAT */
> @@ -634,9 +629,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  		outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
>  	}
>  
> -	status = i801_wait_intr(priv);
> -exit:
> -	return i801_check_post(priv, status);
> +	return i801_wait_intr(priv);
>  }
>  
>  static int i801_set_block_buffer_mode(struct i801_priv *priv)
> @@ -791,6 +784,12 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  	else
>  		ret = i801_transaction(priv, xact);
>  
> +	/*
> +	 * ret can be status if positive or an error if negative.  Let
> +	 * the post check handle it.
> +	 */
> +	ret = i801_check_post(priv, ret);
> +
>  	if (hostc >= 0)
>  		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc);
>