[PATCH] em28xx: fix compiler warnings

[PATCH] em28xx: fix compiler warnings

[Hans Verkuil]

Fix three compiler warnings:

drivers/media/usb/em28xx/em28xx-input.c: In function ‘em28xx_i2c_ir_handle_key’:
drivers/media/usb/em28xx/em28xx-input.c:318:1: warning: the frame size of 1096 bytes is larger than 1024 bytes [-Wframe-larger-than=]
 }
 ^
  CC [M]  drivers/media/usb/em28xx/em28xx-dvb.o
drivers/media/usb/em28xx/em28xx-camera.c: In function ‘em28xx_probe_sensor_micron’:
drivers/media/usb/em28xx/em28xx-camera.c:199:1: warning: the frame size of 1096 bytes is larger than 1024 bytes [-Wframe-larger-than=]
 }
 ^
drivers/media/usb/em28xx/em28xx-camera.c: In function ‘em28xx_probe_sensor_omnivision’:
drivers/media/usb/em28xx/em28xx-camera.c:304:1: warning: the frame size of 1088 bytes is larger than 1024 bytes [-Wframe-larger-than=]
 }
 ^

Note: there is no way the code in em28xx_i2c_ir_handle_key() is correct: it's
using an almost completely uninitialized i2c_client struct with random flags,
dev and name fields. Can't this turned into a proper i2c_client struct in
struct em28xx? At least with this patch it's no longer random data.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
index 6d2ea9a..c8490ba 100644
--- a/drivers/media/usb/em28xx/em28xx-camera.c
+++ b/drivers/media/usb/em28xx/em28xx-camera.c
@@ -110,40 +110,40 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
 	__be16 id_be;
 	u16 id;
 
-	struct i2c_client client = dev->i2c_client[dev->def_i2c_bus];
+	dev->tmp_i2c_client = dev->i2c_client[dev->def_i2c_bus];
 
 	dev->em28xx_sensor = EM28XX_NOSENSOR;
 	for (i = 0; micron_sensor_addrs[i] != I2C_CLIENT_END; i++) {
-		client.addr = micron_sensor_addrs[i];
+		dev->tmp_i2c_client.addr = micron_sensor_addrs[i];
 		/* NOTE: i2c_smbus_read_word_data() doesn't work with BE data */
 		/* Read chip ID from register 0x00 */
 		reg = 0x00;
-		ret = i2c_master_send(&client, &reg, 1);
+		ret = i2c_master_send(&dev->tmp_i2c_client, &reg, 1);
 		if (ret < 0) {
 			if (ret != -ENXIO)
 				em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
-					      client.addr << 1, ret);
+					      dev->tmp_i2c_client.addr << 1, ret);
 			continue;
 		}
-		ret = i2c_master_recv(&client, (u8 *)&id_be, 2);
+		ret = i2c_master_recv(&dev->tmp_i2c_client, (u8 *)&id_be, 2);
 		if (ret < 0) {
 			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
-				      client.addr << 1, ret);
+				      dev->tmp_i2c_client.addr << 1, ret);
 			continue;
 		}
 		id = be16_to_cpu(id_be);
 		/* Read chip ID from register 0xff */
 		reg = 0xff;
-		ret = i2c_master_send(&client, &reg, 1);
+		ret = i2c_master_send(&dev->tmp_i2c_client, &reg, 1);
 		if (ret < 0) {
 			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
-				      client.addr << 1, ret);
+				      dev->tmp_i2c_client.addr << 1, ret);
 			continue;
 		}
-		ret = i2c_master_recv(&client, (u8 *)&id_be, 2);
+		ret = i2c_master_recv(&dev->tmp_i2c_client, (u8 *)&id_be, 2);
 		if (ret < 0) {
 			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
-				      client.addr << 1, ret);
+				      dev->tmp_i2c_client.addr << 1, ret);
 			continue;
 		}
 		/* Validate chip ID to be sure we have a Micron device */
@@ -191,7 +191,7 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
 		else
 			em28xx_info("sensor %s detected\n", name);
 
-		dev->i2c_client[dev->def_i2c_bus].addr = client.addr;
+		dev->i2c_client[dev->def_i2c_bus].addr = dev->tmp_i2c_client.addr;
 		return 0;
 	}
 
@@ -207,28 +207,29 @@ static int em28xx_probe_sensor_omnivision(struct em28xx *dev)
 	char *name;
 	u8 reg;
 	u16 id;
-	struct i2c_client client = dev->i2c_client[dev->def_i2c_bus];
+
+	dev->tmp_i2c_client = dev->i2c_client[dev->def_i2c_bus];
 
 	dev->em28xx_sensor = EM28XX_NOSENSOR;
 	/* NOTE: these devices have the register auto incrementation disabled
 	 * by default, so we have to use single byte reads !              */
 	for (i = 0; omnivision_sensor_addrs[i] != I2C_CLIENT_END; i++) {
-		client.addr = omnivision_sensor_addrs[i];
+		dev->tmp_i2c_client.addr = omnivision_sensor_addrs[i];
 		/* Read manufacturer ID from registers 0x1c-0x1d (BE) */
 		reg = 0x1c;
-		ret = i2c_smbus_read_byte_data(&client, reg);
+		ret = i2c_smbus_read_byte_data(&dev->tmp_i2c_client, reg);
 		if (ret < 0) {
 			if (ret != -ENXIO)
 				em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
-					      client.addr << 1, ret);
+					      dev->tmp_i2c_client.addr << 1, ret);
 			continue;
 		}
 		id = ret << 8;
 		reg = 0x1d;
-		ret = i2c_smbus_read_byte_data(&client, reg);
+		ret = i2c_smbus_read_byte_data(&dev->tmp_i2c_client, reg);
 		if (ret < 0) {
 			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
-				      client.addr << 1, ret);
+				      dev->tmp_i2c_client.addr << 1, ret);
 			continue;
 		}
 		id += ret;
@@ -237,18 +238,18 @@ static int em28xx_probe_sensor_omnivision(struct em28xx *dev)
 			continue;
 		/* Read product ID from registers 0x0a-0x0b (BE) */
 		reg = 0x0a;
-		ret = i2c_smbus_read_byte_data(&client, reg);
+		ret = i2c_smbus_read_byte_data(&dev->tmp_i2c_client, reg);
 		if (ret < 0) {
 			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
-				      client.addr << 1, ret);
+				      dev->tmp_i2c_client.addr << 1, ret);
 			continue;
 		}
 		id = ret << 8;
 		reg = 0x0b;
-		ret = i2c_smbus_read_byte_data(&client, reg);
+		ret = i2c_smbus_read_byte_data(&dev->tmp_i2c_client, reg);
 		if (ret < 0) {
 			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
-				      client.addr << 1, ret);
+				      dev->tmp_i2c_client.addr << 1, ret);
 			continue;
 		}
 		id += ret;
@@ -296,7 +297,7 @@ static int em28xx_probe_sensor_omnivision(struct em28xx *dev)
 		else
 			em28xx_info("sensor %s detected\n", name);
 
-		dev->i2c_client[dev->def_i2c_bus].addr = client.addr;
+		dev->i2c_client[dev->def_i2c_bus].addr = dev->tmp_i2c_client.addr;
 		return 0;
 	}
 
diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
index ed843bd..07069b6 100644
--- a/drivers/media/usb/em28xx/em28xx-input.c
+++ b/drivers/media/usb/em28xx/em28xx-input.c
@@ -298,12 +298,11 @@ static int em28xx_i2c_ir_handle_key(struct em28xx_IR *ir)
 	static u32 scancode;
 	enum rc_type protocol;
 	int rc;
-	struct i2c_client client;
 
-	client.adapter = &ir->dev->i2c_adap[dev->def_i2c_bus];
-	client.addr = ir->i2c_dev_addr;
+	dev->tmp_i2c_client.adapter = &ir->dev->i2c_adap[dev->def_i2c_bus];
+	dev->tmp_i2c_client.addr = ir->i2c_dev_addr;
 
-	rc = ir->get_key_i2c(&client, &protocol, &scancode);
+	rc = ir->get_key_i2c(&dev->tmp_i2c_client, &protocol, &scancode);
 	if (rc < 0) {
 		dprintk("ir->get_key_i2c() failed: %d\n", rc);
 		return rc;
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 84ef8ef..437ca08 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -630,6 +630,7 @@ struct em28xx {
 	struct i2c_adapter i2c_adap[NUM_I2C_BUSES];
 	struct i2c_client i2c_client[NUM_I2C_BUSES];
 	struct em28xx_i2c_bus i2c_bus[NUM_I2C_BUSES];
+	struct i2c_client tmp_i2c_client;
 
 	unsigned char eeprom_addrwidth_16bit:1;
 	unsigned def_i2c_bus;	/* Default I2C bus */

Re: [PATCH] em28xx: fix compiler warnings

[Shuah Khan]

On Tue, Aug 5, 2014 at 1:00 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> Fix three compiler warnings:
>
> drivers/media/usb/em28xx/em28xx-input.c: In function ‘em28xx_i2c_ir_handle_key’:
> drivers/media/usb/em28xx/em28xx-input.c:318:1: warning: the frame size of 1096 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>  }
>  ^
>   CC [M]  drivers/media/usb/em28xx/em28xx-dvb.o
> drivers/media/usb/em28xx/em28xx-camera.c: In function ‘em28xx_probe_sensor_micron’:
> drivers/media/usb/em28xx/em28xx-camera.c:199:1: warning: the frame size of 1096 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>  }
>  ^
> drivers/media/usb/em28xx/em28xx-camera.c: In function ‘em28xx_probe_sensor_omnivision’:
> drivers/media/usb/em28xx/em28xx-camera.c:304:1: warning: the frame size of 1088 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>  }
>  ^
>
> Note: there is no way the code in em28xx_i2c_ir_handle_key() is correct: it's
> using an almost completely uninitialized i2c_client struct with random flags,
> dev and name fields. Can't this turned into a proper i2c_client struct in
> struct em28xx? At least with this patch it's no longer random data.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>
> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c

> diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
> index ed843bd..07069b6 100644
> --- a/drivers/media/usb/em28xx/em28xx-input.c
> +++ b/drivers/media/usb/em28xx/em28xx-input.c
> @@ -298,12 +298,11 @@ static int em28xx_i2c_ir_handle_key(struct em28xx_IR *ir)
>         static u32 scancode;
>         enum rc_type protocol;
>         int rc;
> -       struct i2c_client client;
>
> -       client.adapter = &ir->dev->i2c_adap[dev->def_i2c_bus];
> -       client.addr = ir->i2c_dev_addr;
> +       dev->tmp_i2c_client.adapter = &ir->dev->i2c_adap[dev->def_i2c_bus];
> +       dev->tmp_i2c_client.addr = ir->i2c_dev_addr;
>
> -       rc = ir->get_key_i2c(&client, &protocol, &scancode);
> +       rc = ir->get_key_i2c(&dev->tmp_i2c_client, &protocol, &scancode);
>         if (rc < 0) {
>                 dprintk("ir->get_key_i2c() failed: %d\n", rc);
>                 return rc;
> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> index 84ef8ef..437ca08 100644
> --- a/drivers/media/usb/em28xx/em28xx.h
> +++ b/drivers/media/usb/em28xx/em28xx.h
> @@ -630,6 +630,7 @@ struct em28xx {
>         struct i2c_adapter i2c_adap[NUM_I2C_BUSES];
>         struct i2c_client i2c_client[NUM_I2C_BUSES];
>         struct em28xx_i2c_bus i2c_bus[NUM_I2C_BUSES];
> +       struct i2c_client tmp_i2c_client;

Hans,

Is it necessary to add this temp variable to the structure? It is
always assigned
whenever it is used in this patch. It might make sense to make it
local variable.
Somehow seeing a tmp field in the structure doesn't sound right and
also since it
maintains state being in the structure, it could be used with incorrect data.

-- Shuah

Re: [PATCH] em28xx: fix compiler warnings

[Hans Verkuil]

On 08/05/2014 03:57 PM, Shuah Khan wrote:
> On Tue, Aug 5, 2014 at 1:00 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> Fix three compiler warnings:
>>
>> drivers/media/usb/em28xx/em28xx-input.c: In function ‘em28xx_i2c_ir_handle_key’:
>> drivers/media/usb/em28xx/em28xx-input.c:318:1: warning: the frame size of 1096 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>>  }
>>  ^
>>   CC [M]  drivers/media/usb/em28xx/em28xx-dvb.o
>> drivers/media/usb/em28xx/em28xx-camera.c: In function ‘em28xx_probe_sensor_micron’:
>> drivers/media/usb/em28xx/em28xx-camera.c:199:1: warning: the frame size of 1096 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>>  }
>>  ^
>> drivers/media/usb/em28xx/em28xx-camera.c: In function ‘em28xx_probe_sensor_omnivision’:
>> drivers/media/usb/em28xx/em28xx-camera.c:304:1: warning: the frame size of 1088 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>>  }
>>  ^
>>
>> Note: there is no way the code in em28xx_i2c_ir_handle_key() is correct: it's
>> using an almost completely uninitialized i2c_client struct with random flags,
>> dev and name fields. Can't this turned into a proper i2c_client struct in
>> struct em28xx? At least with this patch it's no longer random data.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
> 
>> diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
>> index ed843bd..07069b6 100644
>> --- a/drivers/media/usb/em28xx/em28xx-input.c
>> +++ b/drivers/media/usb/em28xx/em28xx-input.c
>> @@ -298,12 +298,11 @@ static int em28xx_i2c_ir_handle_key(struct em28xx_IR *ir)
>>         static u32 scancode;
>>         enum rc_type protocol;
>>         int rc;
>> -       struct i2c_client client;
>>
>> -       client.adapter = &ir->dev->i2c_adap[dev->def_i2c_bus];
>> -       client.addr = ir->i2c_dev_addr;
>> +       dev->tmp_i2c_client.adapter = &ir->dev->i2c_adap[dev->def_i2c_bus];
>> +       dev->tmp_i2c_client.addr = ir->i2c_dev_addr;
>>
>> -       rc = ir->get_key_i2c(&client, &protocol, &scancode);
>> +       rc = ir->get_key_i2c(&dev->tmp_i2c_client, &protocol, &scancode);
>>         if (rc < 0) {
>>                 dprintk("ir->get_key_i2c() failed: %d\n", rc);
>>                 return rc;
>> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
>> index 84ef8ef..437ca08 100644
>> --- a/drivers/media/usb/em28xx/em28xx.h
>> +++ b/drivers/media/usb/em28xx/em28xx.h
>> @@ -630,6 +630,7 @@ struct em28xx {
>>         struct i2c_adapter i2c_adap[NUM_I2C_BUSES];
>>         struct i2c_client i2c_client[NUM_I2C_BUSES];
>>         struct em28xx_i2c_bus i2c_bus[NUM_I2C_BUSES];
>> +       struct i2c_client tmp_i2c_client;
> 
> Hans,
> 
> Is it necessary to add this temp variable to the structure? It is
> always assigned
> whenever it is used in this patch. It might make sense to make it
> local variable.

I'm not sure what you mean. It *was* a local variable, that was the problem.
There are two option: one is to add it to the main struct, then other is to
allocate and free it inside the function. In general I dislike that since it
adds aan extra check (did we really get the memory?) and you have to make sure
you will free the memory. And that's besides the overhead of having to allocate
memory. Originally I named tmp_i2c_client 'probe_i2c_client', but then I saw
that the ir code needs it as well. If the ir code is fixed so it has its own
i2c client, then the name can revert to probe_i2c_client.

Regards,

	Hans

> Somehow seeing a tmp field in the structure doesn't sound right and
> also since it
> maintains state being in the structure, it could be used with incorrect data.
> 
> -- Shuah
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] em28xx: fix compiler warnings

[Shuah Khan]

On Tue, Aug 5, 2014 at 8:18 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 08/05/2014 03:57 PM, Shuah Khan wrote:
> I'm not sure what you mean. It *was* a local variable, that was the problem.
> There are two option: one is to add it to the main struct, then other is to
> allocate and free it inside the function. In general I dislike that since it
> adds aan extra check (did we really get the memory?) and you have to make sure
> you will free the memory. And that's besides the overhead of having to allocate
> memory. Originally I named tmp_i2c_client 'probe_i2c_client', but then I saw
> that the ir code needs it as well. If the ir code is fixed so it has its own
> i2c client, then the name can revert to probe_i2c_client.
>

Right. Adding it to the main structure is better than alloc and free the memory.
Would i2c_client_buf or i2c_client_data sound better than tmp_i2c_client?

-- Shuah

Re: [PATCH] em28xx: fix compiler warnings

[Frank Schäfer]

Hi Hans,

Am 05.08.2014 um 09:00 schrieb Hans Verkuil:
> Fix three compiler warnings:
>
> drivers/media/usb/em28xx/em28xx-input.c: In function ‘em28xx_i2c_ir_handle_key’:
> drivers/media/usb/em28xx/em28xx-input.c:318:1: warning: the frame size of 1096 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>  }
>  ^
>   CC [M]  drivers/media/usb/em28xx/em28xx-dvb.o
> drivers/media/usb/em28xx/em28xx-camera.c: In function ‘em28xx_probe_sensor_micron’:
> drivers/media/usb/em28xx/em28xx-camera.c:199:1: warning: the frame size of 1096 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>  }
>  ^
> drivers/media/usb/em28xx/em28xx-camera.c: In function ‘em28xx_probe_sensor_omnivision’:
> drivers/media/usb/em28xx/em28xx-camera.c:304:1: warning: the frame size of 1088 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>  }
>  ^
Hmmm... I don't get these weird warnings.
How can I reproduce them ?

> Note: there is no way the code in em28xx_i2c_ir_handle_key() is correct: it's
> using an almost completely uninitialized i2c_client struct with random flags,
> dev and name fields. Can't this turned into a proper i2c_client struct in
> struct em28xx? At least with this patch it's no longer random data.
Why do you think the client setup is random ?
Which fields do you think are wrong ? AFAICS this patch doesn't change
any fields.

What's wrong with using local i2c_client variables ?

Indeed, the way the driver currently tracks i2c clients / subdevices is
... let's say "improvable".
But IMHO, we should go the opposite direction and get rid of the
i2c_clients in the main device struct.
They are in fact just temporary helpers and dangerous to use with
devices with multiple i2c clients on the same bus.

Regards,
Frank


>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>
> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
> index 6d2ea9a..c8490ba 100644
> --- a/drivers/media/usb/em28xx/em28xx-camera.c
> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
> @@ -110,40 +110,40 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
>  	__be16 id_be;
>  	u16 id;
>  
> -	struct i2c_client client = dev->i2c_client[dev->def_i2c_bus];
> +	dev->tmp_i2c_client = dev->i2c_client[dev->def_i2c_bus];
>  
>  	dev->em28xx_sensor = EM28XX_NOSENSOR;
>  	for (i = 0; micron_sensor_addrs[i] != I2C_CLIENT_END; i++) {
> -		client.addr = micron_sensor_addrs[i];
> +		dev->tmp_i2c_client.addr = micron_sensor_addrs[i];
>  		/* NOTE: i2c_smbus_read_word_data() doesn't work with BE data */
>  		/* Read chip ID from register 0x00 */
>  		reg = 0x00;
> -		ret = i2c_master_send(&client, &reg, 1);
> +		ret = i2c_master_send(&dev->tmp_i2c_client, &reg, 1);
>  		if (ret < 0) {
>  			if (ret != -ENXIO)
>  				em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
> -					      client.addr << 1, ret);
> +					      dev->tmp_i2c_client.addr << 1, ret);
>  			continue;
>  		}
> -		ret = i2c_master_recv(&client, (u8 *)&id_be, 2);
> +		ret = i2c_master_recv(&dev->tmp_i2c_client, (u8 *)&id_be, 2);
>  		if (ret < 0) {
>  			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
> -				      client.addr << 1, ret);
> +				      dev->tmp_i2c_client.addr << 1, ret);
>  			continue;
>  		}
>  		id = be16_to_cpu(id_be);
>  		/* Read chip ID from register 0xff */
>  		reg = 0xff;
> -		ret = i2c_master_send(&client, &reg, 1);
> +		ret = i2c_master_send(&dev->tmp_i2c_client, &reg, 1);
>  		if (ret < 0) {
>  			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
> -				      client.addr << 1, ret);
> +				      dev->tmp_i2c_client.addr << 1, ret);
>  			continue;
>  		}
> -		ret = i2c_master_recv(&client, (u8 *)&id_be, 2);
> +		ret = i2c_master_recv(&dev->tmp_i2c_client, (u8 *)&id_be, 2);
>  		if (ret < 0) {
>  			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
> -				      client.addr << 1, ret);
> +				      dev->tmp_i2c_client.addr << 1, ret);
>  			continue;
>  		}
>  		/* Validate chip ID to be sure we have a Micron device */
> @@ -191,7 +191,7 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
>  		else
>  			em28xx_info("sensor %s detected\n", name);
>  
> -		dev->i2c_client[dev->def_i2c_bus].addr = client.addr;
> +		dev->i2c_client[dev->def_i2c_bus].addr = dev->tmp_i2c_client.addr;
>  		return 0;
>  	}
>  
> @@ -207,28 +207,29 @@ static int em28xx_probe_sensor_omnivision(struct em28xx *dev)
>  	char *name;
>  	u8 reg;
>  	u16 id;
> -	struct i2c_client client = dev->i2c_client[dev->def_i2c_bus];
> +
> +	dev->tmp_i2c_client = dev->i2c_client[dev->def_i2c_bus];
>  
>  	dev->em28xx_sensor = EM28XX_NOSENSOR;
>  	/* NOTE: these devices have the register auto incrementation disabled
>  	 * by default, so we have to use single byte reads !              */
>  	for (i = 0; omnivision_sensor_addrs[i] != I2C_CLIENT_END; i++) {
> -		client.addr = omnivision_sensor_addrs[i];
> +		dev->tmp_i2c_client.addr = omnivision_sensor_addrs[i];
>  		/* Read manufacturer ID from registers 0x1c-0x1d (BE) */
>  		reg = 0x1c;
> -		ret = i2c_smbus_read_byte_data(&client, reg);
> +		ret = i2c_smbus_read_byte_data(&dev->tmp_i2c_client, reg);
>  		if (ret < 0) {
>  			if (ret != -ENXIO)
>  				em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
> -					      client.addr << 1, ret);
> +					      dev->tmp_i2c_client.addr << 1, ret);
>  			continue;
>  		}
>  		id = ret << 8;
>  		reg = 0x1d;
> -		ret = i2c_smbus_read_byte_data(&client, reg);
> +		ret = i2c_smbus_read_byte_data(&dev->tmp_i2c_client, reg);
>  		if (ret < 0) {
>  			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
> -				      client.addr << 1, ret);
> +				      dev->tmp_i2c_client.addr << 1, ret);
>  			continue;
>  		}
>  		id += ret;
> @@ -237,18 +238,18 @@ static int em28xx_probe_sensor_omnivision(struct em28xx *dev)
>  			continue;
>  		/* Read product ID from registers 0x0a-0x0b (BE) */
>  		reg = 0x0a;
> -		ret = i2c_smbus_read_byte_data(&client, reg);
> +		ret = i2c_smbus_read_byte_data(&dev->tmp_i2c_client, reg);
>  		if (ret < 0) {
>  			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
> -				      client.addr << 1, ret);
> +				      dev->tmp_i2c_client.addr << 1, ret);
>  			continue;
>  		}
>  		id = ret << 8;
>  		reg = 0x0b;
> -		ret = i2c_smbus_read_byte_data(&client, reg);
> +		ret = i2c_smbus_read_byte_data(&dev->tmp_i2c_client, reg);
>  		if (ret < 0) {
>  			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
> -				      client.addr << 1, ret);
> +				      dev->tmp_i2c_client.addr << 1, ret);
>  			continue;
>  		}
>  		id += ret;
> @@ -296,7 +297,7 @@ static int em28xx_probe_sensor_omnivision(struct em28xx *dev)
>  		else
>  			em28xx_info("sensor %s detected\n", name);
>  
> -		dev->i2c_client[dev->def_i2c_bus].addr = client.addr;
> +		dev->i2c_client[dev->def_i2c_bus].addr = dev->tmp_i2c_client.addr;
>  		return 0;
>  	}
>  
> diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
> index ed843bd..07069b6 100644
> --- a/drivers/media/usb/em28xx/em28xx-input.c
> +++ b/drivers/media/usb/em28xx/em28xx-input.c
> @@ -298,12 +298,11 @@ static int em28xx_i2c_ir_handle_key(struct em28xx_IR *ir)
>  	static u32 scancode;
>  	enum rc_type protocol;
>  	int rc;
> -	struct i2c_client client;
>  
> -	client.adapter = &ir->dev->i2c_adap[dev->def_i2c_bus];
> -	client.addr = ir->i2c_dev_addr;
> +	dev->tmp_i2c_client.adapter = &ir->dev->i2c_adap[dev->def_i2c_bus];
> +	dev->tmp_i2c_client.addr = ir->i2c_dev_addr;
>  
> -	rc = ir->get_key_i2c(&client, &protocol, &scancode);
> +	rc = ir->get_key_i2c(&dev->tmp_i2c_client, &protocol, &scancode);
>  	if (rc < 0) {
>  		dprintk("ir->get_key_i2c() failed: %d\n", rc);
>  		return rc;
> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> index 84ef8ef..437ca08 100644
> --- a/drivers/media/usb/em28xx/em28xx.h
> +++ b/drivers/media/usb/em28xx/em28xx.h
> @@ -630,6 +630,7 @@ struct em28xx {
>  	struct i2c_adapter i2c_adap[NUM_I2C_BUSES];
>  	struct i2c_client i2c_client[NUM_I2C_BUSES];
>  	struct em28xx_i2c_bus i2c_bus[NUM_I2C_BUSES];
> +	struct i2c_client tmp_i2c_client;
>  
>  	unsigned char eeprom_addrwidth_16bit:1;
>  	unsigned def_i2c_bus;	/* Default I2C bus */

Re: [PATCH] em28xx: fix compiler warnings

[Hans Verkuil]

On 08/05/2014 05:18 PM, Frank Schäfer wrote:
> Hi Hans,
> 
> Am 05.08.2014 um 09:00 schrieb Hans Verkuil:
>> Fix three compiler warnings:
>>
>> drivers/media/usb/em28xx/em28xx-input.c: In function ‘em28xx_i2c_ir_handle_key’:
>> drivers/media/usb/em28xx/em28xx-input.c:318:1: warning: the frame size of 1096 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>>  }
>>  ^
>>   CC [M]  drivers/media/usb/em28xx/em28xx-dvb.o
>> drivers/media/usb/em28xx/em28xx-camera.c: In function ‘em28xx_probe_sensor_micron’:
>> drivers/media/usb/em28xx/em28xx-camera.c:199:1: warning: the frame size of 1096 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>>  }
>>  ^
>> drivers/media/usb/em28xx/em28xx-camera.c: In function ‘em28xx_probe_sensor_omnivision’:
>> drivers/media/usb/em28xx/em28xx-camera.c:304:1: warning: the frame size of 1088 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>>  }
>>  ^
> Hmmm... I don't get these weird warnings.
> How can I reproduce them ?

I'm using gcc 4.9.1 and I'm compiling the kernel using just a regular make command.
In my .config I have CONFIG_FRAME_WARN=1024.

> 
>> Note: there is no way the code in em28xx_i2c_ir_handle_key() is correct: it's
>> using an almost completely uninitialized i2c_client struct with random flags,
>> dev and name fields. Can't this turned into a proper i2c_client struct in
>> struct em28xx? At least with this patch it's no longer random data.
> Why do you think the client setup is random ?

Well, this is the code:

	struct i2c_client client;
 
	client.adapter = &ir->dev->i2c_adap[dev->def_i2c_bus];
	client.addr = ir->i2c_dev_addr;

All other fields of the client struct are undefined, but it is used as is. That
can't be right. With my patch the i2c_client is either that that was used by the
probe, or it is all zero. Which is still better than having random values.

> Which fields do you think are wrong ? AFAICS this patch doesn't change
> any fields.
> 
> What's wrong with using local i2c_client variables ?

Nothing, except that they take a lot of stack space which the compiler complains about.
The stack in the kernel is limited, so this should be avoided.

> 
> Indeed, the way the driver currently tracks i2c clients / subdevices is
> ... let's say "improvable".
> But IMHO, we should go the opposite direction and get rid of the
> i2c_clients in the main device struct.
> They are in fact just temporary helpers and dangerous to use with
> devices with multiple i2c clients on the same bus.

Feel free to find another solution, but allocating i2c_client structs on the
stack is not the right solution.

If you are wondering why these warnings are not seen in the daily build: I'm
increasing the CONFIG_FRAME_WARN setting to 2048. I'll actually disable this
for the next daily build. See what happens. I don't think there are many of
these warnings left, a lot have been cleaned up over the years.

Regards,

	Hans

> 
> Regards,
> Frank
> 
> 
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
>> index 6d2ea9a..c8490ba 100644
>> --- a/drivers/media/usb/em28xx/em28xx-camera.c
>> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
>> @@ -110,40 +110,40 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
>>  	__be16 id_be;
>>  	u16 id;
>>  
>> -	struct i2c_client client = dev->i2c_client[dev->def_i2c_bus];
>> +	dev->tmp_i2c_client = dev->i2c_client[dev->def_i2c_bus];
>>  
>>  	dev->em28xx_sensor = EM28XX_NOSENSOR;
>>  	for (i = 0; micron_sensor_addrs[i] != I2C_CLIENT_END; i++) {
>> -		client.addr = micron_sensor_addrs[i];
>> +		dev->tmp_i2c_client.addr = micron_sensor_addrs[i];
>>  		/* NOTE: i2c_smbus_read_word_data() doesn't work with BE data */
>>  		/* Read chip ID from register 0x00 */
>>  		reg = 0x00;
>> -		ret = i2c_master_send(&client, &reg, 1);
>> +		ret = i2c_master_send(&dev->tmp_i2c_client, &reg, 1);
>>  		if (ret < 0) {
>>  			if (ret != -ENXIO)
>>  				em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
>> -					      client.addr << 1, ret);
>> +					      dev->tmp_i2c_client.addr << 1, ret);
>>  			continue;
>>  		}
>> -		ret = i2c_master_recv(&client, (u8 *)&id_be, 2);
>> +		ret = i2c_master_recv(&dev->tmp_i2c_client, (u8 *)&id_be, 2);
>>  		if (ret < 0) {
>>  			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
>> -				      client.addr << 1, ret);
>> +				      dev->tmp_i2c_client.addr << 1, ret);
>>  			continue;
>>  		}
>>  		id = be16_to_cpu(id_be);
>>  		/* Read chip ID from register 0xff */
>>  		reg = 0xff;
>> -		ret = i2c_master_send(&client, &reg, 1);
>> +		ret = i2c_master_send(&dev->tmp_i2c_client, &reg, 1);
>>  		if (ret < 0) {
>>  			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
>> -				      client.addr << 1, ret);
>> +				      dev->tmp_i2c_client.addr << 1, ret);
>>  			continue;
>>  		}
>> -		ret = i2c_master_recv(&client, (u8 *)&id_be, 2);
>> +		ret = i2c_master_recv(&dev->tmp_i2c_client, (u8 *)&id_be, 2);
>>  		if (ret < 0) {
>>  			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
>> -				      client.addr << 1, ret);
>> +				      dev->tmp_i2c_client.addr << 1, ret);
>>  			continue;
>>  		}
>>  		/* Validate chip ID to be sure we have a Micron device */
>> @@ -191,7 +191,7 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
>>  		else
>>  			em28xx_info("sensor %s detected\n", name);
>>  
>> -		dev->i2c_client[dev->def_i2c_bus].addr = client.addr;
>> +		dev->i2c_client[dev->def_i2c_bus].addr = dev->tmp_i2c_client.addr;
>>  		return 0;
>>  	}
>>  
>> @@ -207,28 +207,29 @@ static int em28xx_probe_sensor_omnivision(struct em28xx *dev)
>>  	char *name;
>>  	u8 reg;
>>  	u16 id;
>> -	struct i2c_client client = dev->i2c_client[dev->def_i2c_bus];
>> +
>> +	dev->tmp_i2c_client = dev->i2c_client[dev->def_i2c_bus];
>>  
>>  	dev->em28xx_sensor = EM28XX_NOSENSOR;
>>  	/* NOTE: these devices have the register auto incrementation disabled
>>  	 * by default, so we have to use single byte reads !              */
>>  	for (i = 0; omnivision_sensor_addrs[i] != I2C_CLIENT_END; i++) {
>> -		client.addr = omnivision_sensor_addrs[i];
>> +		dev->tmp_i2c_client.addr = omnivision_sensor_addrs[i];
>>  		/* Read manufacturer ID from registers 0x1c-0x1d (BE) */
>>  		reg = 0x1c;
>> -		ret = i2c_smbus_read_byte_data(&client, reg);
>> +		ret = i2c_smbus_read_byte_data(&dev->tmp_i2c_client, reg);
>>  		if (ret < 0) {
>>  			if (ret != -ENXIO)
>>  				em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
>> -					      client.addr << 1, ret);
>> +					      dev->tmp_i2c_client.addr << 1, ret);
>>  			continue;
>>  		}
>>  		id = ret << 8;
>>  		reg = 0x1d;
>> -		ret = i2c_smbus_read_byte_data(&client, reg);
>> +		ret = i2c_smbus_read_byte_data(&dev->tmp_i2c_client, reg);
>>  		if (ret < 0) {
>>  			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
>> -				      client.addr << 1, ret);
>> +				      dev->tmp_i2c_client.addr << 1, ret);
>>  			continue;
>>  		}
>>  		id += ret;
>> @@ -237,18 +238,18 @@ static int em28xx_probe_sensor_omnivision(struct em28xx *dev)
>>  			continue;
>>  		/* Read product ID from registers 0x0a-0x0b (BE) */
>>  		reg = 0x0a;
>> -		ret = i2c_smbus_read_byte_data(&client, reg);
>> +		ret = i2c_smbus_read_byte_data(&dev->tmp_i2c_client, reg);
>>  		if (ret < 0) {
>>  			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
>> -				      client.addr << 1, ret);
>> +				      dev->tmp_i2c_client.addr << 1, ret);
>>  			continue;
>>  		}
>>  		id = ret << 8;
>>  		reg = 0x0b;
>> -		ret = i2c_smbus_read_byte_data(&client, reg);
>> +		ret = i2c_smbus_read_byte_data(&dev->tmp_i2c_client, reg);
>>  		if (ret < 0) {
>>  			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
>> -				      client.addr << 1, ret);
>> +				      dev->tmp_i2c_client.addr << 1, ret);
>>  			continue;
>>  		}
>>  		id += ret;
>> @@ -296,7 +297,7 @@ static int em28xx_probe_sensor_omnivision(struct em28xx *dev)
>>  		else
>>  			em28xx_info("sensor %s detected\n", name);
>>  
>> -		dev->i2c_client[dev->def_i2c_bus].addr = client.addr;
>> +		dev->i2c_client[dev->def_i2c_bus].addr = dev->tmp_i2c_client.addr;
>>  		return 0;
>>  	}
>>  
>> diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
>> index ed843bd..07069b6 100644
>> --- a/drivers/media/usb/em28xx/em28xx-input.c
>> +++ b/drivers/media/usb/em28xx/em28xx-input.c
>> @@ -298,12 +298,11 @@ static int em28xx_i2c_ir_handle_key(struct em28xx_IR *ir)
>>  	static u32 scancode;
>>  	enum rc_type protocol;
>>  	int rc;
>> -	struct i2c_client client;
>>  
>> -	client.adapter = &ir->dev->i2c_adap[dev->def_i2c_bus];
>> -	client.addr = ir->i2c_dev_addr;
>> +	dev->tmp_i2c_client.adapter = &ir->dev->i2c_adap[dev->def_i2c_bus];
>> +	dev->tmp_i2c_client.addr = ir->i2c_dev_addr;
>>  
>> -	rc = ir->get_key_i2c(&client, &protocol, &scancode);
>> +	rc = ir->get_key_i2c(&dev->tmp_i2c_client, &protocol, &scancode);
>>  	if (rc < 0) {
>>  		dprintk("ir->get_key_i2c() failed: %d\n", rc);
>>  		return rc;
>> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
>> index 84ef8ef..437ca08 100644
>> --- a/drivers/media/usb/em28xx/em28xx.h
>> +++ b/drivers/media/usb/em28xx/em28xx.h
>> @@ -630,6 +630,7 @@ struct em28xx {
>>  	struct i2c_adapter i2c_adap[NUM_I2C_BUSES];
>>  	struct i2c_client i2c_client[NUM_I2C_BUSES];
>>  	struct em28xx_i2c_bus i2c_bus[NUM_I2C_BUSES];
>> +	struct i2c_client tmp_i2c_client;
>>  
>>  	unsigned char eeprom_addrwidth_16bit:1;
>>  	unsigned def_i2c_bus;	/* Default I2C bus */
> 

Re: [PATCH] em28xx: fix compiler warnings

[Frank Schäfer]

Am 07.08.2014 um 08:45 schrieb Hans Verkuil:
> On 08/05/2014 05:18 PM, Frank Schäfer wrote:
>> Hi Hans,
>>
>> Am 05.08.2014 um 09:00 schrieb Hans Verkuil:
>>> Fix three compiler warnings:
>>>
>>> drivers/media/usb/em28xx/em28xx-input.c: In function ‘em28xx_i2c_ir_handle_key’:
>>> drivers/media/usb/em28xx/em28xx-input.c:318:1: warning: the frame size of 1096 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>>>  }
>>>  ^
>>>   CC [M]  drivers/media/usb/em28xx/em28xx-dvb.o
>>> drivers/media/usb/em28xx/em28xx-camera.c: In function ‘em28xx_probe_sensor_micron’:
>>> drivers/media/usb/em28xx/em28xx-camera.c:199:1: warning: the frame size of 1096 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>>>  }
>>>  ^
>>> drivers/media/usb/em28xx/em28xx-camera.c: In function ‘em28xx_probe_sensor_omnivision’:
>>> drivers/media/usb/em28xx/em28xx-camera.c:304:1: warning: the frame size of 1088 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>>>  }
>>>  ^
>> Hmmm... I don't get these weird warnings.
>> How can I reproduce them ?
> I'm using gcc 4.9.1 and I'm compiling the kernel using just a regular make command.
> In my .config I have CONFIG_FRAME_WARN=1024.
Weird. With gcc version 4.8.1 20130909 [gcc-4_8-branch revision 202388]
I get much smaller frame sizes:

...
drivers/media/usb/em28xx/em28xx-input.c: In function
‘em28xx_i2c_ir_handle_key’:
drivers/media/usb/em28xx/em28xx-input.c:318:1: warning: the frame size
of 424 bytes is larger than 256 bytes [-Wframe-larger-than=]
 }
 ^
...
drivers/media/usb/em28xx/em28xx-camera.c: In function
‘em28xx_probe_sensor_micron’:
drivers/media/usb/em28xx/em28xx-camera.c:199:1: warning: the frame size
of 432 bytes is larger than 256 bytes [-Wframe-larger-than=]
 }
 ^
...
drivers/media/usb/em28xx/em28xx-camera.c: In function
‘em28xx_probe_sensor_omnivision’:
drivers/media/usb/em28xx/em28xx-camera.c:304:1: warning: the frame size
of 428 bytes is larger than 256 bytes [-Wframe-larger-than=]
 }
 ^
...


Anyway, I really don't think a framesize of 1096 is a problem.

>>> Note: there is no way the code in em28xx_i2c_ir_handle_key() is correct: it's
>>> using an almost completely uninitialized i2c_client struct with random flags,
>>> dev and name fields. Can't this turned into a proper i2c_client struct in
>>> struct em28xx? At least with this patch it's no longer random data.
>> Why do you think the client setup is random ?
> Well, this is the code:
>
> 	struct i2c_client client;
>  
> 	client.adapter = &ir->dev->i2c_adap[dev->def_i2c_bus];
> 	client.addr = ir->i2c_dev_addr;
>
> All other fields of the client struct are undefined, but it is used as is. That
> can't be right. With my patch the i2c_client is either that that was used by the
> probe, or it is all zero. Which is still better than having random values.
Take a closer look at the code:

1.) sensor probing:

    struct i2c_client client = dev->i2c_client[dev->def_i2c_bus];

dev->i2c_client[bus] is initialized on bus registration in
em28xx_i2c_register():

    dev->i2c_client[bus] = em28xx_client_template;
    dev->i2c_client[bus].adapter = &dev->i2c_adap[bus];

em28xx_client_template is defined static:

static struct i2c_client em28xx_client_template = {
    .name = "em28xx internal",
};

So nothing is random or undefined here.


2.) i2c rc key polling:

em28xx_i2c_ir_handle_key() passes the client structure to one of the 4
get_key functions

    rc = ir->get_key_i2c(&client, &protocol, &scancode);

which either call

    i2c_transfer(client->adapter, msg, len)

directly or the helper function

    i2c_master_recv(client, buf, len))

which creates an i2c message before calling i2c_transfer().
The only members used from the i2c_client struct are

    msg.addr = client->addr;
    msg.flags = client->flags & I2C_M_TEN;

So the only fields from struct i2c_client which need to be setup are
"adapter" and "addr" and "flags".
Adapter an addres are initialized properly to

    client.adapter = &ir->dev->i2c_adap[dev->def_i2c_bus];
    client.addr = ir->i2c_dev_addr;

The only thing which is indeed missing here and needs to be fixed is

    client.flags = 0;


>> Which fields do you think are wrong ? AFAICS this patch doesn't change
>> any fields.
>>
>> What's wrong with using local i2c_client variables ?
> Nothing, except that they take a lot of stack space which the compiler complains about.
Well, it warns because you are forcing a warning.

> The stack in the kernel is limited, so this should be avoided.
Limited to what ? 8k ? So what's the problem ?

>> Indeed, the way the driver currently tracks i2c clients / subdevices is
>> ... let's say "improvable".
>> But IMHO, we should go the opposite direction and get rid of the
>> i2c_clients in the main device struct.
>> They are in fact just temporary helpers and dangerous to use with
>> devices with multiple i2c clients on the same bus.
> Feel free to find another solution, but allocating i2c_client structs on the
> stack is not the right solution.
Putting temporary helper variables which are even unused by most devices
into the main device struct is definitely is the wrong solution.
struct em28xx is already bloated with tons of varibales.
Going local is the right solution for stuff like this. I would really
like to get rid of dev->i2c_client[], too.

Anyway, in the specific case of em28xx-input it seems to be reasonable
to put the i2c_client to struct em28xx_IR and set it up only one time at
module initialization.
This will speed up things. :D
But please, use malloc/free here, its pretty simple and really not
forbidden. ;-)
97% of the em28xx devices don't have an i2c ir decoder.

Regards,
Frank


> If you are wondering why these warnings are not seen in the daily build: I'm
> increasing the CONFIG_FRAME_WARN setting to 2048. I'll actually disable this
> for the next daily build. See what happens. I don't think there are many of
> these warnings left, a lot have been cleaned up over the years.
>
> Regards,
>
> 	Hans
>
>> Regards,
>> Frank
>>
>>
>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>
>>> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
>>> index 6d2ea9a..c8490ba 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-camera.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
>>> @@ -110,40 +110,40 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
>>>  	__be16 id_be;
>>>  	u16 id;
>>>  
>>> -	struct i2c_client client = dev->i2c_client[dev->def_i2c_bus];
>>> +	dev->tmp_i2c_client = dev->i2c_client[dev->def_i2c_bus];
>>>  
>>>  	dev->em28xx_sensor = EM28XX_NOSENSOR;
>>>  	for (i = 0; micron_sensor_addrs[i] != I2C_CLIENT_END; i++) {
>>> -		client.addr = micron_sensor_addrs[i];
>>> +		dev->tmp_i2c_client.addr = micron_sensor_addrs[i];
>>>  		/* NOTE: i2c_smbus_read_word_data() doesn't work with BE data */
>>>  		/* Read chip ID from register 0x00 */
>>>  		reg = 0x00;
>>> -		ret = i2c_master_send(&client, &reg, 1);
>>> +		ret = i2c_master_send(&dev->tmp_i2c_client, &reg, 1);
>>>  		if (ret < 0) {
>>>  			if (ret != -ENXIO)
>>>  				em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
>>> -					      client.addr << 1, ret);
>>> +					      dev->tmp_i2c_client.addr << 1, ret);
>>>  			continue;
>>>  		}
>>> -		ret = i2c_master_recv(&client, (u8 *)&id_be, 2);
>>> +		ret = i2c_master_recv(&dev->tmp_i2c_client, (u8 *)&id_be, 2);
>>>  		if (ret < 0) {
>>>  			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
>>> -				      client.addr << 1, ret);
>>> +				      dev->tmp_i2c_client.addr << 1, ret);
>>>  			continue;
>>>  		}
>>>  		id = be16_to_cpu(id_be);
>>>  		/* Read chip ID from register 0xff */
>>>  		reg = 0xff;
>>> -		ret = i2c_master_send(&client, &reg, 1);
>>> +		ret = i2c_master_send(&dev->tmp_i2c_client, &reg, 1);
>>>  		if (ret < 0) {
>>>  			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
>>> -				      client.addr << 1, ret);
>>> +				      dev->tmp_i2c_client.addr << 1, ret);
>>>  			continue;
>>>  		}
>>> -		ret = i2c_master_recv(&client, (u8 *)&id_be, 2);
>>> +		ret = i2c_master_recv(&dev->tmp_i2c_client, (u8 *)&id_be, 2);
>>>  		if (ret < 0) {
>>>  			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
>>> -				      client.addr << 1, ret);
>>> +				      dev->tmp_i2c_client.addr << 1, ret);
>>>  			continue;
>>>  		}
>>>  		/* Validate chip ID to be sure we have a Micron device */
>>> @@ -191,7 +191,7 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
>>>  		else
>>>  			em28xx_info("sensor %s detected\n", name);
>>>  
>>> -		dev->i2c_client[dev->def_i2c_bus].addr = client.addr;
>>> +		dev->i2c_client[dev->def_i2c_bus].addr = dev->tmp_i2c_client.addr;
>>>  		return 0;
>>>  	}
>>>  
>>> @@ -207,28 +207,29 @@ static int em28xx_probe_sensor_omnivision(struct em28xx *dev)
>>>  	char *name;
>>>  	u8 reg;
>>>  	u16 id;
>>> -	struct i2c_client client = dev->i2c_client[dev->def_i2c_bus];
>>> +
>>> +	dev->tmp_i2c_client = dev->i2c_client[dev->def_i2c_bus];
>>>  
>>>  	dev->em28xx_sensor = EM28XX_NOSENSOR;
>>>  	/* NOTE: these devices have the register auto incrementation disabled
>>>  	 * by default, so we have to use single byte reads !              */
>>>  	for (i = 0; omnivision_sensor_addrs[i] != I2C_CLIENT_END; i++) {
>>> -		client.addr = omnivision_sensor_addrs[i];
>>> +		dev->tmp_i2c_client.addr = omnivision_sensor_addrs[i];
>>>  		/* Read manufacturer ID from registers 0x1c-0x1d (BE) */
>>>  		reg = 0x1c;
>>> -		ret = i2c_smbus_read_byte_data(&client, reg);
>>> +		ret = i2c_smbus_read_byte_data(&dev->tmp_i2c_client, reg);
>>>  		if (ret < 0) {
>>>  			if (ret != -ENXIO)
>>>  				em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
>>> -					      client.addr << 1, ret);
>>> +					      dev->tmp_i2c_client.addr << 1, ret);
>>>  			continue;
>>>  		}
>>>  		id = ret << 8;
>>>  		reg = 0x1d;
>>> -		ret = i2c_smbus_read_byte_data(&client, reg);
>>> +		ret = i2c_smbus_read_byte_data(&dev->tmp_i2c_client, reg);
>>>  		if (ret < 0) {
>>>  			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
>>> -				      client.addr << 1, ret);
>>> +				      dev->tmp_i2c_client.addr << 1, ret);
>>>  			continue;
>>>  		}
>>>  		id += ret;
>>> @@ -237,18 +238,18 @@ static int em28xx_probe_sensor_omnivision(struct em28xx *dev)
>>>  			continue;
>>>  		/* Read product ID from registers 0x0a-0x0b (BE) */
>>>  		reg = 0x0a;
>>> -		ret = i2c_smbus_read_byte_data(&client, reg);
>>> +		ret = i2c_smbus_read_byte_data(&dev->tmp_i2c_client, reg);
>>>  		if (ret < 0) {
>>>  			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
>>> -				      client.addr << 1, ret);
>>> +				      dev->tmp_i2c_client.addr << 1, ret);
>>>  			continue;
>>>  		}
>>>  		id = ret << 8;
>>>  		reg = 0x0b;
>>> -		ret = i2c_smbus_read_byte_data(&client, reg);
>>> +		ret = i2c_smbus_read_byte_data(&dev->tmp_i2c_client, reg);
>>>  		if (ret < 0) {
>>>  			em28xx_errdev("couldn't read from i2c device 0x%02x: error %i\n",
>>> -				      client.addr << 1, ret);
>>> +				      dev->tmp_i2c_client.addr << 1, ret);
>>>  			continue;
>>>  		}
>>>  		id += ret;
>>> @@ -296,7 +297,7 @@ static int em28xx_probe_sensor_omnivision(struct em28xx *dev)
>>>  		else
>>>  			em28xx_info("sensor %s detected\n", name);
>>>  
>>> -		dev->i2c_client[dev->def_i2c_bus].addr = client.addr;
>>> +		dev->i2c_client[dev->def_i2c_bus].addr = dev->tmp_i2c_client.addr;
>>>  		return 0;
>>>  	}
>>>  
>>> diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
>>> index ed843bd..07069b6 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-input.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-input.c
>>> @@ -298,12 +298,11 @@ static int em28xx_i2c_ir_handle_key(struct em28xx_IR *ir)
>>>  	static u32 scancode;
>>>  	enum rc_type protocol;
>>>  	int rc;
>>> -	struct i2c_client client;
>>>  
>>> -	client.adapter = &ir->dev->i2c_adap[dev->def_i2c_bus];
>>> -	client.addr = ir->i2c_dev_addr;
>>> +	dev->tmp_i2c_client.adapter = &ir->dev->i2c_adap[dev->def_i2c_bus];
>>> +	dev->tmp_i2c_client.addr = ir->i2c_dev_addr;
>>>  
>>> -	rc = ir->get_key_i2c(&client, &protocol, &scancode);
>>> +	rc = ir->get_key_i2c(&dev->tmp_i2c_client, &protocol, &scancode);
>>>  	if (rc < 0) {
>>>  		dprintk("ir->get_key_i2c() failed: %d\n", rc);
>>>  		return rc;
>>> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
>>> index 84ef8ef..437ca08 100644
>>> --- a/drivers/media/usb/em28xx/em28xx.h
>>> +++ b/drivers/media/usb/em28xx/em28xx.h
>>> @@ -630,6 +630,7 @@ struct em28xx {
>>>  	struct i2c_adapter i2c_adap[NUM_I2C_BUSES];
>>>  	struct i2c_client i2c_client[NUM_I2C_BUSES];
>>>  	struct em28xx_i2c_bus i2c_bus[NUM_I2C_BUSES];
>>> +	struct i2c_client tmp_i2c_client;
>>>  
>>>  	unsigned char eeprom_addrwidth_16bit:1;
>>>  	unsigned def_i2c_bus;	/* Default I2C bus */

Re: [PATCH] em28xx: fix compiler warnings

[Hans Verkuil]

On 08/07/2014 06:36 PM, Frank Schäfer wrote:
> 
> Am 07.08.2014 um 08:45 schrieb Hans Verkuil:
>> On 08/05/2014 05:18 PM, Frank Schäfer wrote:
>>> Hi Hans,
>>>
>>> Am 05.08.2014 um 09:00 schrieb Hans Verkuil:
>>>> Fix three compiler warnings:
>>>>
>>>> drivers/media/usb/em28xx/em28xx-input.c: In function ‘em28xx_i2c_ir_handle_key’:
>>>> drivers/media/usb/em28xx/em28xx-input.c:318:1: warning: the frame size of 1096 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>>>>  }
>>>>  ^
>>>>   CC [M]  drivers/media/usb/em28xx/em28xx-dvb.o
>>>> drivers/media/usb/em28xx/em28xx-camera.c: In function ‘em28xx_probe_sensor_micron’:
>>>> drivers/media/usb/em28xx/em28xx-camera.c:199:1: warning: the frame size of 1096 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>>>>  }
>>>>  ^
>>>> drivers/media/usb/em28xx/em28xx-camera.c: In function ‘em28xx_probe_sensor_omnivision’:
>>>> drivers/media/usb/em28xx/em28xx-camera.c:304:1: warning: the frame size of 1088 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>>>>  }
>>>>  ^
>>> Hmmm... I don't get these weird warnings.
>>> How can I reproduce them ?
>> I'm using gcc 4.9.1 and I'm compiling the kernel using just a regular make command.
>> In my .config I have CONFIG_FRAME_WARN=1024.
> Weird. With gcc version 4.8.1 20130909 [gcc-4_8-branch revision 202388]
> I get much smaller frame sizes:

Are you compiling for 32 or 64 bits? I'm compiling for 64 bits.

> 
> ...
> drivers/media/usb/em28xx/em28xx-input.c: In function
> ‘em28xx_i2c_ir_handle_key’:
> drivers/media/usb/em28xx/em28xx-input.c:318:1: warning: the frame size
> of 424 bytes is larger than 256 bytes [-Wframe-larger-than=]
>  }
>  ^
> ...
> drivers/media/usb/em28xx/em28xx-camera.c: In function
> ‘em28xx_probe_sensor_micron’:
> drivers/media/usb/em28xx/em28xx-camera.c:199:1: warning: the frame size
> of 432 bytes is larger than 256 bytes [-Wframe-larger-than=]
>  }
>  ^
> ...
> drivers/media/usb/em28xx/em28xx-camera.c: In function
> ‘em28xx_probe_sensor_omnivision’:
> drivers/media/usb/em28xx/em28xx-camera.c:304:1: warning: the frame size
> of 428 bytes is larger than 256 bytes [-Wframe-larger-than=]
>  }
>  ^
> ...
> 
> 
> Anyway, I really don't think a framesize of 1096 is a problem.

And I agree. I did some digging and for some reason the framesize warning was configured
for 1024 bytes on my 64-bit system, when the default is 2048. In fact, in the daily
build I removed the hacks that increased the size of this config option and everything
compiles just fine without warnings. Apparently all the offenders have been addressed
over the years.

> 
>>>> Note: there is no way the code in em28xx_i2c_ir_handle_key() is correct: it's
>>>> using an almost completely uninitialized i2c_client struct with random flags,
>>>> dev and name fields. Can't this turned into a proper i2c_client struct in
>>>> struct em28xx? At least with this patch it's no longer random data.
>>> Why do you think the client setup is random ?
>> Well, this is the code:
>>
>> 	struct i2c_client client;
>>  
>> 	client.adapter = &ir->dev->i2c_adap[dev->def_i2c_bus];
>> 	client.addr = ir->i2c_dev_addr;
>>
>> All other fields of the client struct are undefined, but it is used as is. That
>> can't be right. With my patch the i2c_client is either that that was used by the
>> probe, or it is all zero. Which is still better than having random values.
> Take a closer look at the code:
> 
> 1.) sensor probing:
> 
>     struct i2c_client client = dev->i2c_client[dev->def_i2c_bus];
> 
> dev->i2c_client[bus] is initialized on bus registration in
> em28xx_i2c_register():
> 
>     dev->i2c_client[bus] = em28xx_client_template;
>     dev->i2c_client[bus].adapter = &dev->i2c_adap[bus];
> 
> em28xx_client_template is defined static:
> 
> static struct i2c_client em28xx_client_template = {
>     .name = "em28xx internal",
> };
> 
> So nothing is random or undefined here.

I never said that. For sensor probing it is fine.

> 
> 
> 2.) i2c rc key polling:
> 
> em28xx_i2c_ir_handle_key() passes the client structure to one of the 4
> get_key functions
> 
>     rc = ir->get_key_i2c(&client, &protocol, &scancode);
> 
> which either call
> 
>     i2c_transfer(client->adapter, msg, len)
> 
> directly or the helper function
> 
>     i2c_master_recv(client, buf, len))
> 
> which creates an i2c message before calling i2c_transfer().
> The only members used from the i2c_client struct are
> 
>     msg.addr = client->addr;
>     msg.flags = client->flags & I2C_M_TEN;
> 
> So the only fields from struct i2c_client which need to be setup are
> "adapter" and "addr" and "flags".
> Adapter an addres are initialized properly to
> 
>     client.adapter = &ir->dev->i2c_adap[dev->def_i2c_bus];
>     client.addr = ir->i2c_dev_addr;
> 
> The only thing which is indeed missing here and needs to be fixed is
> 
>     client.flags = 0;

Are there no debugging calls that use client.name? Basically what I don't
understand is why this isn't a proper i2c_client, registered and all and
in its proper place in the /sys/ hierarchy.

It feels very much like a quick hack. And if nothing else, at least zero
the struct before use. That will make any problems that this hack causes
reproducible instead of dependent on whatever random values were on the
stack.

Regards,

	Hans

Re: [PATCH] em28xx: fix compiler warnings

[Frank Schäfer]

Am 09.08.2014 um 11:58 schrieb Hans Verkuil:
> On 08/07/2014 06:36 PM, Frank Schäfer wrote:
>> Am 07.08.2014 um 08:45 schrieb Hans Verkuil:
>>> On 08/05/2014 05:18 PM, Frank Schäfer wrote:
>>>> Hi Hans,
>>>>
>>>> Am 05.08.2014 um 09:00 schrieb Hans Verkuil:
>>>>> Fix three compiler warnings:
>>>>>
>>>>> drivers/media/usb/em28xx/em28xx-input.c: In function ‘em28xx_i2c_ir_handle_key’:
>>>>> drivers/media/usb/em28xx/em28xx-input.c:318:1: warning: the frame size of 1096 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>>>>>  }
>>>>>  ^
>>>>>   CC [M]  drivers/media/usb/em28xx/em28xx-dvb.o
>>>>> drivers/media/usb/em28xx/em28xx-camera.c: In function ‘em28xx_probe_sensor_micron’:
>>>>> drivers/media/usb/em28xx/em28xx-camera.c:199:1: warning: the frame size of 1096 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>>>>>  }
>>>>>  ^
>>>>> drivers/media/usb/em28xx/em28xx-camera.c: In function ‘em28xx_probe_sensor_omnivision’:
>>>>> drivers/media/usb/em28xx/em28xx-camera.c:304:1: warning: the frame size of 1088 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>>>>>  }
>>>>>  ^
>>>> Hmmm... I don't get these weird warnings.
>>>> How can I reproduce them ?
>>> I'm using gcc 4.9.1 and I'm compiling the kernel using just a regular make command.
>>> In my .config I have CONFIG_FRAME_WARN=1024.
>> Weird. With gcc version 4.8.1 20130909 [gcc-4_8-branch revision 202388]
>> I get much smaller frame sizes:
> Are you compiling for 32 or 64 bits? I'm compiling for 64 bits.

Ah yes, it was a 32 bit kernel.
With a 64 bit kernel I get frame sizes of 736-744 bytes.
Hmm... still much smaller than the 1088-1096 bytes gcc 4.9.1 reports...

>
>> ...
>> drivers/media/usb/em28xx/em28xx-input.c: In function
>> ‘em28xx_i2c_ir_handle_key’:
>> drivers/media/usb/em28xx/em28xx-input.c:318:1: warning: the frame size
>> of 424 bytes is larger than 256 bytes [-Wframe-larger-than=]
>>  }
>>  ^
>> ...
>> drivers/media/usb/em28xx/em28xx-camera.c: In function
>> ‘em28xx_probe_sensor_micron’:
>> drivers/media/usb/em28xx/em28xx-camera.c:199:1: warning: the frame size
>> of 432 bytes is larger than 256 bytes [-Wframe-larger-than=]
>>  }
>>  ^
>> ...
>> drivers/media/usb/em28xx/em28xx-camera.c: In function
>> ‘em28xx_probe_sensor_omnivision’:
>> drivers/media/usb/em28xx/em28xx-camera.c:304:1: warning: the frame size
>> of 428 bytes is larger than 256 bytes [-Wframe-larger-than=]
>>  }
>>  ^
>> ...
[...]
>> 2.) i2c rc key polling:
>>
>> em28xx_i2c_ir_handle_key() passes the client structure to one of the 4
>> get_key functions
>>
>>     rc = ir->get_key_i2c(&client, &protocol, &scancode);
>>
>> which either call
>>
>>     i2c_transfer(client->adapter, msg, len)
>>
>> directly or the helper function
>>
>>     i2c_master_recv(client, buf, len))
>>
>> which creates an i2c message before calling i2c_transfer().
>> The only members used from the i2c_client struct are
>>
>>     msg.addr = client->addr;
>>     msg.flags = client->flags & I2C_M_TEN;
>>
>> So the only fields from struct i2c_client which need to be setup are
>> "adapter" and "addr" and "flags".
>> Adapter an addres are initialized properly to
>>
>>     client.adapter = &ir->dev->i2c_adap[dev->def_i2c_bus];
>>     client.addr = ir->i2c_dev_addr;
>>
>> The only thing which is indeed missing here and needs to be fixed is
>>
>>     client.flags = 0;
> Are there no debugging calls that use client.name? 

No, only adpater, address and flags are required/used for an i2c_transfer().

> Basically what I don't
> understand is why this isn't a proper i2c_client, registered and all and
> in its proper place in the /sys/ hierarchy.
>
> It feels very much like a quick hack. 

The ir i2c decoder is indeed a pure em28xx internal thing. No external
client driver is used.
Hence the code is straight forward.
The only benefit of registering an i2c_client would be to make it
available via sysfs.
I'm not sure If it's worth the amount of extra work+code.

> And if nothing else, at least zero
> the struct before use. That will make any problems that this hack causes
> reproducible instead of dependent on whatever random values were on the
> stack.

The only problem here is the missing initialization of field "flags".
It's a bug that needs to be fixed. Fortunately it doesn't cause any trouble.

In general, if structs with optional fields are used, the reader of the
code easily gets the feeling that something might be missing.
I know what I'm talking about... ;-)

A patch is in the works.

Regards,
Frank

> Regards,
>
> 	Hans