ir_probe allocated struct i2c_client on stack;
it's pretty big structure, so allocate it with kzalloc
make checkstack output without this patch:
x059d ir_probe [ir-kbd-i2c]: 1000
compile tested only
Signed-off-by: Marcin Slusarz <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Jean Delvare <[email protected]>
---
drivers/media/video/ir-kbd-i2c.c | 18 +++++++++++-------
1 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/media/video/ir-kbd-i2c.c b/drivers/media/video/ir-kbd-i2c.c
index 9851987..aec122f 100644
--- a/drivers/media/video/ir-kbd-i2c.c
+++ b/drivers/media/video/ir-kbd-i2c.c
@@ -510,9 +510,9 @@ static int ir_probe(struct i2c_adapter *adap)
static const int probe_cx88[] = { 0x18, 0x6b, 0x71, -1 };
static const int probe_cx23885[] = { 0x6b, -1 };
const int *probe = NULL;
- struct i2c_client c;
+ struct i2c_client *c;
unsigned char buf;
- int i,rc;
+ int i, rc;
switch (adap->id) {
case I2C_HW_B_BT848:
@@ -537,19 +537,23 @@ static int ir_probe(struct i2c_adapter *adap)
if (NULL == probe)
return 0;
- memset(&c,0,sizeof(c));
- c.adapter = adap;
+ c = kzalloc(sizeof(*c), GFP_KERNEL);
+ if (!c)
+ return -ENOMEM;
+
+ c->adapter = adap;
for (i = 0; -1 != probe[i]; i++) {
- c.addr = probe[i];
- rc = i2c_master_recv(&c,&buf,0);
+ c->addr = probe[i];
+ rc = i2c_master_recv(c, &buf, 0);
dprintk(1,"probe 0x%02x @ %s: %s\n",
probe[i], adap->name,
(0 == rc) ? "yes" : "no");
if (0 == rc) {
- ir_attach(adap,probe[i],0,0);
+ ir_attach(adap, probe[i], 0, 0);
break;
}
}
+ kfree(c);
return 0;
}
--
1.5.3.7
Hi Marcin,
On Mon, 25 Feb 2008 21:51:00 +0100, Marcin Slusarz wrote:
> ir_probe allocated struct i2c_client on stack;
> it's pretty big structure, so allocate it with kzalloc
>
> make checkstack output without this patch:
> x059d ir_probe [ir-kbd-i2c]: 1000
>
> compile tested only
>
> Signed-off-by: Marcin Slusarz <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Jean Delvare <[email protected]>
> ---
> drivers/media/video/ir-kbd-i2c.c | 18 +++++++++++-------
> 1 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/video/ir-kbd-i2c.c b/drivers/media/video/ir-kbd-i2c.c
> index 9851987..aec122f 100644
> --- a/drivers/media/video/ir-kbd-i2c.c
> +++ b/drivers/media/video/ir-kbd-i2c.c
> @@ -510,9 +510,9 @@ static int ir_probe(struct i2c_adapter *adap)
> static const int probe_cx88[] = { 0x18, 0x6b, 0x71, -1 };
> static const int probe_cx23885[] = { 0x6b, -1 };
> const int *probe = NULL;
> - struct i2c_client c;
> + struct i2c_client *c;
> unsigned char buf;
> - int i,rc;
> + int i, rc;
>
> switch (adap->id) {
> case I2C_HW_B_BT848:
> @@ -537,19 +537,23 @@ static int ir_probe(struct i2c_adapter *adap)
> if (NULL == probe)
> return 0;
>
> - memset(&c,0,sizeof(c));
> - c.adapter = adap;
> + c = kzalloc(sizeof(*c), GFP_KERNEL);
> + if (!c)
> + return -ENOMEM;
> +
> + c->adapter = adap;
> for (i = 0; -1 != probe[i]; i++) {
> - c.addr = probe[i];
> - rc = i2c_master_recv(&c,&buf,0);
> + c->addr = probe[i];
> + rc = i2c_master_recv(c, &buf, 0);
> dprintk(1,"probe 0x%02x @ %s: %s\n",
> probe[i], adap->name,
> (0 == rc) ? "yes" : "no");
> if (0 == rc) {
> - ir_attach(adap,probe[i],0,0);
> + ir_attach(adap, probe[i], 0, 0);
> break;
> }
> }
> + kfree(c);
> return 0;
> }
>
While this works, I'd rather change the code to call i2c_transfer()
instead of i2c_master_recv(). i2c_transfer() is meant exactly for this
case (no i2c_client at hand.) This solves the stack usage problem
without requiring a temporary memory allocation:
* * * * *
Limit stack usage in ir_probe by calling i2c_transfer, which doesn't
require a struct i2c_client, instead of i2c_master_recv which does.
Signed-off-by: Jean Delvare <[email protected]>
---
drivers/media/video/ir-kbd-i2c.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
--- linux-2.6.25-rc3.orig/drivers/media/video/ir-kbd-i2c.c 2008-02-26 11:35:51.000000000 +0100
+++ linux-2.6.25-rc3/drivers/media/video/ir-kbd-i2c.c 2008-02-26 11:44:54.000000000 +0100
@@ -510,8 +510,11 @@ static int ir_probe(struct i2c_adapter *
static const int probe_cx88[] = { 0x18, 0x6b, 0x71, -1 };
static const int probe_cx23885[] = { 0x6b, -1 };
const int *probe = NULL;
- struct i2c_client c;
- unsigned char buf;
+ struct i2c_msg msg = {
+ .flags = I2C_M_RD,
+ .len = 0,
+ .buf = NULL,
+ };
int i,rc;
switch (adap->id) {
@@ -537,15 +540,13 @@ static int ir_probe(struct i2c_adapter *
if (NULL == probe)
return 0;
- memset(&c,0,sizeof(c));
- c.adapter = adap;
for (i = 0; -1 != probe[i]; i++) {
- c.addr = probe[i];
- rc = i2c_master_recv(&c,&buf,0);
+ msg.addr = probe[i];
+ rc = i2c_transfer(adap, &msg, 1);
dprintk(1,"probe 0x%02x @ %s: %s\n",
probe[i], adap->name,
- (0 == rc) ? "yes" : "no");
- if (0 == rc) {
+ (1 == rc) ? "yes" : "no");
+ if (1 == rc) {
ir_attach(adap,probe[i],0,0);
break;
}
Built-tested, I've also tested loading the ir-kbd-i2c driver on an
unsupported cx88 adapter. Review and more testing welcome.
--
Jean Delvare
On Tue, Feb 26, 2008 at 01:32:22PM +0100, Jean Delvare wrote:
> Hi Marcin,
>
> On Mon, 25 Feb 2008 21:51:00 +0100, Marcin Slusarz wrote:
> > ir_probe allocated struct i2c_client on stack;
> > it's pretty big structure, so allocate it with kzalloc
> >
> > make checkstack output without this patch:
> > x059d ir_probe [ir-kbd-i2c]: 1000
> >
> > compile tested only
> >
> > Signed-off-by: Marcin Slusarz <[email protected]>
> > Cc: Mauro Carvalho Chehab <[email protected]>
> > Cc: Jean Delvare <[email protected]>
> > ---
> > drivers/media/video/ir-kbd-i2c.c | 18 +++++++++++-------
> > 1 files changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/media/video/ir-kbd-i2c.c b/drivers/media/video/ir-kbd-i2c.c
> > index 9851987..aec122f 100644
> > --- a/drivers/media/video/ir-kbd-i2c.c
> > +++ b/drivers/media/video/ir-kbd-i2c.c
> > @@ -510,9 +510,9 @@ static int ir_probe(struct i2c_adapter *adap)
> > static const int probe_cx88[] = { 0x18, 0x6b, 0x71, -1 };
> > static const int probe_cx23885[] = { 0x6b, -1 };
> > const int *probe = NULL;
> > - struct i2c_client c;
> > + struct i2c_client *c;
> > unsigned char buf;
> > - int i,rc;
> > + int i, rc;
> >
> > switch (adap->id) {
> > case I2C_HW_B_BT848:
> > @@ -537,19 +537,23 @@ static int ir_probe(struct i2c_adapter *adap)
> > if (NULL == probe)
> > return 0;
> >
> > - memset(&c,0,sizeof(c));
> > - c.adapter = adap;
> > + c = kzalloc(sizeof(*c), GFP_KERNEL);
> > + if (!c)
> > + return -ENOMEM;
> > +
> > + c->adapter = adap;
> > for (i = 0; -1 != probe[i]; i++) {
> > - c.addr = probe[i];
> > - rc = i2c_master_recv(&c,&buf,0);
> > + c->addr = probe[i];
> > + rc = i2c_master_recv(c, &buf, 0);
> > dprintk(1,"probe 0x%02x @ %s: %s\n",
> > probe[i], adap->name,
> > (0 == rc) ? "yes" : "no");
> > if (0 == rc) {
> > - ir_attach(adap,probe[i],0,0);
> > + ir_attach(adap, probe[i], 0, 0);
> > break;
> > }
> > }
> > + kfree(c);
> > return 0;
> > }
> >
>
> While this works, I'd rather change the code to call i2c_transfer()
> instead of i2c_master_recv(). i2c_transfer() is meant exactly for this
> case (no i2c_client at hand.) This solves the stack usage problem
> without requiring a temporary memory allocation:
>
> * * * * *
>
> Limit stack usage in ir_probe by calling i2c_transfer, which doesn't
> require a struct i2c_client, instead of i2c_master_recv which does.
>
> Signed-off-by: Jean Delvare <[email protected]>
> ---
> drivers/media/video/ir-kbd-i2c.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> --- linux-2.6.25-rc3.orig/drivers/media/video/ir-kbd-i2c.c 2008-02-26 11:35:51.000000000 +0100
> +++ linux-2.6.25-rc3/drivers/media/video/ir-kbd-i2c.c 2008-02-26 11:44:54.000000000 +0100
> @@ -510,8 +510,11 @@ static int ir_probe(struct i2c_adapter *
> static const int probe_cx88[] = { 0x18, 0x6b, 0x71, -1 };
> static const int probe_cx23885[] = { 0x6b, -1 };
> const int *probe = NULL;
> - struct i2c_client c;
> - unsigned char buf;
> + struct i2c_msg msg = {
> + .flags = I2C_M_RD,
> + .len = 0,
> + .buf = NULL,
> + };
> int i,rc;
>
> switch (adap->id) {
> @@ -537,15 +540,13 @@ static int ir_probe(struct i2c_adapter *
> if (NULL == probe)
> return 0;
>
> - memset(&c,0,sizeof(c));
> - c.adapter = adap;
> for (i = 0; -1 != probe[i]; i++) {
> - c.addr = probe[i];
> - rc = i2c_master_recv(&c,&buf,0);
> + msg.addr = probe[i];
> + rc = i2c_transfer(adap, &msg, 1);
> dprintk(1,"probe 0x%02x @ %s: %s\n",
> probe[i], adap->name,
> - (0 == rc) ? "yes" : "no");
> - if (0 == rc) {
> + (1 == rc) ? "yes" : "no");
> + if (1 == rc) {
> ir_attach(adap,probe[i],0,0);
> break;
> }
>
>
> Built-tested, I've also tested loading the ir-kbd-i2c driver on an
> unsupported cx88 adapter. Review and more testing welcome.
I know nothing about I2C API, so I can't comment on correctness of this patch ;)
Do you have an idea (or patch :D) how to solve this:
0x00000234 v4l_compat_translate_ioctl [v4l1-compat]: 1376
? That's on top of my make checkstack output
Marcin
Hi Marcin,
On Tue, 26 Feb 2008 22:03:16 +0100, Marcin Slusarz wrote:
> Do you have an idea (or patch :D) how to solve this:
> 0x00000234 v4l_compat_translate_ioctl [v4l1-compat]: 1376
> ? That's on top of my make checkstack output
Random ideas (but I am in no way a specialist of this exercise):
* You could try moving the structures to the blocks where they are used,
in the case a given structure is used for only one ioctl. I'm not too
sure how gcc handles local variables declared inside blocks with
regards to stack reservation though. I thought it would work but my
experiments today seem to suggest it doesn't.
* You can move the handling of some ioctls to dedicated functions, just
like I did in i2c-dev:
http://lists.lm-sensors.org/pipermail/i2c/2008-February/003010.html
However there is a risk that gcc will inline these functions (that's
what happened to me...) Not sure how to prevent gcc from inlining.
* You can allocate the structures dynamically, as you originally wanted
to do for ir-kbd-i2c. However this has a performance penalty and will
fragment the memory, so it's not ideal.
* If each ioctl uses only one of the structures, you may define a union
of all the structures. The size of the union will be the size of the
biggest structure, so you save a lot of space on the stack.
--
Jean Delvare
On Tue, Feb 26, 2008 at 11:23:20PM +0100, Jean Delvare wrote:
> Hi Marcin,
Hi
> On Tue, 26 Feb 2008 22:03:16 +0100, Marcin Slusarz wrote:
> > Do you have an idea (or patch :D) how to solve this:
> > 0x00000234 v4l_compat_translate_ioctl [v4l1-compat]: 1376
> > ? That's on top of my make checkstack output
>
> Random ideas (but I am in no way a specialist of this exercise):
>
> * You could try moving the structures to the blocks where they are used,
> in the case a given structure is used for only one ioctl. I'm not too
> sure how gcc handles local variables declared inside blocks with
> regards to stack reservation though. I thought it would work but my
> experiments today seem to suggest it doesn't.
That won't work. Variables at beginning of function take only ~600 bytes,
so the rest must be from inner blocks and inlines (probably).
> * You can move the handling of some ioctls to dedicated functions, just
> like I did in i2c-dev:
> http://lists.lm-sensors.org/pipermail/i2c/2008-February/003010.html
> However there is a risk that gcc will inline these functions (that's
> what happened to me...) Not sure how to prevent gcc from inlining.
There's "noinline" attribute in linux/compiler.h (compiler-gcc.h actually)
for these situations.
> * You can allocate the structures dynamically, as you originally wanted
> to do for ir-kbd-i2c. However this has a performance penalty and will
> fragment the memory, so it's not ideal.
>
> * If each ioctl uses only one of the structures, you may define a union
> of all the structures. The size of the union will be the size of the
> biggest structure, so you save a lot of space on the stack.
Nice idea.
I'll try 2nd and 4th approaches.
Marcin Slusarz
On Wed, 27 Feb 2008 11:23:26 +0100
Marcin Slusarz <[email protected]> wrote:
> On Tue, Feb 26, 2008 at 11:23:20PM +0100, Jean Delvare wrote:
> > Hi Marcin,
> Hi
>
> > On Tue, 26 Feb 2008 22:03:16 +0100, Marcin Slusarz wrote:
> > > Do you have an idea (or patch :D) how to solve this:
> > > 0x00000234 v4l_compat_translate_ioctl [v4l1-compat]: 1376
> > > ? That's on top of my make checkstack output
> >
> > Random ideas (but I am in no way a specialist of this exercise):
> >
> > * You could try moving the structures to the blocks where they are used,
> > in the case a given structure is used for only one ioctl. I'm not too
> > sure how gcc handles local variables declared inside blocks with
> > regards to stack reservation though. I thought it would work but my
> > experiments today seem to suggest it doesn't.
> That won't work. Variables at beginning of function take only ~600 bytes,
> so the rest must be from inner blocks and inlines (probably).
>
> > * You can move the handling of some ioctls to dedicated functions, just
> > like I did in i2c-dev:
> > http://lists.lm-sensors.org/pipermail/i2c/2008-February/003010.html
> > However there is a risk that gcc will inline these functions (that's
> > what happened to me...) Not sure how to prevent gcc from inlining.
> There's "noinline" attribute in linux/compiler.h (compiler-gcc.h actually)
> for these situations.
>
> > * You can allocate the structures dynamically, as you originally wanted
> > to do for ir-kbd-i2c. However this has a performance penalty and will
> > fragment the memory, so it's not ideal.
> >
> > * If each ioctl uses only one of the structures, you may define a union
> > of all the structures. The size of the union will be the size of the
> > biggest structure, so you save a lot of space on the stack.
> Nice idea.
>
> I'll try 2nd and 4th approaches.
The union will probably solve. This function is very complex, since it needs to
deal with almost all v4l1 v4l2 ioctls (about 80-90). Splitting into small
functions might help, but probably, gcc will create the functions as inline.
>
> Marcin Slusarz
Cheers,
Mauro
On Wed, 27 Feb 2008 11:23:26 +0100, Marcin Slusarz wrote:
> On Tue, Feb 26, 2008 at 11:23:20PM +0100, Jean Delvare wrote:
> > * You can move the handling of some ioctls to dedicated functions, just
> > like I did in i2c-dev:
> > http://lists.lm-sensors.org/pipermail/i2c/2008-February/003010.html
> > However there is a risk that gcc will inline these functions (that's
> > what happened to me...) Not sure how to prevent gcc from inlining.
>
> There's "noinline" attribute in linux/compiler.h (compiler-gcc.h actually)
> for these situations.
I didn't know about noinline, thanks for the tip. It works as expected,
however in the case of i2cdev_ioctl I'm not sure if it's worth it.
Without it, I have:
0x02ae i2cdev_ioctl [i2c-dev]: 192
With noinline, I have:
0x02ac i2cdev_ioctl_smbus [i2c-dev]: 112
0x05bc i2cdev_ioctl_rdrw [i2c-dev]: 112
0x087e i2cdev_ioctl [i2c-dev]: 32
32 + 112 = 144, 192 - 144 = only 48 bytes saved on the stack, at the
price of some performance loss. OTOH this makes the binary slightly
smaller (not sure why), which is always nice:
add/remove: 2/0 grow/shrink: 0/1 up/down: 1121/-1182 (-61)
function old new delta
i2cdev_ioctl_rdrw - 696 +696
i2cdev_ioctl_smbus - 425 +425
i2cdev_ioctl 1482 300 -1182
--
Jean Delvare