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

Sigmund Augdal sigmund at snap.tv
Thu Jun 5 11:13:56 CEST 2008


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 at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tda827x-oops.patch
Type: text/x-patch
Size: 742 bytes
Desc: not available
Url : http://www.linuxtv.org/pipermail/linux-dvb/attachments/20080605/b8b7c692/attachment.bin 


More information about the linux-dvb mailing list