Ooops in tda827x.c

linux-dvb Sigmund Augdal 2 participants 7 messages Jun 3, 2008 – Jun 5, 2008 Media (V4L/DVB)

Share & tools

Sigmund Augdal

author

2008-06-03 23:22 UTC #0 lore raw

changeset 49ba58715fe0 (7393) introduces an ooops in tda827x.c in tda827xa_lna_gain. The initialization of the "msg" variable accesses priv->cfg before the NULL check causing an oops when it is in fact NULL.

Best regards

Sigmund Augdal

_______________________________________________
linux-dvb mailing list
[email protected]
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Sigmund Augdal

author

2008-06-04 13:06 UTC #1 lore raw

Attached patch fixes the problem.

Best regards

Sigmund Augdal

Quoted 7 lines · Sigmund Augdal

changeset 49ba58715fe0 (7393) introduces an ooops in tda827x.c in tda827xa_lna_gain. The initialization of the "msg" variable accesses priv->cfg before the NULL check causing an oops when it is in fact NULL.
Best regards

Quoted 8 lines

Sigmund Augdal
_______________________________________________
linux-dvb mailing list
[email protected]
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

linux/drivers/media/common/tuners/tda827x.c +2 -1

diff -r 6541620a09b7 linux/drivers/media/common/tuners/tda827x.c
@@ -419,13 +419,14 @@
 	unsigned char buf[] = {0x22, 0x01};
 	int arg;
 	int gp_func;
-	struct i2c_msg msg = { .addr = priv->cfg->switch_addr, .flags = 0,
+	struct i2c_msg msg = { .flags = 0,
 			       .buf = buf, .len = sizeof(buf) };
 
 	if (NULL == priv->cfg) {
 		dprintk("tda827x_config not defined, cannot set LNA gain!\n");
 		return;
 	}
+	msg.addr = priv->cfg->switch_addr;
 	if (priv->cfg->config) {
 		if (high)
 			dprintk("setting LNA to high gain\n");

_______________________________________________
linux-dvb mailing list
[email protected]
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Michael Krufky

2008-06-04 13:39 UTC #2 lore raw

Quoted 9 lines · Sigmund Augdal

Quoted 8 lines
changeset 49ba58715fe0 (7393) introduces an ooops in tda827x.c in tda827xa_lna_gain. The initialization of the "msg" variable accesses priv->cfg before the NULL check causing an oops when it is in fact NULL.
Best regards
Sigmund Augdal

2008/6/4 Sigmund Augdal <[email protected]>:

Quoted 6 lines

Attached patch fixes the problem.
Best regards
Sigmund Augdal

Sigmund,

The driver was only able to get into this function without priv->cfg being defined, because m920x passes in NULL as cfg.

In my opinion, this is flawed by design, and m920x should pass in an empty structure rather than a NULL pointer, but I understand why people might disagree with that.

With that said, your patch looks good and I see that it fixes the issue. Please provide a sign-off so that your fix can be integrated and you will receive credit for your work.

Use the form:

Signed-off-by Your Name <[email protected]>

Regards,
Mike

_______________________________________________
linux-dvb mailing list
[email protected]
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Michael Krufky

2008-06-04 13:45 UTC #3 lore raw

Quoted 29 lines · Michael Krufky

Quoted 9 lines · Sigmund Augdal
Quoted 8 lines
changeset 49ba58715fe0 (7393) introduces an ooops in tda827x.c in tda827xa_lna_gain. The initialization of the "msg" variable accesses priv->cfg before the NULL check causing an oops when it is in fact NULL.
Best regards
Sigmund Augdal
2008/6/4 Sigmund Augdal <[email protected]>:
Quoted 6 lines
Attached patch fixes the problem.
Best regards
Sigmund Augdal
Sigmund,
The driver was only able to get into this function without priv->cfg being defined, because m920x passes in NULL as cfg.
In my opinion, this is flawed by design, and m920x should pass in an empty structure rather than a NULL pointer, but I understand why people might disagree with that.

Quoted 8 lines

With that said, your patch looks good and I see that it fixes the issue. Please provide a sign-off so that your fix can be integrated and you will receive credit for your work.
Use the form:
Signed-off-by Your Name <[email protected]>

Sigmund,

Looking at the C-1501 patch that you sent in, here is the cause of your OOPS.

if (dvb_attach(tda827x_attach, budget_ci->budget.dvb_frontend, 0x61,
+ &budget_ci->budget.i2c_adap, 0) == NULL)

You are passing "0" to the config structure of tda827x_attach. First off, "0" is an illegal value. This should be a pointer to a "struct tda827x_config"

...Please take a look at the tda827x_attach calls in saa7134-dvb.c for a better idea on what belongs there.

Regards,

Mike

_______________________________________________
linux-dvb mailing list
[email protected]
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Sigmund Augdal

author

2008-06-04 13:54 UTC #4 lore raw

Quoted 46 lines · Michael Krufky

Quoted 29 lines · Michael Krufky
Quoted 9 lines · Sigmund Augdal
Quoted 8 lines
changeset 49ba58715fe0 (7393) introduces an ooops in tda827x.c in tda827xa_lna_gain. The initialization of the "msg" variable accesses priv->cfg before the NULL check causing an oops when it is in fact NULL.
Best regards
Sigmund Augdal
2008/6/4 Sigmund Augdal <[email protected]>:
Quoted 6 lines
Attached patch fixes the problem.
Best regards
Sigmund Augdal
Sigmund,
The driver was only able to get into this function without priv->cfg being defined, because m920x passes in NULL as cfg.
In my opinion, this is flawed by design, and m920x should pass in an empty structure rather than a NULL pointer, but I understand why people might disagree with that.
Quoted 8 lines
With that said, your patch looks good and I see that it fixes the issue. Please provide a sign-off so that your fix can be integrated and you will receive credit for your work.
Use the form:
Signed-off-by Your Name <[email protected]>
Sigmund,
Looking at the C-1501 patch that you sent in, here is the cause of your OOPS.
if (dvb_attach(tda827x_attach, budget_ci->budget.dvb_frontend, 0x61,
(quoted diff) +1 -0
+				       &budget_ci->budget.i2c_adap, 0) == NULL)

Quoted 3 lines

You are passing "0" to the config structure of tda827x_attach. First off, "0" is an illegal value. This should be a pointer to a "struct tda827x_config"
...Please take a look at the tda827x_attach calls in saa7134-dvb.c for a better idea on what belongs there.

The documentation in tda827x.h says that this parameter is optional. Furthermore there are other drivers that don't use it (as you allready mentioned), and ever further the module used to work without crashing before the above-mentioned changeset. I think that applying a two line patch to fix a regression is worthwhile compared to having a number of drivers allocate structures to hold no useful information. I do however agree that I should have used NULL rather than 0.

Regards

Sigmund

Quoted 5 lines

Regards,
Mike

_______________________________________________
linux-dvb mailing list
[email protected]
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Michael Krufky

2008-06-04 14:04 UTC #5 lore raw

Quoted 51 lines · Sigmund Augdal

Quoted 49 lines · Michael Krufky
Quoted 29 lines · Michael Krufky
Quoted 11 lines · Sigmund Augdal
Quoted 10 lines
changeset 49ba58715fe0 (7393) introduces an ooops in tda827x.c in tda827xa_lna_gain. The initialization of the "msg" variable accesses priv->cfg before the NULL check causing an oops when it is in fact NULL.
Best regards
Sigmund Augdal
2008/6/4 Sigmund Augdal <[email protected]>:
Quoted 7 lines
Attached patch fixes the problem.
Best regards
Sigmund Augdal
Sigmund,
The driver was only able to get into this function without priv->cfg being defined, because m920x passes in NULL as cfg.
In my opinion, this is flawed by design, and m920x should pass in an empty structure rather than a NULL pointer, but I understand why people might disagree with that.
Quoted 9 lines
With that said, your patch looks good and I see that it fixes the issue. Please provide a sign-off so that your fix can be integrated and you will receive credit for your work.
Use the form:
Signed-off-by Your Name <[email protected]>
Sigmund,
Looking at the C-1501 patch that you sent in, here is the cause of your OOPS.
if (dvb_attach(tda827x_attach, budget_ci->budget.dvb_frontend, 0x61,
(quoted diff) +1 -0
+				       &budget_ci->budget.i2c_adap, 0) == NULL)
You are passing "0" to the config structure of tda827x_attach. First off, "0" is an illegal value. This should be a pointer to a "struct tda827x_config"

Quoted 9 lines

Quoted 2 lines
...Please take a look at the tda827x_attach calls in saa7134-dvb.c for a better idea on what belongs there.
The documentation in tda827x.h says that this parameter is optional. Furthermore there are other drivers that don't use it (as you allready mentioned), and ever further the module used to work without crashing before the above-mentioned changeset. I think that applying a two line patch to fix a regression is worthwhile compared to having a number of drivers allocate structures to hold no useful information. I do however agree that I should have used NULL rather than 0.

I agree with you as well -- I just wanted to state the facts so that this thread makes sense to other readers.

As stated in my prior email, your tda287x fix should definitely be merged after you send in your sign-off.

Regards,

Mike

_______________________________________________
linux-dvb mailing list
[email protected]
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Sigmund Augdal

author

2008-06-05 09:13 UTC #6 lore raw

Quoted 65 lines · Michael Krufky

Quoted 51 lines · Sigmund Augdal
Quoted 49 lines · Michael Krufky
Quoted 29 lines · Michael Krufky
Quoted 11 lines · Sigmund Augdal
Quoted 10 lines
changeset 49ba58715fe0 (7393) introduces an ooops in tda827x.c in tda827xa_lna_gain. The initialization of the "msg" variable accesses priv->cfg before the NULL check causing an oops when it is in fact NULL.
Best regards
Sigmund Augdal
2008/6/4 Sigmund Augdal <[email protected]>:
Quoted 7 lines
Attached patch fixes the problem.
Best regards
Sigmund Augdal
Sigmund,
The driver was only able to get into this function without priv->cfg being defined, because m920x passes in NULL as cfg.
In my opinion, this is flawed by design, and m920x should pass in an empty structure rather than a NULL pointer, but I understand why people might disagree with that.
Quoted 9 lines
With that said, your patch looks good and I see that it fixes the issue. Please provide a sign-off so that your fix can be integrated and you will receive credit for your work.
Use the form:
Signed-off-by Your Name <[email protected]>
Sigmund,
Looking at the C-1501 patch that you sent in, here is the cause of your OOPS.
if (dvb_attach(tda827x_attach, budget_ci->budget.dvb_frontend, 0x61,
(quoted diff) +1 -0
+				       &budget_ci->budget.i2c_adap, 0) == NULL)
You are passing "0" to the config structure of tda827x_attach. First off, "0" is an illegal value. This should be a pointer to a "struct tda827x_config"
Quoted 9 lines
Quoted 2 lines
...Please take a look at the tda827x_attach calls in saa7134-dvb.c for a better idea on what belongs there.
The documentation in tda827x.h says that this parameter is optional. Furthermore there are other drivers that don't use it (as you allready mentioned), and ever further the module used to work without crashing before the above-mentioned changeset. I think that applying a two line patch to fix a regression is worthwhile compared to having a number of drivers allocate structures to hold no useful information. I do however agree that I should have used NULL rather than 0.
I agree with you as well -- I just wanted to state the facts so that this thread makes sense to other readers.

Quoted 7 lines

As stated in my prior email, your tda287x fix should definitely be merged after you send in your sign-off.
Regards,
Mike

New patch with signed of line, passed through checkpatch as well.

Best regards,

Sigmund Augdal

Signed-off-by Sigmund Augdal <[email protected]>

linux/drivers/media/common/tuners/tda827x.c +2 -1

diff -r 6541620a09b7 linux/drivers/media/common/tuners/tda827x.c
@@ -419,13 +419,14 @@
 	unsigned char buf[] = {0x22, 0x01};
 	int arg;
 	int gp_func;
-	struct i2c_msg msg = { .addr = priv->cfg->switch_addr, .flags = 0,
+	struct i2c_msg msg = { .flags = 0,
 			       .buf = buf, .len = sizeof(buf) };
 
 	if (NULL == priv->cfg) {
 		dprintk("tda827x_config not defined, cannot set LNA gain!\n");
 		return;
 	}
+	msg.addr = priv->cfg->switch_addr;
 	if (priv->cfg->config) {
 		if (high)
 			dprintk("setting LNA to high gain\n");

_______________________________________________
linux-dvb mailing list
[email protected]
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb