[1/5] i2c-i801: hwpec and check_pre cleanups

From: Corey Minyard <cminyard@mvista.com>

Add an "xact_extra" variable and use it to carry the PEC enable and
interrupt flags .

Also move i801_check_pre() to i801_access()  That consolidates it to
one call, and there's no point in doing all the work if the hardware
isn't ready.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/i2c/busses/i2c-i801.c | 51 ++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 25 deletions(-)

Hi Corey,

On Tue, 19 Jan 2016 11:09:33 -0600, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Add an "xact_extra" variable and use it to carry the PEC enable and
> interrupt flags .

Which is a good thing, because...? This needs a justification. At this
point I just see an implementation change, with no reason to prefer one
implementation to the other. In such cases I prefer to leave things as
they are, to avoid introducing bugs.

Stray space in description above.

Is it just me or PEC handling is quite broken in this driver,
regardless of your patch? If I read the code correctly:
* hwpec isn't used for non-block transactions.
* i801_block_transaction_byte_by_byte() takes hwpec as a parameter but
  never uses it.
Which basically means that PEC is only really implemented for one-shot
block transactions, right?

> Also move i801_check_pre() to i801_access()  That consolidates it to
> one call, and there's no point in doing all the work if the hardware
> isn't ready.

Why do both changes in a single patch? They don't seem to be related.
These changes would be easier to review and integrate as separate
patches.

> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 51 ++++++++++++++++++++++---------------------
>  1 file changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index f62d697..62cf1e5 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -230,6 +230,7 @@ struct i801_priv {
>  	/* isr processing */
>  	wait_queue_head_t waitq;
>  	u8 status;
> +	u8 xact_extra; /* Used to set INTREN if irqs enabled, and HWPEC */
>  
>  	/* Command state used by isr for byte-by-byte block transactions */
>  	u8 cmd;
> @@ -401,13 +402,13 @@ static int i801_transaction(struct i801_priv *priv, int xact)
>  	int result;
>  	const struct i2c_adapter *adap = &priv->adapter;
>  
> -	result = i801_check_pre(priv);
> -	if (result < 0)
> -		return result;
> +	/*
> +	 * the current contents of SMBHSTCNT can be overwritten, since PEC,
> +	 * SMBSCMD are passed in xact

This is no longer true, PEC is in priv->xact_extra after your changes,
not xact.

> +	 */
> +	outb_p(xact | priv->xact_extra | SMBHSTCNT_START,  SMBHSTCNT(priv));
>  
>  	if (priv->features & FEATURE_IRQ) {
> -		outb_p(xact | SMBHSTCNT_INTREN | SMBHSTCNT_START,
> -		       SMBHSTCNT(priv));
>  		result = wait_event_timeout(priv->waitq,
>  					    (status = priv->status),
>  					    adap->timeout);
> @@ -420,17 +421,13 @@ static int i801_transaction(struct i801_priv *priv, int xact)
>  		return i801_check_post(priv, 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);
>  }
>  
>  static int i801_block_transaction_by_block(struct i801_priv *priv,
>  					   union i2c_smbus_data *data,
> -					   char read_write, int hwpec)
> +					   char read_write)
>  {
>  	int i, len;
>  	int status;
> @@ -445,8 +442,7 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
>  			outb_p(data->block[i+1], SMBBLKDAT(priv));
>  	}
>  
> -	status = i801_transaction(priv, I801_BLOCK_DATA |
> -				  (hwpec ? SMBHSTCNT_PEC_EN : 0));
> +	status = i801_transaction(priv, I801_BLOCK_DATA);
>  	if (status)
>  		return status;
>  
> @@ -553,8 +549,7 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
>   */
>  static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  					       union i2c_smbus_data *data,
> -					       char read_write, int command,
> -					       int hwpec)
> +					       char read_write, int command)
>  {
>  	int i, len;
>  	int smbcmd;
> @@ -562,10 +557,6 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  	int result;
>  	const struct i2c_adapter *adap = &priv->adapter;
>  
> -	result = i801_check_pre(priv);
> -	if (result < 0)
> -		return result;
> -
>  	len = data->block[0];
>  
>  	if (read_write == I2C_SMBUS_WRITE) {
> @@ -583,7 +574,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  		priv->is_read = (read_write == I2C_SMBUS_READ);
>  		if (len == 1 && priv->is_read)
>  			smbcmd |= SMBHSTCNT_LAST_BYTE;
> -		priv->cmd = smbcmd | SMBHSTCNT_INTREN;
> +		priv->cmd = smbcmd | priv->xact_extra;
>  		priv->len = len;
>  		priv->count = 0;
>  		priv->data = &data->block[1];
> @@ -691,13 +682,15 @@ static int i801_block_transaction(struct i801_priv *priv,
>  	   doesn't mention this limitation. */
>  	if ((priv->features & FEATURE_BLOCK_BUFFER)
>  	 && command != I2C_SMBUS_I2C_BLOCK_DATA
> -	 && i801_set_block_buffer_mode(priv) == 0)
> +	 && i801_set_block_buffer_mode(priv) == 0) {
> +		if (hwpec)
> +			priv->xact_extra |= SMBHSTCNT_PEC_EN;
>  		result = i801_block_transaction_by_block(priv, data,
> -							 read_write, hwpec);
> -	else
> +							 read_write);
> +	} else
>  		result = i801_block_transaction_byte_by_byte(priv, data,
>  							     read_write,
> -							     command, hwpec);
> +							     command);
>  
>  	if (command == I2C_SMBUS_I2C_BLOCK_DATA
>  	 && read_write == I2C_SMBUS_WRITE) {
> @@ -716,6 +709,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  	int block = 0;
>  	int ret, xact = 0;
>  	struct i801_priv *priv = i2c_get_adapdata(adap);
> +	int result;

You already have ret here which serves the same purpose, no need to
introduce another variable.

>  
>  	hwpec = (priv->features & FEATURE_SMBUS_PEC) && (flags & I2C_CLIENT_PEC)
>  		&& size != I2C_SMBUS_QUICK
> @@ -776,11 +770,17 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  		return -EOPNOTSUPP;
>  	}
>  
> -	if (hwpec)	/* enable/disable hardware PEC */
> +	result = i801_check_pre(priv);
> +	if (result < 0)
> +		return result;

At first I was worried that this move is breaking the symmetry with
i2c_check_post(), but now I see you do the same cleanup for
i2c_check_post() later in the series. Good.

So I have no objection to moving the call to i801_check_pre() earlier,
in the common path. But shouldn't it be even earlier? As you said,
there's no point in doing all the work if the controller is not ready.
Shouldn't i801_check_pre() be called first thing in i801_access()?

> +
> +	if (hwpec) {	/* enable/disable hardware PEC */
>  		outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_CRC, SMBAUXCTL(priv));
> -	else
> +	} else {
>  		outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
>  		       SMBAUXCTL(priv));
> +		priv->xact_extra &= ~SMBHSTCNT_PEC_EN;
> +	}
>  
>  	if (block)
>  		ret = i801_block_transaction(priv, data, read_write, size,
> @@ -1381,6 +1381,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	}
>  
>  	if (priv->features & FEATURE_IRQ) {
> +		priv->xact_extra |= SMBHSTCNT_INTREN;

A few lines below, we may disable FEATURE_IRQ if we failed to allocate
the irq. If this happens, your change leaves the driver in an
inconsistent state where priv->xact_extra contains SMBHSTCNT_INTREN but
interrupts should not be used.

>  		init_waitqueue_head(&priv->waitq);
>  
>  		err = devm_request_irq(&dev->dev, dev->irq, i801_isr,

Sorry, I accidentally sent the previous reply while I wasn't done with
the review yet.

On Tue, 19 Jan 2016 11:09:33 -0600, minyard@acm.org wrote:
> @@ -691,13 +682,15 @@ static int i801_block_transaction(struct i801_priv *priv,
>  	   doesn't mention this limitation. */
>  	if ((priv->features & FEATURE_BLOCK_BUFFER)
>  	 && command != I2C_SMBUS_I2C_BLOCK_DATA
> -	 && i801_set_block_buffer_mode(priv) == 0)
> +	 && i801_set_block_buffer_mode(priv) == 0) {
> +		if (hwpec)
> +			priv->xact_extra |= SMBHSTCNT_PEC_EN;
>  		result = i801_block_transaction_by_block(priv, data,
> -							 read_write, hwpec);
> -	else
> +							 read_write);
> +	} else
>  		result = i801_block_transaction_byte_by_byte(priv, data,
>  							     read_write,
> -							     command, hwpec);
> +							     command);
>  
> (...)
> @@ -776,11 +770,17 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  		return -EOPNOTSUPP;
>  	}
>  
> -	if (hwpec)	/* enable/disable hardware PEC */
> +	result = i801_check_pre(priv);
> +	if (result < 0)
> +		return result;
> +
> +	if (hwpec) {	/* enable/disable hardware PEC */
>  		outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_CRC, SMBAUXCTL(priv));
> -	else
> +	} else {
>  		outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
>  		       SMBAUXCTL(priv));
> +		priv->xact_extra &= ~SMBHSTCNT_PEC_EN;
> +	}

I'm confused by this asymmetry. You clear the PEC flag here, but you
set it in i801_block_transaction() above. Originally the flag was set
in i801_block_transaction_by_block() (and didn't have to be cleared, as
it was temporary.)

With your implementation, PEC may or may not be enabled if a driver
asks for a non-block transaction with PEC. If this is the first
transaction then it will not be enabled (bug already existed before
your patch.) But if the previous transaction was a block transaction
with PEC then the flag will still be present, so PEC will still be
enabled. The previous implementation was wrong but at least it was
consistently so.

This makes me believe we should rather fix the bugs first, and then
look into cleaning up this part of the code.

If you start storing transaction-dependent information in struct
i801_priv, you must make sure that nothing will leak from one
transaction to the next. I still have to review the rest of your patch
series, but I don't think it makes sense to carry the PEC flag that way
if the rest of the transaction information is still passed as function
parameters.