* [linux-dvb] Ooops in tda827x.c @ 2008-06-03 23:22 Sigmund Augdal 2008-06-04 13:06 ` [linux-dvb] [PATCH] " Sigmund Augdal 0 siblings, 1 reply; 7+ messages in thread From: Sigmund Augdal @ 2008-06-03 23:22 UTC (permalink / raw) To: linux-dvb; +Cc: Hartmut Hackmann 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [linux-dvb] [PATCH] Re: Ooops in tda827x.c 2008-06-03 23:22 [linux-dvb] Ooops in tda827x.c Sigmund Augdal @ 2008-06-04 13:06 ` Sigmund Augdal 2008-06-04 13:39 ` Michael Krufky 0 siblings, 1 reply; 7+ messages in thread From: Sigmund Augdal @ 2008-06-04 13:06 UTC (permalink / raw) To: linux-dvb; +Cc: Hartmut Hackmann [-- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [linux-dvb] [PATCH] Re: Ooops in tda827x.c 2008-06-04 13:06 ` [linux-dvb] [PATCH] " Sigmund Augdal @ 2008-06-04 13:39 ` Michael Krufky 2008-06-04 13:45 ` Michael Krufky 0 siblings, 1 reply; 7+ messages in thread From: Michael Krufky @ 2008-06-04 13:39 UTC (permalink / raw) To: Sigmund Augdal; +Cc: Hartmut Hackmann, linux-dvb > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [linux-dvb] [PATCH] Re: Ooops in tda827x.c 2008-06-04 13:39 ` Michael Krufky @ 2008-06-04 13:45 ` Michael Krufky 2008-06-04 13:54 ` Sigmund Augdal 0 siblings, 1 reply; 7+ messages in thread From: Michael Krufky @ 2008-06-04 13:45 UTC (permalink / raw) To: Sigmund Augdal; +Cc: Hartmut Hackmann, linux-dvb 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [linux-dvb] [PATCH] Re: Ooops in tda827x.c 2008-06-04 13:45 ` Michael Krufky @ 2008-06-04 13:54 ` Sigmund Augdal 2008-06-04 14:03 ` Michael Krufky 0 siblings, 1 reply; 7+ messages in thread From: Sigmund Augdal @ 2008-06-04 13:54 UTC (permalink / raw) To: Michael Krufky; +Cc: Hartmut Hackmann, linux-dvb 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [linux-dvb] [PATCH] Re: Ooops in tda827x.c 2008-06-04 13:54 ` Sigmund Augdal @ 2008-06-04 14:03 ` Michael Krufky 2008-06-05 9:13 ` Sigmund Augdal 0 siblings, 1 reply; 7+ messages in thread From: Michael Krufky @ 2008-06-04 14:03 UTC (permalink / raw) To: Sigmund Augdal; +Cc: Hartmut Hackmann, linux-dvb 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [linux-dvb] [PATCH] Re: Ooops in tda827x.c 2008-06-04 14:03 ` Michael Krufky @ 2008-06-05 9:13 ` Sigmund Augdal 0 siblings, 0 replies; 7+ messages in thread From: Sigmund Augdal @ 2008-06-05 9:13 UTC (permalink / raw) To: Michael Krufky; +Cc: Hartmut Hackmann, linux-dvb [-- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2008-06-05 9:13 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-06-03 23:22 [linux-dvb] Ooops in tda827x.c Sigmund Augdal 2008-06-04 13:06 ` [linux-dvb] [PATCH] " Sigmund Augdal 2008-06-04 13:39 ` Michael Krufky 2008-06-04 13:45 ` Michael Krufky 2008-06-04 13:54 ` Sigmund Augdal 2008-06-04 14:03 ` Michael Krufky 2008-06-05 9:13 ` Sigmund Augdal
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox