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@xxxxxxxxx>
>>
>> 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, ®, 1);
>> + ret = i2c_master_send(&dev->tmp_i2c_client, ®, 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, ®, 1);
>> + ret = i2c_master_send(&dev->tmp_i2c_client, ®, 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 */
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
