Re: [PATCH 7/8] drivers/i2c/busses/i2c-amd8111.c: Fix unsigned return type

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Julia,

On Thu, 30 Sep 2010 22:27:02 +0200 (CEST), Julia Lawall wrote:
> The functions the functions amd_ec_wait_write and amd_ec_wait_read have an
> unsigned return type, but return a negative constant to indicate an error
> condition.
> 
> A sematic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @exists@
> identifier f;
> constant C;
> @@
> 
>  unsigned f(...)
>  { <+...
> *  return -C;
>  ...+> }
> // </smpl>
> 
> Fixing amd_ec_wait_write and amd_ec_wait_read leads to the need to adjust
> the return type of the functions amd_ec_write and amd_ec_read, which are
> the only functions that call amd_ec_wait_write and amd_ec_wait_read.
> amd_ec_write and amd_ec_read, in turn, are only called from within the
> function amd8111_access, which already returns a signed typed value.  Each
> of the calls to amd_ec_write and amd_ec_read are updated using the
> following semantic patch:
> 
> // <smpl>
> @@
> @@
> 
> + status = amd_ec_write
> - amd_ec_write
>   (...);
> + if (status) return status;
> 
> @@
> @@
> 
> + status = amd_ec_read
> - amd_ec_read
>   (...);
> + if (status) return status;
> // </smpl>
> 
> The patch also adds the declaration of the status variable.
> 
> Signed-off-by: Julia Lawall <julia@xxxxxxx>
> 
> ---
> Only compile tested.

Can't test it either, no hardware.

> 
>  drivers/i2c/busses/i2c-amd8111.c    |  152 ++++++++++++++------
>  1 files changed, 110 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-amd8111.c b/drivers/i2c/busses/i2c-amd8111.c
> index af1e5e2..e12c584 100644
> --- a/drivers/i2c/busses/i2c-amd8111.c
> +++ b/drivers/i2c/busses/i2c-amd8111.c
> @@ -69,7 +69,7 @@ static struct pci_driver amd8111_driver;
>   * ACPI 2.0 chapter 13 access of registers of the EC
>   */
>  
> -static unsigned int amd_ec_wait_write(struct amd_smbus *smbus)
> +static int amd_ec_wait_write(struct amd_smbus *smbus)
>  {
>  	int timeout = 500;
>  
> @@ -85,7 +85,7 @@ static unsigned int amd_ec_wait_write(struct amd_smbus *smbus)
>  	return 0;
>  }
>  
> -static unsigned int amd_ec_wait_read(struct amd_smbus *smbus)
> +static int amd_ec_wait_read(struct amd_smbus *smbus)
>  {
>  	int timeout = 500;
>  
> @@ -101,7 +101,7 @@ static unsigned int amd_ec_wait_read(struct amd_smbus *smbus)
>  	return 0;
>  }
>  
> -static unsigned int amd_ec_read(struct amd_smbus *smbus, unsigned char address,
> +static int amd_ec_read(struct amd_smbus *smbus, unsigned char address,
>  		unsigned char *data)
>  {
>  	int status;
> @@ -124,7 +124,7 @@ static unsigned int amd_ec_read(struct amd_smbus *smbus, unsigned char address,
>  	return 0;
>  }
>  
> -static unsigned int amd_ec_write(struct amd_smbus *smbus, unsigned char address,
> +static int amd_ec_write(struct amd_smbus *smbus, unsigned char address,
>  		unsigned char data)
>  {
>  	int status;
> @@ -196,7 +196,7 @@ static s32 amd8111_access(struct i2c_adapter * adap, u16 addr,
>  {
>  	struct amd_smbus *smbus = adap->algo_data;
>  	unsigned char protocol, len, pec, temp[2];
> -	int i;
> +	int i, status;
>  
>  	protocol = (read_write == I2C_SMBUS_READ) ? AMD_SMB_PRTCL_READ
>  						  : AMD_SMB_PRTCL_WRITE;
> @@ -209,38 +209,62 @@ static s32 amd8111_access(struct i2c_adapter * adap, u16 addr,
>  			break;
>  
>  		case I2C_SMBUS_BYTE:
> -			if (read_write == I2C_SMBUS_WRITE)
> -				amd_ec_write(smbus, AMD_SMB_CMD, command);
> +			if (read_write == I2C_SMBUS_WRITE) {
> +				status = amd_ec_write(smbus, AMD_SMB_CMD,
> +							command);
> +				if (status)
> +					return status;
> +			}
>  			protocol |= AMD_SMB_PRTCL_BYTE;
>  			break;
>  
>  		case I2C_SMBUS_BYTE_DATA:
> -			amd_ec_write(smbus, AMD_SMB_CMD, command);
> -			if (read_write == I2C_SMBUS_WRITE)
> -				amd_ec_write(smbus, AMD_SMB_DATA, data->byte);
> +			status = amd_ec_write(smbus, AMD_SMB_CMD, command);
> +			if (status)
> +				return status;
> +			if (read_write == I2C_SMBUS_WRITE) {
> +				status = amd_ec_write(smbus, AMD_SMB_DATA,
> +							data->byte);
> +				if (status)
> +					return status;
> +			}
>  			protocol |= AMD_SMB_PRTCL_BYTE_DATA;
>  			break;
>  
>  		case I2C_SMBUS_WORD_DATA:
> -			amd_ec_write(smbus, AMD_SMB_CMD, command);
> +			status = amd_ec_write(smbus, AMD_SMB_CMD, command);
> +			if (status)
> +				return status;
>  			if (read_write == I2C_SMBUS_WRITE) {
> -				amd_ec_write(smbus, AMD_SMB_DATA,
> +				status = amd_ec_write(smbus, AMD_SMB_DATA,
>  					     data->word & 0xff);
> -				amd_ec_write(smbus, AMD_SMB_DATA + 1,
> +				if (status)
> +					return status;
> +				status = amd_ec_write(smbus, AMD_SMB_DATA + 1,
>  					     data->word >> 8);
> +				if (status)
> +					return status;
>  			}
>  			protocol |= AMD_SMB_PRTCL_WORD_DATA | pec;
>  			break;
>  
>  		case I2C_SMBUS_BLOCK_DATA:
> -			amd_ec_write(smbus, AMD_SMB_CMD, command);
> +			status = amd_ec_write(smbus, AMD_SMB_CMD, command);
> +			if (status)
> +				return status;
>  			if (read_write == I2C_SMBUS_WRITE) {
>  				len = min_t(u8, data->block[0],
>  					    I2C_SMBUS_BLOCK_MAX);
> -				amd_ec_write(smbus, AMD_SMB_BCNT, len);
> -				for (i = 0; i < len; i++)
> -					amd_ec_write(smbus, AMD_SMB_DATA + i,
> -						     data->block[i + 1]);
> +				status = amd_ec_write(smbus, AMD_SMB_BCNT, len);
> +				if (status)
> +					return status;
> +				for (i = 0; i < len; i++) {
> +					status =
> +					  amd_ec_write(smbus, AMD_SMB_DATA + i,
> +						       data->block[i + 1]);
> +					if (status)
> +						return status;
> +				}
>  			}
>  			protocol |= AMD_SMB_PRTCL_BLOCK_DATA | pec;
>  			break;
> @@ -248,19 +272,35 @@ static s32 amd8111_access(struct i2c_adapter * adap, u16 addr,
>  		case I2C_SMBUS_I2C_BLOCK_DATA:
>  			len = min_t(u8, data->block[0],
>  				    I2C_SMBUS_BLOCK_MAX);
> -			amd_ec_write(smbus, AMD_SMB_CMD, command);
> -			amd_ec_write(smbus, AMD_SMB_BCNT, len);
> +			status = amd_ec_write(smbus, AMD_SMB_CMD, command);
> +			if (status)
> +				return status;
> +			status = amd_ec_write(smbus, AMD_SMB_BCNT, len);
> +			if (status)
> +				return status;
>  			if (read_write == I2C_SMBUS_WRITE)
> -				for (i = 0; i < len; i++)
> -					amd_ec_write(smbus, AMD_SMB_DATA + i,
> -						     data->block[i + 1]);
> +				for (i = 0; i < len; i++) {
> +					status =
> +					  amd_ec_write(smbus, AMD_SMB_DATA + i,
> +						       data->block[i + 1]);
> +					if (status)
> +						return status;
> +				}
>  			protocol |= AMD_SMB_PRTCL_I2C_BLOCK_DATA;
>  			break;
>  
>  		case I2C_SMBUS_PROC_CALL:
> -			amd_ec_write(smbus, AMD_SMB_CMD, command);
> -			amd_ec_write(smbus, AMD_SMB_DATA, data->word & 0xff);
> -			amd_ec_write(smbus, AMD_SMB_DATA + 1, data->word >> 8);
> +			status = amd_ec_write(smbus, AMD_SMB_CMD, command);
> +			if (status)
> +				return status;
> +			status = amd_ec_write(smbus, AMD_SMB_DATA,
> +						data->word & 0xff);
> +			if (status)
> +				return status;
> +			status = amd_ec_write(smbus, AMD_SMB_DATA + 1,
> +						data->word >> 8);
> +			if (status)
> +				return status;
>  			protocol = AMD_SMB_PRTCL_PROC_CALL | pec;
>  			read_write = I2C_SMBUS_READ;
>  			break;
> @@ -268,11 +308,18 @@ static s32 amd8111_access(struct i2c_adapter * adap, u16 addr,
>  		case I2C_SMBUS_BLOCK_PROC_CALL:
>  			len = min_t(u8, data->block[0],
>  				    I2C_SMBUS_BLOCK_MAX - 1);
> -			amd_ec_write(smbus, AMD_SMB_CMD, command);
> -			amd_ec_write(smbus, AMD_SMB_BCNT, len);
> -			for (i = 0; i < len; i++)
> -				amd_ec_write(smbus, AMD_SMB_DATA + i,
> +			status = amd_ec_write(smbus, AMD_SMB_CMD, command);
> +			if (status)
> +				return status;
> +			status = amd_ec_write(smbus, AMD_SMB_BCNT, len);
> +			if (status)
> +				return status;
> +			for (i = 0; i < len; i++) {
> +				status = amd_ec_write(smbus, AMD_SMB_DATA + i,
>  					     data->block[i + 1]);
> +				if (status)
> +					return status;
> +			}
>  			protocol = AMD_SMB_PRTCL_BLOCK_PROC_CALL | pec;
>  			read_write = I2C_SMBUS_READ;
>  			break;
> @@ -282,24 +329,34 @@ static s32 amd8111_access(struct i2c_adapter * adap, u16 addr,
>  			return -EOPNOTSUPP;
>  	}
>  
> -	amd_ec_write(smbus, AMD_SMB_ADDR, addr << 1);
> -	amd_ec_write(smbus, AMD_SMB_PRTCL, protocol);
> +	status = amd_ec_write(smbus, AMD_SMB_ADDR, addr << 1);
> +	if (status)
> +		return status;
> +	status = amd_ec_write(smbus, AMD_SMB_PRTCL, protocol);
> +	if (status)
> +		return status;
>  
>  	/* FIXME this discards status from ec_read(); so temp[0] will
>  	 * hold stack garbage ... the rest of this routine will act
>  	 * nonsensically.  Ignored ec_write() status might explain
>  	 * some such failures...
>  	 */

This comment can go away now :)

> -	amd_ec_read(smbus, AMD_SMB_STS, temp + 0);
> +	status = amd_ec_read(smbus, AMD_SMB_STS, temp + 0);
> +	if (status)
> +		return status;
>  
>  	if (~temp[0] & AMD_SMB_STS_DONE) {
>  		udelay(500);
> -		amd_ec_read(smbus, AMD_SMB_STS, temp + 0);
> +		status = amd_ec_read(smbus, AMD_SMB_STS, temp + 0);
> +		if (status)
> +			return status;
>  	}
>  
>  	if (~temp[0] & AMD_SMB_STS_DONE) {
>  		msleep(1);
> -		amd_ec_read(smbus, AMD_SMB_STS, temp + 0);
> +		status = amd_ec_read(smbus, AMD_SMB_STS, temp + 0);
> +		if (status)
> +			return status;
>  	}
>  
>  	if ((~temp[0] & AMD_SMB_STS_DONE) || (temp[0] & AMD_SMB_STS_STATUS))
> @@ -311,24 +368,35 @@ static s32 amd8111_access(struct i2c_adapter * adap, u16 addr,
>  	switch (size) {
>  		case I2C_SMBUS_BYTE:
>  		case I2C_SMBUS_BYTE_DATA:
> -			amd_ec_read(smbus, AMD_SMB_DATA, &data->byte);
> +			status = amd_ec_read(smbus, AMD_SMB_DATA, &data->byte);
> +			if (status)
> +				return status;
>  			break;
>  
>  		case I2C_SMBUS_WORD_DATA:
>  		case I2C_SMBUS_PROC_CALL:
> -			amd_ec_read(smbus, AMD_SMB_DATA, temp + 0);
> -			amd_ec_read(smbus, AMD_SMB_DATA + 1, temp + 1);
> +			status = amd_ec_read(smbus, AMD_SMB_DATA, temp + 0);
> +			if (status)
> +				return status;
> +			status = amd_ec_read(smbus, AMD_SMB_DATA + 1, temp + 1);
> +			if (status)
> +				return status;
>  			data->word = (temp[1] << 8) | temp[0];
>  			break;
>  
>  		case I2C_SMBUS_BLOCK_DATA:
>  		case I2C_SMBUS_BLOCK_PROC_CALL:
> -			amd_ec_read(smbus, AMD_SMB_BCNT, &len);
> +			status = amd_ec_read(smbus, AMD_SMB_BCNT, &len);
> +			if (status)
> +				return status;
>  			len = min_t(u8, len, I2C_SMBUS_BLOCK_MAX);
>  		case I2C_SMBUS_I2C_BLOCK_DATA:
> -			for (i = 0; i < len; i++)
> -				amd_ec_read(smbus, AMD_SMB_DATA + i,
> -					    data->block + i + 1);
> +			for (i = 0; i < len; i++) {
> +				status = amd_ec_read(smbus, AMD_SMB_DATA + i,
> +							data->block + i + 1);
> +				if (status)
> +					return status;
> +			}
>  			data->block[0] = len;
>  			break;
>  	}

Patch looks good, applied (for 2.6.37, as it doesn't fix any reported
issue), thanks.

-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux