* [PATCH 1/4] [media] pvrusb2: reduce stack usage pvr2_eeprom_analyze()
@ 2017-02-02 14:53 Arnd Bergmann
2017-02-02 14:53 ` [PATCH 2/4] [media] em28xx: reduce stack usage in probe functions Arnd Bergmann
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Arnd Bergmann @ 2017-02-02 14:53 UTC (permalink / raw)
To: Mike Isely, Mauro Carvalho Chehab
Cc: Arnd Bergmann, linux-media, linux-kernel
The driver uses a relatively large data structure on the stack, which
showed up on my radar as we get a warning with the "latent entropy"
GCC plugin:
drivers/media/usb/pvrusb2/pvrusb2-eeprom.c:153:1: error: the frame size of 1376 bytes is larger than 1152 bytes [-Werror=frame-larger-than=]
The warning is usually hidden as we raise the warning limit to 2048
when the plugin is enabled, but I'd like to lower that again in the
future, and making this function smaller helps to do that without
build regressions.
Further analysis shows that putting an 'i2c_client' structure on
the stack is not really supported, as the embedded 'struct device'
is not initialized here, and we are only saved by the fact that
the function that is called here does not use the pointer at all.
Fixes: d855497edbfb ("V4L/DVB (4228a): pvrusb2 to kernel 2.6.18")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/media/usb/pvrusb2/pvrusb2-eeprom.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-eeprom.c b/drivers/media/usb/pvrusb2/pvrusb2-eeprom.c
index 276b17fb9aad..b8fcd9660094 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-eeprom.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-eeprom.c
@@ -122,15 +122,10 @@ int pvr2_eeprom_analyze(struct pvr2_hdw *hdw)
memset(&tvdata,0,sizeof(tvdata));
eeprom = pvr2_eeprom_fetch(hdw);
- if (!eeprom) return -EINVAL;
-
- {
- struct i2c_client fake_client;
- /* Newer version expects a useless client interface */
- fake_client.addr = hdw->eeprom_addr;
- fake_client.adapter = &hdw->i2c_adap;
- tveeprom_hauppauge_analog(&fake_client,&tvdata,eeprom);
- }
+ if (!eeprom)
+ return -EINVAL;
+
+ tveeprom_hauppauge_analog(NULL, &tvdata, eeprom);
trace_eeprom("eeprom assumed v4l tveeprom module");
trace_eeprom("eeprom direct call results:");
--
2.9.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/4] [media] em28xx: reduce stack usage in probe functions
2017-02-02 14:53 [PATCH 1/4] [media] pvrusb2: reduce stack usage pvr2_eeprom_analyze() Arnd Bergmann
@ 2017-02-02 14:53 ` Arnd Bergmann
2017-02-13 14:00 ` Hans Verkuil
2017-02-02 14:53 ` [PATCH 3/4] [media] cx231xx-i2c: reduce stack size in bus scan Arnd Bergmann
2017-02-02 14:53 ` [PATCH 4/4] [media] mxl111sf: reduce stack usage in init function Arnd Bergmann
2 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2017-02-02 14:53 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Arnd Bergmann, linux-media, linux-kernel
The two i2c probe functions use a lot of stack since they put
an i2c_client structure in a local variable:
drivers/media/usb/em28xx/em28xx-camera.c: In function 'em28xx_probe_sensor_micron':
drivers/media/usb/em28xx/em28xx-camera.c:205:1: error: the frame size of 1256 bytes is larger than 1152 bytes [-Werror=frame-larger-than=]
drivers/media/usb/em28xx/em28xx-camera.c: In function 'em28xx_probe_sensor_omnivision':
drivers/media/usb/em28xx/em28xx-camera.c:317:1: error: the frame size of 1248 bytes is larger than 1152 bytes [-Werror=frame-larger-than=]
This cleans up both of the above by removing the need for those
structures, calling the lower-level i2c function directly.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/media/usb/em28xx/em28xx-camera.c | 87 ++++++++++++++++++--------------
1 file changed, 50 insertions(+), 37 deletions(-)
diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
index 89c890ba7dd6..e64940f95a91 100644
--- a/drivers/media/usb/em28xx/em28xx-camera.c
+++ b/drivers/media/usb/em28xx/em28xx-camera.c
@@ -99,6 +99,25 @@ static int em28xx_initialize_mt9m001(struct em28xx *dev)
return 0;
}
+/* NOTE: i2c_smbus_read_word_data() doesn't work with BE data */
+static int em28xx_i2c_read_chip_id(struct em28xx *dev, u16 addr, u8 reg, void *buf)
+{
+ struct i2c_client *client = &dev->i2c_client[dev->def_i2c_bus];
+ struct i2c_msg msg[2];
+
+ msg[0].addr = addr;
+ msg[0].flags = client->flags & I2C_M_TEN;
+ msg[0].len = 1;
+ msg[0].buf = ®
+ msg[1].addr = addr;
+ msg[1].flags = client->flags & I2C_M_TEN;
+ msg[1].flags |= I2C_M_RD;
+ msg[1].len = 2;
+ msg[1].buf = buf;
+
+ return i2c_transfer(client->adapter, msg, 2);
+}
+
/*
* Probes Micron sensors with 8 bit address and 16 bit register width
*/
@@ -106,48 +125,29 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
{
int ret, i;
char *name;
- u8 reg;
__be16 id_be;
+ u16 addr;
u16 id;
- struct i2c_client 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];
- /* NOTE: i2c_smbus_read_word_data() doesn't work with BE data */
+ addr = micron_sensor_addrs[i];
/* Read chip ID from register 0x00 */
- reg = 0x00;
- ret = i2c_master_send(&client, ®, 1);
+ ret = em28xx_i2c_read_chip_id(dev, addr, 0x00, &id_be);
if (ret < 0) {
if (ret != -ENXIO)
dev_err(&dev->intf->dev,
"couldn't read from i2c device 0x%02x: error %i\n",
- client.addr << 1, ret);
- continue;
- }
- ret = i2c_master_recv(&client, (u8 *)&id_be, 2);
- if (ret < 0) {
- dev_err(&dev->intf->dev,
- "couldn't read from i2c device 0x%02x: error %i\n",
- client.addr << 1, ret);
+ 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 = em28xx_i2c_read_chip_id(dev, addr, 0xff, &id_be);
if (ret < 0) {
dev_err(&dev->intf->dev,
"couldn't read from i2c device 0x%02x: error %i\n",
- client.addr << 1, ret);
- continue;
- }
- ret = i2c_master_recv(&client, (u8 *)&id_be, 2);
- if (ret < 0) {
- dev_err(&dev->intf->dev,
- "couldn't read from i2c device 0x%02x: error %i\n",
- client.addr << 1, ret);
+ addr << 1, ret);
continue;
}
/* Validate chip ID to be sure we have a Micron device */
@@ -197,13 +197,26 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
dev_info(&dev->intf->dev,
"sensor %s detected\n", name);
- dev->i2c_client[dev->def_i2c_bus].addr = client.addr;
+ dev->i2c_client[dev->def_i2c_bus].addr = addr;
return 0;
}
return -ENODEV;
}
+/* like i2c_smbus_read_byte_data, but allows passing an addr */
+static int em28xx_smbus_read_byte(struct em28xx *dev, u16 addr, u8 command)
+{
+ struct i2c_client *client = &dev->i2c_client[dev->def_i2c_bus];
+ union i2c_smbus_data data;
+ int status;
+
+ status = i2c_smbus_xfer(client->adapter, addr, client->flags,
+ I2C_SMBUS_READ, command,
+ I2C_SMBUS_BYTE_DATA, &data);
+ return (status < 0) ? status : data.byte;
+}
+
/*
* Probes Omnivision sensors with 8 bit address and register width
*/
@@ -212,31 +225,31 @@ static int em28xx_probe_sensor_omnivision(struct em28xx *dev)
int ret, i;
char *name;
u8 reg;
+ u16 addr;
u16 id;
- struct i2c_client 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];
+ addr = omnivision_sensor_addrs[i];
/* Read manufacturer ID from registers 0x1c-0x1d (BE) */
reg = 0x1c;
- ret = i2c_smbus_read_byte_data(&client, reg);
+ ret = em28xx_smbus_read_byte(dev, addr, reg);
if (ret < 0) {
if (ret != -ENXIO)
dev_err(&dev->intf->dev,
"couldn't read from i2c device 0x%02x: error %i\n",
- client.addr << 1, ret);
+ addr << 1, ret);
continue;
}
id = ret << 8;
reg = 0x1d;
- ret = i2c_smbus_read_byte_data(&client, reg);
+ ret = em28xx_smbus_read_byte(dev, addr, reg);
if (ret < 0) {
dev_err(&dev->intf->dev,
"couldn't read from i2c device 0x%02x: error %i\n",
- client.addr << 1, ret);
+ addr << 1, ret);
continue;
}
id += ret;
@@ -245,20 +258,20 @@ 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 = em28xx_smbus_read_byte(dev, addr, reg);
if (ret < 0) {
dev_err(&dev->intf->dev,
"couldn't read from i2c device 0x%02x: error %i\n",
- client.addr << 1, ret);
+ addr << 1, ret);
continue;
}
id = ret << 8;
reg = 0x0b;
- ret = i2c_smbus_read_byte_data(&client, reg);
+ ret = em28xx_smbus_read_byte(dev, addr, reg);
if (ret < 0) {
dev_err(&dev->intf->dev,
"couldn't read from i2c device 0x%02x: error %i\n",
- client.addr << 1, ret);
+ addr << 1, ret);
continue;
}
id += ret;
@@ -309,7 +322,7 @@ static int em28xx_probe_sensor_omnivision(struct em28xx *dev)
dev_info(&dev->intf->dev,
"sensor %s detected\n", name);
- dev->i2c_client[dev->def_i2c_bus].addr = client.addr;
+ dev->i2c_client[dev->def_i2c_bus].addr = addr;
return 0;
}
--
2.9.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/4] [media] cx231xx-i2c: reduce stack size in bus scan
2017-02-02 14:53 [PATCH 1/4] [media] pvrusb2: reduce stack usage pvr2_eeprom_analyze() Arnd Bergmann
2017-02-02 14:53 ` [PATCH 2/4] [media] em28xx: reduce stack usage in probe functions Arnd Bergmann
@ 2017-02-02 14:53 ` Arnd Bergmann
2017-02-02 14:53 ` [PATCH 4/4] [media] mxl111sf: reduce stack usage in init function Arnd Bergmann
2 siblings, 0 replies; 6+ messages in thread
From: Arnd Bergmann @ 2017-02-02 14:53 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Arnd Bergmann, Peter Rosin, Wolfram Sang, linux-media, linux-kernel
The cx231xx_do_i2c_scan function needs a lot of stack because
it puts an i2c_client structure on it:
drivers/media/usb/cx231xx/cx231xx-i2c.c: In function 'cx231xx_do_i2c_scan':
drivers/media/usb/cx231xx/cx231xx-i2c.c:518:1: error: the frame size of 1248 bytes is larger than 1152 bytes [-Werror=frame-larger-than=]
This changes it to call i2c_transfer() directly instead, avoiding the
need for the structure.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/media/usb/cx231xx/cx231xx-i2c.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/media/usb/cx231xx/cx231xx-i2c.c b/drivers/media/usb/cx231xx/cx231xx-i2c.c
index 35e9acfe63d3..24e23a06d8c6 100644
--- a/drivers/media/usb/cx231xx/cx231xx-i2c.c
+++ b/drivers/media/usb/cx231xx/cx231xx-i2c.c
@@ -491,20 +491,24 @@ void cx231xx_do_i2c_scan(struct cx231xx *dev, int i2c_port)
{
unsigned char buf;
int i, rc;
- struct i2c_client client;
+ struct i2c_adapter *adap;
+ struct i2c_msg msg = {
+ .flags = I2C_M_RD,
+ .len = 1,
+ .buf = &buf,
+ };
if (!i2c_scan)
return;
/* Don't generate I2C errors during scan */
dev->i2c_scan_running = true;
-
- memset(&client, 0, sizeof(client));
- client.adapter = cx231xx_get_i2c_adap(dev, i2c_port);
+ adap = cx231xx_get_i2c_adap(dev, i2c_port);
for (i = 0; i < 128; i++) {
- client.addr = i;
- rc = i2c_master_recv(&client, &buf, 0);
+ msg.addr = i;
+ rc = i2c_transfer(adap, &msg, 1);
+
if (rc < 0)
continue;
dev_info(dev->dev,
--
2.9.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 4/4] [media] mxl111sf: reduce stack usage in init function
2017-02-02 14:53 [PATCH 1/4] [media] pvrusb2: reduce stack usage pvr2_eeprom_analyze() Arnd Bergmann
2017-02-02 14:53 ` [PATCH 2/4] [media] em28xx: reduce stack usage in probe functions Arnd Bergmann
2017-02-02 14:53 ` [PATCH 3/4] [media] cx231xx-i2c: reduce stack size in bus scan Arnd Bergmann
@ 2017-02-02 14:53 ` Arnd Bergmann
2 siblings, 0 replies; 6+ messages in thread
From: Arnd Bergmann @ 2017-02-02 14:53 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Arnd Bergmann, Michael Krufky, linux-media, linux-kernel
mxl111sf uses a lot of kernel stack memory as it puts an i2c_client
structure on the stack:
drivers/media/usb/dvb-usb-v2/mxl111sf.c: In function 'mxl111sf_init':
drivers/media/usb/dvb-usb-v2/mxl111sf.c:953:1: error: the frame size of 1248 bytes is larger than 1152 bytes [-Werror=frame-larger-than=]
We can avoid doing this by open-coding the call to i2c_transfer()
instead of calling tveeprom_read(), and not passing an i2c_client
pointer to tveeprom_hauppauge_analog(), which would ignore that
anyway.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/media/usb/dvb-usb-v2/mxl111sf.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/media/usb/dvb-usb-v2/mxl111sf.c b/drivers/media/usb/dvb-usb-v2/mxl111sf.c
index 80c635980526..60bc5cc9a483 100644
--- a/drivers/media/usb/dvb-usb-v2/mxl111sf.c
+++ b/drivers/media/usb/dvb-usb-v2/mxl111sf.c
@@ -919,7 +919,12 @@ static int mxl111sf_init(struct dvb_usb_device *d)
struct mxl111sf_state *state = d_to_priv(d);
int ret;
static u8 eeprom[256];
- struct i2c_client c;
+ u8 reg = 0;
+ struct i2c_msg msg[2] = {
+ { .addr = 0xa0 >> 1, .len = 1, .buf = ® },
+ { .addr = 0xa0 >> 1, .flags = I2C_M_RD,
+ .len = sizeof(eeprom), .buf = eeprom },
+ };
ret = get_chip_info(state);
if (mxl_fail(ret))
@@ -930,13 +935,10 @@ static int mxl111sf_init(struct dvb_usb_device *d)
if (state->chip_rev > MXL111SF_V6)
mxl111sf_config_pin_mux_modes(state, PIN_MUX_TS_SPI_IN_MODE_1);
- c.adapter = &d->i2c_adap;
- c.addr = 0xa0 >> 1;
-
- ret = tveeprom_read(&c, eeprom, sizeof(eeprom));
+ ret = i2c_transfer(&d->i2c_adap, msg, 2);
if (mxl_fail(ret))
return 0;
- tveeprom_hauppauge_analog(&c, &state->tv, (0x84 == eeprom[0xa0]) ?
+ tveeprom_hauppauge_analog(NULL, &state->tv, (0x84 == eeprom[0xa0]) ?
eeprom + 0xa0 : eeprom + 0x80);
#if 0
switch (state->tv.model) {
--
2.9.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/4] [media] em28xx: reduce stack usage in probe functions
2017-02-02 14:53 ` [PATCH 2/4] [media] em28xx: reduce stack usage in probe functions Arnd Bergmann
@ 2017-02-13 14:00 ` Hans Verkuil
2017-02-16 20:13 ` Frank Schäfer
0 siblings, 1 reply; 6+ messages in thread
From: Hans Verkuil @ 2017-02-13 14:00 UTC (permalink / raw)
To: Arnd Bergmann, Mauro Carvalho Chehab
Cc: linux-media, linux-kernel, Frank Schäfer
Hi Arnd,
I'll take the others of this patch series, but will postpone this one until it has
been tested.
I've asked Frank to see if he can test it, if not, then it will have to wait until
March when I have access to an omnivision-em28xx device.
Regards,
Hans
On 02/02/2017 03:53 PM, Arnd Bergmann wrote:
> The two i2c probe functions use a lot of stack since they put
> an i2c_client structure in a local variable:
>
> drivers/media/usb/em28xx/em28xx-camera.c: In function 'em28xx_probe_sensor_micron':
> drivers/media/usb/em28xx/em28xx-camera.c:205:1: error: the frame size of 1256 bytes is larger than 1152 bytes [-Werror=frame-larger-than=]
> drivers/media/usb/em28xx/em28xx-camera.c: In function 'em28xx_probe_sensor_omnivision':
> drivers/media/usb/em28xx/em28xx-camera.c:317:1: error: the frame size of 1248 bytes is larger than 1152 bytes [-Werror=frame-larger-than=]
>
> This cleans up both of the above by removing the need for those
> structures, calling the lower-level i2c function directly.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/media/usb/em28xx/em28xx-camera.c | 87 ++++++++++++++++++--------------
> 1 file changed, 50 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
> index 89c890ba7dd6..e64940f95a91 100644
> --- a/drivers/media/usb/em28xx/em28xx-camera.c
> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
> @@ -99,6 +99,25 @@ static int em28xx_initialize_mt9m001(struct em28xx *dev)
> return 0;
> }
>
> +/* NOTE: i2c_smbus_read_word_data() doesn't work with BE data */
> +static int em28xx_i2c_read_chip_id(struct em28xx *dev, u16 addr, u8 reg, void *buf)
> +{
> + struct i2c_client *client = &dev->i2c_client[dev->def_i2c_bus];
> + struct i2c_msg msg[2];
> +
> + msg[0].addr = addr;
> + msg[0].flags = client->flags & I2C_M_TEN;
> + msg[0].len = 1;
> + msg[0].buf = ®
> + msg[1].addr = addr;
> + msg[1].flags = client->flags & I2C_M_TEN;
> + msg[1].flags |= I2C_M_RD;
> + msg[1].len = 2;
> + msg[1].buf = buf;
> +
> + return i2c_transfer(client->adapter, msg, 2);
> +}
> +
> /*
> * Probes Micron sensors with 8 bit address and 16 bit register width
> */
> @@ -106,48 +125,29 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
> {
> int ret, i;
> char *name;
> - u8 reg;
> __be16 id_be;
> + u16 addr;
> u16 id;
>
> - struct i2c_client 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];
> - /* NOTE: i2c_smbus_read_word_data() doesn't work with BE data */
> + addr = micron_sensor_addrs[i];
> /* Read chip ID from register 0x00 */
> - reg = 0x00;
> - ret = i2c_master_send(&client, ®, 1);
> + ret = em28xx_i2c_read_chip_id(dev, addr, 0x00, &id_be);
> if (ret < 0) {
> if (ret != -ENXIO)
> dev_err(&dev->intf->dev,
> "couldn't read from i2c device 0x%02x: error %i\n",
> - client.addr << 1, ret);
> - continue;
> - }
> - ret = i2c_master_recv(&client, (u8 *)&id_be, 2);
> - if (ret < 0) {
> - dev_err(&dev->intf->dev,
> - "couldn't read from i2c device 0x%02x: error %i\n",
> - client.addr << 1, ret);
> + 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 = em28xx_i2c_read_chip_id(dev, addr, 0xff, &id_be);
> if (ret < 0) {
> dev_err(&dev->intf->dev,
> "couldn't read from i2c device 0x%02x: error %i\n",
> - client.addr << 1, ret);
> - continue;
> - }
> - ret = i2c_master_recv(&client, (u8 *)&id_be, 2);
> - if (ret < 0) {
> - dev_err(&dev->intf->dev,
> - "couldn't read from i2c device 0x%02x: error %i\n",
> - client.addr << 1, ret);
> + addr << 1, ret);
> continue;
> }
> /* Validate chip ID to be sure we have a Micron device */
> @@ -197,13 +197,26 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
> dev_info(&dev->intf->dev,
> "sensor %s detected\n", name);
>
> - dev->i2c_client[dev->def_i2c_bus].addr = client.addr;
> + dev->i2c_client[dev->def_i2c_bus].addr = addr;
> return 0;
> }
>
> return -ENODEV;
> }
>
> +/* like i2c_smbus_read_byte_data, but allows passing an addr */
> +static int em28xx_smbus_read_byte(struct em28xx *dev, u16 addr, u8 command)
> +{
> + struct i2c_client *client = &dev->i2c_client[dev->def_i2c_bus];
> + union i2c_smbus_data data;
> + int status;
> +
> + status = i2c_smbus_xfer(client->adapter, addr, client->flags,
> + I2C_SMBUS_READ, command,
> + I2C_SMBUS_BYTE_DATA, &data);
> + return (status < 0) ? status : data.byte;
> +}
> +
> /*
> * Probes Omnivision sensors with 8 bit address and register width
> */
> @@ -212,31 +225,31 @@ static int em28xx_probe_sensor_omnivision(struct em28xx *dev)
> int ret, i;
> char *name;
> u8 reg;
> + u16 addr;
> u16 id;
> - struct i2c_client 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];
> + addr = omnivision_sensor_addrs[i];
> /* Read manufacturer ID from registers 0x1c-0x1d (BE) */
> reg = 0x1c;
> - ret = i2c_smbus_read_byte_data(&client, reg);
> + ret = em28xx_smbus_read_byte(dev, addr, reg);
> if (ret < 0) {
> if (ret != -ENXIO)
> dev_err(&dev->intf->dev,
> "couldn't read from i2c device 0x%02x: error %i\n",
> - client.addr << 1, ret);
> + addr << 1, ret);
> continue;
> }
> id = ret << 8;
> reg = 0x1d;
> - ret = i2c_smbus_read_byte_data(&client, reg);
> + ret = em28xx_smbus_read_byte(dev, addr, reg);
> if (ret < 0) {
> dev_err(&dev->intf->dev,
> "couldn't read from i2c device 0x%02x: error %i\n",
> - client.addr << 1, ret);
> + addr << 1, ret);
> continue;
> }
> id += ret;
> @@ -245,20 +258,20 @@ 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 = em28xx_smbus_read_byte(dev, addr, reg);
> if (ret < 0) {
> dev_err(&dev->intf->dev,
> "couldn't read from i2c device 0x%02x: error %i\n",
> - client.addr << 1, ret);
> + addr << 1, ret);
> continue;
> }
> id = ret << 8;
> reg = 0x0b;
> - ret = i2c_smbus_read_byte_data(&client, reg);
> + ret = em28xx_smbus_read_byte(dev, addr, reg);
> if (ret < 0) {
> dev_err(&dev->intf->dev,
> "couldn't read from i2c device 0x%02x: error %i\n",
> - client.addr << 1, ret);
> + addr << 1, ret);
> continue;
> }
> id += ret;
> @@ -309,7 +322,7 @@ static int em28xx_probe_sensor_omnivision(struct em28xx *dev)
> dev_info(&dev->intf->dev,
> "sensor %s detected\n", name);
>
> - dev->i2c_client[dev->def_i2c_bus].addr = client.addr;
> + dev->i2c_client[dev->def_i2c_bus].addr = addr;
> return 0;
> }
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/4] [media] em28xx: reduce stack usage in probe functions
2017-02-13 14:00 ` Hans Verkuil
@ 2017-02-16 20:13 ` Frank Schäfer
0 siblings, 0 replies; 6+ messages in thread
From: Frank Schäfer @ 2017-02-16 20:13 UTC (permalink / raw)
To: Hans Verkuil, Arnd Bergmann, Mauro Carvalho Chehab; +Cc: linux-media
Hi Arnd,
Am 13.02.2017 um 15:00 schrieb Hans Verkuil:
> Hi Arnd,
>
> I'll take the others of this patch series, but will postpone this one until it has
> been tested.
>
> I've asked Frank to see if he can test it, if not, then it will have to wait until
> March when I have access to an omnivision-em28xx device.
>
> Regards,
>
> Hans
>
> On 02/02/2017 03:53 PM, Arnd Bergmann wrote:
>> The two i2c probe functions use a lot of stack since they put
>> an i2c_client structure in a local variable:
>>
>> drivers/media/usb/em28xx/em28xx-camera.c: In function 'em28xx_probe_sensor_micron':
>> drivers/media/usb/em28xx/em28xx-camera.c:205:1: error: the frame size of 1256 bytes is larger than 1152 bytes [-Werror=frame-larger-than=]
>> drivers/media/usb/em28xx/em28xx-camera.c: In function 'em28xx_probe_sensor_omnivision':
>> drivers/media/usb/em28xx/em28xx-camera.c:317:1: error: the frame size of 1248 bytes is larger than 1152 bytes [-Werror=frame-larger-than=]
>>
>> This cleans up both of the above by removing the need for those
>> structures, calling the lower-level i2c function directly.
in the past, it was necessary to keep dev->i2c_client[dev->def_i2c_bus]
unmodified, otherwise bad things could have happened after sensor probing.
In the meantime many things have been refactored/fixed and this seems to
be no longer true.
So we can go the simple way and just use a pointer to
dev->i2c_client[dev->def_i2c_bus] instead.
But let me test that with another device this weekend to be 100% sure.
Regards,
Frank
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>> drivers/media/usb/em28xx/em28xx-camera.c | 87 ++++++++++++++++++--------------
>> 1 file changed, 50 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
>> index 89c890ba7dd6..e64940f95a91 100644
>> --- a/drivers/media/usb/em28xx/em28xx-camera.c
>> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
>> @@ -99,6 +99,25 @@ static int em28xx_initialize_mt9m001(struct em28xx *dev)
>> return 0;
>> }
>>
>> +/* NOTE: i2c_smbus_read_word_data() doesn't work with BE data */
>> +static int em28xx_i2c_read_chip_id(struct em28xx *dev, u16 addr, u8 reg, void *buf)
>> +{
>> + struct i2c_client *client = &dev->i2c_client[dev->def_i2c_bus];
>> + struct i2c_msg msg[2];
>> +
>> + msg[0].addr = addr;
>> + msg[0].flags = client->flags & I2C_M_TEN;
>> + msg[0].len = 1;
>> + msg[0].buf = ®
>> + msg[1].addr = addr;
>> + msg[1].flags = client->flags & I2C_M_TEN;
>> + msg[1].flags |= I2C_M_RD;
>> + msg[1].len = 2;
>> + msg[1].buf = buf;
>> +
>> + return i2c_transfer(client->adapter, msg, 2);
>> +}
>> +
>> /*
>> * Probes Micron sensors with 8 bit address and 16 bit register width
>> */
>> @@ -106,48 +125,29 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
>> {
>> int ret, i;
>> char *name;
>> - u8 reg;
>> __be16 id_be;
>> + u16 addr;
>> u16 id;
>>
>> - struct i2c_client 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];
>> - /* NOTE: i2c_smbus_read_word_data() doesn't work with BE data */
>> + addr = micron_sensor_addrs[i];
>> /* Read chip ID from register 0x00 */
>> - reg = 0x00;
>> - ret = i2c_master_send(&client, ®, 1);
>> + ret = em28xx_i2c_read_chip_id(dev, addr, 0x00, &id_be);
>> if (ret < 0) {
>> if (ret != -ENXIO)
>> dev_err(&dev->intf->dev,
>> "couldn't read from i2c device 0x%02x: error %i\n",
>> - client.addr << 1, ret);
>> - continue;
>> - }
>> - ret = i2c_master_recv(&client, (u8 *)&id_be, 2);
>> - if (ret < 0) {
>> - dev_err(&dev->intf->dev,
>> - "couldn't read from i2c device 0x%02x: error %i\n",
>> - client.addr << 1, ret);
>> + 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 = em28xx_i2c_read_chip_id(dev, addr, 0xff, &id_be);
>> if (ret < 0) {
>> dev_err(&dev->intf->dev,
>> "couldn't read from i2c device 0x%02x: error %i\n",
>> - client.addr << 1, ret);
>> - continue;
>> - }
>> - ret = i2c_master_recv(&client, (u8 *)&id_be, 2);
>> - if (ret < 0) {
>> - dev_err(&dev->intf->dev,
>> - "couldn't read from i2c device 0x%02x: error %i\n",
>> - client.addr << 1, ret);
>> + addr << 1, ret);
>> continue;
>> }
>> /* Validate chip ID to be sure we have a Micron device */
>> @@ -197,13 +197,26 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
>> dev_info(&dev->intf->dev,
>> "sensor %s detected\n", name);
>>
>> - dev->i2c_client[dev->def_i2c_bus].addr = client.addr;
>> + dev->i2c_client[dev->def_i2c_bus].addr = addr;
>> return 0;
>> }
>>
>> return -ENODEV;
>> }
>>
>> +/* like i2c_smbus_read_byte_data, but allows passing an addr */
>> +static int em28xx_smbus_read_byte(struct em28xx *dev, u16 addr, u8 command)
>> +{
>> + struct i2c_client *client = &dev->i2c_client[dev->def_i2c_bus];
>> + union i2c_smbus_data data;
>> + int status;
>> +
>> + status = i2c_smbus_xfer(client->adapter, addr, client->flags,
>> + I2C_SMBUS_READ, command,
>> + I2C_SMBUS_BYTE_DATA, &data);
>> + return (status < 0) ? status : data.byte;
>> +}
>> +
>> /*
>> * Probes Omnivision sensors with 8 bit address and register width
>> */
>> @@ -212,31 +225,31 @@ static int em28xx_probe_sensor_omnivision(struct em28xx *dev)
>> int ret, i;
>> char *name;
>> u8 reg;
>> + u16 addr;
>> u16 id;
>> - struct i2c_client 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];
>> + addr = omnivision_sensor_addrs[i];
>> /* Read manufacturer ID from registers 0x1c-0x1d (BE) */
>> reg = 0x1c;
>> - ret = i2c_smbus_read_byte_data(&client, reg);
>> + ret = em28xx_smbus_read_byte(dev, addr, reg);
>> if (ret < 0) {
>> if (ret != -ENXIO)
>> dev_err(&dev->intf->dev,
>> "couldn't read from i2c device 0x%02x: error %i\n",
>> - client.addr << 1, ret);
>> + addr << 1, ret);
>> continue;
>> }
>> id = ret << 8;
>> reg = 0x1d;
>> - ret = i2c_smbus_read_byte_data(&client, reg);
>> + ret = em28xx_smbus_read_byte(dev, addr, reg);
>> if (ret < 0) {
>> dev_err(&dev->intf->dev,
>> "couldn't read from i2c device 0x%02x: error %i\n",
>> - client.addr << 1, ret);
>> + addr << 1, ret);
>> continue;
>> }
>> id += ret;
>> @@ -245,20 +258,20 @@ 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 = em28xx_smbus_read_byte(dev, addr, reg);
>> if (ret < 0) {
>> dev_err(&dev->intf->dev,
>> "couldn't read from i2c device 0x%02x: error %i\n",
>> - client.addr << 1, ret);
>> + addr << 1, ret);
>> continue;
>> }
>> id = ret << 8;
>> reg = 0x0b;
>> - ret = i2c_smbus_read_byte_data(&client, reg);
>> + ret = em28xx_smbus_read_byte(dev, addr, reg);
>> if (ret < 0) {
>> dev_err(&dev->intf->dev,
>> "couldn't read from i2c device 0x%02x: error %i\n",
>> - client.addr << 1, ret);
>> + addr << 1, ret);
>> continue;
>> }
>> id += ret;
>> @@ -309,7 +322,7 @@ static int em28xx_probe_sensor_omnivision(struct em28xx *dev)
>> dev_info(&dev->intf->dev,
>> "sensor %s detected\n", name);
>>
>> - dev->i2c_client[dev->def_i2c_bus].addr = client.addr;
>> + dev->i2c_client[dev->def_i2c_bus].addr = addr;
>> return 0;
>> }
>>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-16 20:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-02 14:53 [PATCH 1/4] [media] pvrusb2: reduce stack usage pvr2_eeprom_analyze() Arnd Bergmann
2017-02-02 14:53 ` [PATCH 2/4] [media] em28xx: reduce stack usage in probe functions Arnd Bergmann
2017-02-13 14:00 ` Hans Verkuil
2017-02-16 20:13 ` Frank Schäfer
2017-02-02 14:53 ` [PATCH 3/4] [media] cx231xx-i2c: reduce stack size in bus scan Arnd Bergmann
2017-02-02 14:53 ` [PATCH 4/4] [media] mxl111sf: reduce stack usage in init function Arnd Bergmann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.