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

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

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

In each case, the function has an unsigned return type, but returns a
negative constant to indicate an error condition.  The result of calling
the function is always stored in a variable of type (signed) int, and thus
unsigned can be dropped from the return type.

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>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 drivers/i2c/busses/i2c-amd8111.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-amd8111.c b/drivers/i2c/busses/i2c-amd8111.c
index af1e5e2..02a3726 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;
 

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

[Jean Delvare]

Hi Julia,

On Wed, 8 Sep 2010 07:58:45 +0200 (CEST), Julia Lawall wrote:
> From: Julia Lawall <julia@diku.dk>
> 
> In each case, the function has an unsigned return type, but returns a
> negative constant to indicate an error condition.  The result of calling
> the function is always stored in a variable of type (signed) int, and thus
> unsigned can be dropped from the return type.
> 
> 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>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>
> 
> ---
>  drivers/i2c/busses/i2c-amd8111.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-amd8111.c b/drivers/i2c/busses/i2c-amd8111.c
> index af1e5e2..02a3726 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;
>  
> 

Thanks for reporting. Unfortunately the fix above is incomplete.
amd_ec_wait_write() and amd_ec_wait_read() are called by amd_ec_read()
and amd_ec_write(), which will now attempt to return negative error
codes as unsigned ints. So amd_ec_read() and amd_ec_write() must have
their return type fixed too.

Note that the driver code never checks for errors returned by
amd_ec_read() or amd_ec_write() anyway, so the fix alone is a no-op. A
real fix would have to also update the caller sites to properly deal
with errors if then happen.

-- 
Jean Delvare

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

[Julia Lawall]

OK, thanks. I will look at the code again.

julia


On Wed, 8 Sep 2010, Jean Delvare wrote:

> Hi Julia,
> 
> On Wed, 8 Sep 2010 07:58:45 +0200 (CEST), Julia Lawall wrote:
> > From: Julia Lawall <julia@diku.dk>
> > 
> > In each case, the function has an unsigned return type, but returns a
> > negative constant to indicate an error condition.  The result of calling
> > the function is always stored in a variable of type (signed) int, and thus
> > unsigned can be dropped from the return type.
> > 
> > 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>
> > 
> > Signed-off-by: Julia Lawall <julia@diku.dk>
> > 
> > ---
> >  drivers/i2c/busses/i2c-amd8111.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-amd8111.c b/drivers/i2c/busses/i2c-amd8111.c
> > index af1e5e2..02a3726 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;
> >  
> > 
> 
> Thanks for reporting. Unfortunately the fix above is incomplete.
> amd_ec_wait_write() and amd_ec_wait_read() are called by amd_ec_read()
> and amd_ec_write(), which will now attempt to return negative error
> codes as unsigned ints. So amd_ec_read() and amd_ec_write() must have
> their return type fixed too.
> 
> Note that the driver code never checks for errors returned by
> amd_ec_read() or amd_ec_write() anyway, so the fix alone is a no-op. A
> real fix would have to also update the caller sites to properly deal
> with errors if then happen.
> 
> -- 
> Jean Delvare
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Jean Delvare]

Hi Julia,

On Wed, 8 Sep 2010 11:48:17 +0200 (CEST), Julia Lawall wrote:
> OK, thanks. I will look at the code again.

Any update on this patch?

> On Wed, 8 Sep 2010, Jean Delvare wrote:
> 
> > Hi Julia,
> > 
> > On Wed, 8 Sep 2010 07:58:45 +0200 (CEST), Julia Lawall wrote:
> > > From: Julia Lawall <julia@diku.dk>
> > > 
> > > In each case, the function has an unsigned return type, but returns a
> > > negative constant to indicate an error condition.  The result of calling
> > > the function is always stored in a variable of type (signed) int, and thus
> > > unsigned can be dropped from the return type.
> > > 
> > > 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>
> > > 
> > > Signed-off-by: Julia Lawall <julia@diku.dk>
> > > 
> > > ---
> > >  drivers/i2c/busses/i2c-amd8111.c |    4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-amd8111.c b/drivers/i2c/busses/i2c-amd8111.c
> > > index af1e5e2..02a3726 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;
> > >  
> > > 
> > 
> > Thanks for reporting. Unfortunately the fix above is incomplete.
> > amd_ec_wait_write() and amd_ec_wait_read() are called by amd_ec_read()
> > and amd_ec_write(), which will now attempt to return negative error
> > codes as unsigned ints. So amd_ec_read() and amd_ec_write() must have
> > their return type fixed too.
> > 
> > Note that the driver code never checks for errors returned by
> > amd_ec_read() or amd_ec_write() anyway, so the fix alone is a no-op. A
> > real fix would have to also update the caller sites to properly deal
> > with errors if then happen.
> > 
> > -- 
> > Jean Delvare
> > --
> > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Jean Delvare

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

[Julia Lawall]

On Wed, 29 Sep 2010, Jean Delvare wrote:

> Hi Julia,
> 
> On Wed, 8 Sep 2010 11:48:17 +0200 (CEST), Julia Lawall wrote:
> > OK, thanks. I will look at the code again.
> 
> Any update on this patch?

No, I'm sorry.  I haven't forgotten it, but I haven't looked at it 
either.  I should be able to do so in a few days.

julia

> > On Wed, 8 Sep 2010, Jean Delvare wrote:
> > 
> > > Hi Julia,
> > > 
> > > On Wed, 8 Sep 2010 07:58:45 +0200 (CEST), Julia Lawall wrote:
> > > > From: Julia Lawall <julia@diku.dk>
> > > > 
> > > > In each case, the function has an unsigned return type, but returns a
> > > > negative constant to indicate an error condition.  The result of calling
> > > > the function is always stored in a variable of type (signed) int, and thus
> > > > unsigned can be dropped from the return type.
> > > > 
> > > > 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>
> > > > 
> > > > Signed-off-by: Julia Lawall <julia@diku.dk>
> > > > 
> > > > ---
> > > >  drivers/i2c/busses/i2c-amd8111.c |    4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/i2c/busses/i2c-amd8111.c b/drivers/i2c/busses/i2c-amd8111.c
> > > > index af1e5e2..02a3726 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;
> > > >  
> > > > 
> > > 
> > > Thanks for reporting. Unfortunately the fix above is incomplete.
> > > amd_ec_wait_write() and amd_ec_wait_read() are called by amd_ec_read()
> > > and amd_ec_write(), which will now attempt to return negative error
> > > codes as unsigned ints. So amd_ec_read() and amd_ec_write() must have
> > > their return type fixed too.
> > > 
> > > Note that the driver code never checks for errors returned by
> > > amd_ec_read() or amd_ec_write() anyway, so the fix alone is a no-op. A
> > > real fix would have to also update the caller sites to properly deal
> > > with errors if then happen.
> > > 
> > > -- 
> > > Jean Delvare
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> -- 
> Jean Delvare
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Julia Lawall]

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@diku.dk>

---
Only compile tested.

 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...
 	 */
-	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;
 	}

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

[Jean Delvare]

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@diku.dk>
> 
> ---
> 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