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

[Michael Krufky <mkrufky@linuxtv.org>]

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