[PATCH] video: limit stack usage of ir-kbd-i2c.c

[PATCH] video: limit stack usage of ir-kbd-i2c.c

[Marcin Slusarz]

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 <marcin.slusarz@gmail.com>
Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
Cc: Jean Delvare <khali@linux-fr.org>
---
 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

Re: [PATCH] video: limit stack usage of ir-kbd-i2c.c

[Jean Delvare]

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 <marcin.slusarz@gmail.com>
> Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
> Cc: Jean Delvare <khali@linux-fr.org>
> ---
>  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 <khali@linux-fr.org>
---
 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

Re: [PATCH] video: limit stack usage of ir-kbd-i2c.c

[Marcin Slusarz]

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 <marcin.slusarz@gmail.com>
> > Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
> > Cc: Jean Delvare <khali@linux-fr.org>
> > ---
> >  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 <khali@linux-fr.org>
> ---
>  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

Re: [PATCH] video: limit stack usage of ir-kbd-i2c.c

[Jean Delvare]

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

Re: [PATCH] video: limit stack usage of ir-kbd-i2c.c

[Marcin Slusarz]

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

Re: [PATCH] video: limit stack usage of ir-kbd-i2c.c

[Mauro Carvalho Chehab]

On Wed, 27 Feb 2008 11:23:26 +0100
Marcin Slusarz <marcin.slusarz@gmail.com> 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

Re: [PATCH] video: limit stack usage of ir-kbd-i2c.c

[Jean Delvare]

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