[linux-dvb] Ooops in tda827x.c

[linux-dvb] Ooops in tda827x.c

[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

Sigmund Augdal


_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

[linux-dvb] [PATCH] Re: Ooops in tda827x.c

[Sigmund Augdal]

[-- Attachment #1: Type: text/plain, Size: 545 bytes --]

Attached patch fixes the problem.

Best regards

Sigmund Augdal

On Wed, 2008-06-04 at 01:22 +0200, Sigmund Augdal wrote:
> 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
> linux-dvb@linuxtv.org
> http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
> 

[-- Attachment #2: tda827x-oops.patch --]
[-- Type: text/x-patch, Size: 694 bytes --]

diff -r 6541620a09b7 linux/drivers/media/common/tuners/tda827x.c
--- a/linux/drivers/media/common/tuners/tda827x.c	Tue Jun 03 10:32:16 2008 -0300
+++ b/linux/drivers/media/common/tuners/tda827x.c	Wed Jun 04 15:03:15 2008 +0200
@@ -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");

[-- Attachment #3: Type: text/plain, Size: 150 bytes --]

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] Re: Ooops in tda827x.c

[Michael Krufky]

> On Wed, 2008-06-04 at 01:22 +0200, Sigmund Augdal wrote:
>> 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 <sigmund@snap.tv>:
> 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@addre.ss>

Regards,
Mike

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] Re: Ooops in tda827x.c

[Michael Krufky]

Michael Krufky wrote:
>> On Wed, 2008-06-04 at 01:22 +0200, Sigmund Augdal wrote:
>>> 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 <sigmund@snap.tv>:
>> 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@addre.ss>
> 

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
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] Re: Ooops in tda827x.c

[Sigmund Augdal]

On Wed, 2008-06-04 at 09:45 -0400, Michael Krufky wrote:
> Michael Krufky wrote:
> >> On Wed, 2008-06-04 at 01:22 +0200, Sigmund Augdal wrote:
> >>> 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 <sigmund@snap.tv>:
> >> 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@addre.ss>
> > 
> 
> 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.
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
> 
> Regards,
> 
> Mike
> 


_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] Re: Ooops in tda827x.c

[Michael Krufky]

Sigmund Augdal wrote:
> On Wed, 2008-06-04 at 09:45 -0400, Michael Krufky wrote:
>   
>> Michael Krufky wrote:
>>     
>>>> On Wed, 2008-06-04 at 01:22 +0200, Sigmund Augdal wrote:
>>>>         
>>>>> 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 <sigmund@snap.tv>:
>>>       
>>>> 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@addre.ss>
>>>
>>>       
>> 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.
>>     
> 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
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] Re: Ooops in tda827x.c

[Sigmund Augdal]

[-- Attachment #1: Type: text/plain, Size: 2803 bytes --]

On Wed, 2008-06-04 at 10:03 -0400, Michael Krufky wrote:
> Sigmund Augdal wrote:
> > On Wed, 2008-06-04 at 09:45 -0400, Michael Krufky wrote:
> >   
> >> Michael Krufky wrote:
> >>     
> >>>> On Wed, 2008-06-04 at 01:22 +0200, Sigmund Augdal wrote:
> >>>>         
> >>>>> 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 <sigmund@snap.tv>:
> >>>       
> >>>> 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@addre.ss>
> >>>
> >>>       
> >> 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.
> >>     
> > 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
> 
New patch with signed of line, passed through checkpatch as well.

Best regards,

Sigmund Augdal

[-- Attachment #2: tda827x-oops.patch --]
[-- Type: text/x-patch, Size: 742 bytes --]

Signed-off-by: Sigmund Augdal <sigmund@snap.tv>
diff -r 6541620a09b7 linux/drivers/media/common/tuners/tda827x.c
--- a/linux/drivers/media/common/tuners/tda827x.c	Tue Jun 03 10:32:16 2008 -0300
+++ b/linux/drivers/media/common/tuners/tda827x.c	Wed Jun 04 15:03:15 2008 +0200
@@ -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");

[-- Attachment #3: Type: text/plain, Size: 150 bytes --]

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb