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

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(-)

drivers/media/video/ir-kbd-i2c.c +9 -6

index 9851987..aec122f 100644
@@ -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");

Hi Marcin,

Quoted 50 lines · 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 <[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(-)

drivers/media/video/ir-kbd-i2c.c +9 -6

index 9851987..aec122f 100644
@@ -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");

drivers/media/video/ir-kbd-i2c.c static int ir_probe(struct i2c_adapter *adap) · line 559

Quoted 10 lines

drivers/media/video/ir-kbd-i2c.c +2 -1

 		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(-)

drivers/media/video/ir-kbd-i2c.c +9 -8

@@ -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.

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

Hi Marcin,

Quoted 3 lines

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.

Quoted 1 line · Jean Delvare

Hi Marcin,

Hi

Quoted 5 lines · Marcin Slusarz

Quoted 3 lines

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

Quoted 7 lines

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).

Quoted 5 lines

* 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.

Quoted 4 lines

* 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.

Quoted 3 lines

* 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

Quoted 5 lines · Jean Delvare

* 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.