DRX-K: Initial check-in

media New Dan Carpenter 2 participants 5 messages Jun 8, 2012 – Jun 26, 2012

Subsystems (2)

Media (V4L/DVB) Kernel janitors

lore

Share & tools

lore

Dan Carpenter

author

2012-06-08 13:46 UTC #0 lore raw

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

Ralph Metzler

2012-06-10 19:42 UTC #1 lore raw

Quoted 13 lines · Dan Carpenter

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) {

Quoted 3 lines

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.

Quoted 9 lines

  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;

Quoted 3 lines

	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

Dan Carpenter

author

2012-06-10 20:55 UTC #2 lore raw

Quoted 14 lines · Ralph Metzler

Quoted 13 lines · Dan Carpenter
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) {

Quoted 6 lines

Quoted 3 lines
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?

Quoted 10 lines


  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;

Quoted 8 lines

Quoted 3 lines
	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

Ralph Metzler

2012-06-15 16:54 UTC #3 lore raw

Quoted 19 lines · Dan Carpenter

Quoted 18 lines · Ralph Metzler
Quoted 16 lines · Dan Carpenter
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.

Quoted 5 lines

Quoted 2 lines
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

Dan Carpenter

author

2012-06-26 08:40 UTC #4 lore raw

PATCH 1/1 drxk: fix a '&' vs '|' bug

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 <[email protected]>

CC Ralph Metzler <[email protected]>

drivers/media/dvb/frontends/drxk_hard.c +1 -1

index 60b868f..7f6a7b5 100644
@@ -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 |