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@xxxxxxx>:
> >>>
> >>>> 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@xxxxxxxx>
> >>>
> >>>
> >> 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
Signed-off-by: Sigmund Augdal <sigmund@xxxxxxx>
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");
_______________________________________________
linux-dvb mailing list
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
