* re: [media] DRX-K: Initial check-in
@ 2012-06-08 13:46 Dan Carpenter
2012-06-10 19:42 ` Ralph Metzler
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2012-06-08 13:46 UTC (permalink / raw)
To: rjkm; +Cc: linux-media
Hello Ralph Metzler,
The patch 43dd07f758d8: "[media] DRX-K: Initial check-in" from Jul 3,
2011, leads to the following warning:
drivers/media/dvb/frontends/drxk_hard.c:2980 ADCSynchronization()
warn: suspicious bitop condition
2977 status = read16(state, IQM_AF_CLKNEG__A, &clkNeg);
2978 if (status < 0)
2979 goto error;
2980 if ((clkNeg | IQM_AF_CLKNEG_CLKNEGDATA__M) ==
2981 IQM_AF_CLKNEG_CLKNEGDATA_CLK_ADC_DATA_POS) {
IQM_AF_CLKNEG_CLKNEGDATA__M is 2.
IQM_AF_CLKNEG_CLKNEGDATA_CLK_ADC_DATA_POS is 0.
So this condition can never be true.
2982 clkNeg &= (~(IQM_AF_CLKNEG_CLKNEGDATA__M));
2983 clkNeg |=
2984 IQM_AF_CLKNEG_CLKNEGDATA_CLK_ADC_DATA_NEG;
2985 } else {
2986 clkNeg &= (~(IQM_AF_CLKNEG_CLKNEGDATA__M));
2987 clkNeg |=
2988 IQM_AF_CLKNEG_CLKNEGDATA_CLK_ADC_DATA_POS;
clkNeg |= 0; <-- doesn't make much sense to the unenlightened.
2989 }
I wrote a Smatch thing to find places that do x |= 0 inspired by the
above oddity and it finds several other places which do that:
drivers/media/dvb/frontends/drxk_hard.c:2525 GetQAMSignalToNoise() info: why not propagate 'status' from read16() instead of -22?
drivers/media/dvb/frontends/drxk_hard.c:2980 ADCSynchronization() warn: suspicious bitop condition
drivers/media/dvb/frontends/drxk_hard.c:2988 ADCSynchronization() warn: x |= 0
drivers/media/dvb/frontends/drxk_hard.c:3833 SetDVBT() warn: x |= 0
drivers/media/dvb/frontends/drxk_hard.c:3847 SetDVBT() warn: x |= 0
drivers/media/dvb/frontends/drxk_hard.c:3888 SetDVBT() warn: x |= 0
drivers/media/dvb/frontends/drxk_hard.c:3915 SetDVBT() warn: x |= 0
drivers/media/dvb/frontends/drxk_hard.c:3931 SetDVBT() warn: x |= 0
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* re: [media] DRX-K: Initial check-in
2012-06-08 13:46 [media] DRX-K: Initial check-in Dan Carpenter
@ 2012-06-10 19:42 ` Ralph Metzler
2012-06-10 20:54 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Ralph Metzler @ 2012-06-10 19:42 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-media
Dan Carpenter writes:
> Hello Ralph Metzler,
>
> The patch 43dd07f758d8: "[media] DRX-K: Initial check-in" from Jul 3,
> 2011, leads to the following warning:
> drivers/media/dvb/frontends/drxk_hard.c:2980 ADCSynchronization()
> warn: suspicious bitop condition
>
> 2977 status = read16(state, IQM_AF_CLKNEG__A, &clkNeg);
> 2978 if (status < 0)
> 2979 goto error;
> 2980 if ((clkNeg | IQM_AF_CLKNEG_CLKNEGDATA__M) ==
> 2981 IQM_AF_CLKNEG_CLKNEGDATA_CLK_ADC_DATA_POS) {
>
> IQM_AF_CLKNEG_CLKNEGDATA__M is 2.
> IQM_AF_CLKNEG_CLKNEGDATA_CLK_ADC_DATA_POS is 0.
> So this condition can never be true.
It seems this should be & instead of |. The mistake was also present in the windows driver.
>
> 2982 clkNeg &= (~(IQM_AF_CLKNEG_CLKNEGDATA__M));
> 2983 clkNeg |=
> 2984 IQM_AF_CLKNEG_CLKNEGDATA_CLK_ADC_DATA_NEG;
> 2985 } else {
> 2986 clkNeg &= (~(IQM_AF_CLKNEG_CLKNEGDATA__M));
> 2987 clkNeg |=
> 2988 IQM_AF_CLKNEG_CLKNEGDATA_CLK_ADC_DATA_POS;
>
> clkNeg |= 0; <-- doesn't make much sense to the unenlightened.
>
> 2989 }
This is perfectly normal since those defines were automatically created from the
firmware source code. It is better to leave the code as it is. If there ever is a firmware update
and these bits change their values it will be much harder to adjust the driver.
Regards,
Ralph
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [media] DRX-K: Initial check-in
2012-06-10 19:42 ` Ralph Metzler
@ 2012-06-10 20:54 ` Dan Carpenter
2012-06-15 16:54 ` Ralph Metzler
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2012-06-10 20:54 UTC (permalink / raw)
To: Ralph Metzler; +Cc: linux-media
On Sun, Jun 10, 2012 at 09:42:22PM +0200, Ralph Metzler wrote:
> Dan Carpenter writes:
> > Hello Ralph Metzler,
> >
> > The patch 43dd07f758d8: "[media] DRX-K: Initial check-in" from Jul 3,
> > 2011, leads to the following warning:
> > drivers/media/dvb/frontends/drxk_hard.c:2980 ADCSynchronization()
> > warn: suspicious bitop condition
> >
> > 2977 status = read16(state, IQM_AF_CLKNEG__A, &clkNeg);
> > 2978 if (status < 0)
> > 2979 goto error;
> > 2980 if ((clkNeg | IQM_AF_CLKNEG_CLKNEGDATA__M) ==
> > 2981 IQM_AF_CLKNEG_CLKNEGDATA_CLK_ADC_DATA_POS) {
> >
> > IQM_AF_CLKNEG_CLKNEGDATA__M is 2.
> > IQM_AF_CLKNEG_CLKNEGDATA_CLK_ADC_DATA_POS is 0.
> > So this condition can never be true.
>
> It seems this should be & instead of |. The mistake was also present in the windows driver.
>
Good deal. Do you want me to send a patch, or are you going to
handle it? Could I get a Reported-by cookie?
>
> >
> > 2982 clkNeg &= (~(IQM_AF_CLKNEG_CLKNEGDATA__M));
> > 2983 clkNeg |=
> > 2984 IQM_AF_CLKNEG_CLKNEGDATA_CLK_ADC_DATA_NEG;
> > 2985 } else {
> > 2986 clkNeg &= (~(IQM_AF_CLKNEG_CLKNEGDATA__M));
> > 2987 clkNeg |=
> > 2988 IQM_AF_CLKNEG_CLKNEGDATA_CLK_ADC_DATA_POS;
> >
> > clkNeg |= 0; <-- doesn't make much sense to the unenlightened.
> >
> > 2989 }
>
> This is perfectly normal since those defines were automatically created from the
> firmware source code. It is better to leave the code as it is. If there ever is a firmware update
> and these bits change their values it will be much harder to adjust the driver.
>
Sounds good. When I ran my script against the kernel it turns out
that doing x |= FOO; where foo is zero is very normal.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [media] DRX-K: Initial check-in
2012-06-10 20:54 ` Dan Carpenter
@ 2012-06-15 16:54 ` Ralph Metzler
2012-06-26 8:40 ` [patch] [media] drxk: fix a '&' vs '|' bug Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Ralph Metzler @ 2012-06-15 16:54 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-media
Dan Carpenter writes:
> On Sun, Jun 10, 2012 at 09:42:22PM +0200, Ralph Metzler wrote:
> > Dan Carpenter writes:
> > > Hello Ralph Metzler,
> > >
> > > The patch 43dd07f758d8: "[media] DRX-K: Initial check-in" from Jul 3,
> > > 2011, leads to the following warning:
> > > drivers/media/dvb/frontends/drxk_hard.c:2980 ADCSynchronization()
> > > warn: suspicious bitop condition
> > >
> > > 2977 status = read16(state, IQM_AF_CLKNEG__A, &clkNeg);
> > > 2978 if (status < 0)
> > > 2979 goto error;
> > > 2980 if ((clkNeg | IQM_AF_CLKNEG_CLKNEGDATA__M) ==
> > > 2981 IQM_AF_CLKNEG_CLKNEGDATA_CLK_ADC_DATA_POS) {
> > >
> > > IQM_AF_CLKNEG_CLKNEGDATA__M is 2.
> > > IQM_AF_CLKNEG_CLKNEGDATA_CLK_ADC_DATA_POS is 0.
> > > So this condition can never be true.
> >
> > It seems this should be & instead of |. The mistake was also present in the windows driver.
> >
>
> Good deal. Do you want me to send a patch, or are you going to
> handle it? Could I get a Reported-by cookie?
Please send a patch.
I am not maintaining the kernel version.
Regards,
Ralph
^ permalink raw reply [flat|nested] 5+ messages in thread* [patch] [media] drxk: fix a '&' vs '|' bug
2012-06-15 16:54 ` Ralph Metzler
@ 2012-06-26 8:40 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2012-06-26 8:40 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Oliver Endriss, linux-media, kernel-janitors, Ralph Metzler
IQM_AF_CLKNEG_CLKNEGDATA__M is 0x2 and
IQM_AF_CLKNEG_CLKNEGDATA_CLK_ADC_DATA_POS is 0. (clkNeg | 0x2) is never
equal to zero so the condition can never be true.
I consulted with Ralph Metzler and the '|' should be changed to a '&'.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
CC: Ralph Metzler <rjkm@metzlerbros.de>
diff --git a/drivers/media/dvb/frontends/drxk_hard.c b/drivers/media/dvb/frontends/drxk_hard.c
index 60b868f..7f6a7b5 100644
--- a/drivers/media/dvb/frontends/drxk_hard.c
+++ b/drivers/media/dvb/frontends/drxk_hard.c
@@ -2977,7 +2977,7 @@ static int ADCSynchronization(struct drxk_state *state)
status = read16(state, IQM_AF_CLKNEG__A, &clkNeg);
if (status < 0)
goto error;
- if ((clkNeg | IQM_AF_CLKNEG_CLKNEGDATA__M) ==
+ if ((clkNeg & IQM_AF_CLKNEG_CLKNEGDATA__M) ==
IQM_AF_CLKNEG_CLKNEGDATA_CLK_ADC_DATA_POS) {
clkNeg &= (~(IQM_AF_CLKNEG_CLKNEGDATA__M));
clkNeg |=
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-06-26 8:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-06-08 13:46 [media] DRX-K: Initial check-in Dan Carpenter
2012-06-10 19:42 ` Ralph Metzler
2012-06-10 20:54 ` Dan Carpenter
2012-06-15 16:54 ` Ralph Metzler
2012-06-26 8:40 ` [patch] [media] drxk: fix a '&' vs '|' bug Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).