| Message ID | 20180521193951.GA16659@embeddedor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Mon, 21 May 2018 14:39:51 -0500 "Gustavo A. R. Silva" <gustavo@embeddedor.com> escreveu: > Hi Mauro, > > I found some duplicate code with the help of Coccinelle and Coverity. Notice that these are not code patches, they only point out the duplicate code in some media drivers: > > diff -u -p drivers/media/pci/bt8xx/dvb-bt8xx.c /tmp/nothing/media/pci/bt8xx/dvb-bt8xx.c > --- drivers/media/pci/bt8xx/dvb-bt8xx.c > +++ /tmp/nothing/media/pci/bt8xx/dvb-bt8xx.c > @@ -389,9 +389,7 @@ static int advbt771_samsung_tdtc9251dh0_ > else if (c->frequency < 600000000) > bs = 0x08; > else if (c->frequency < 730000000) > - bs = 0x08; > else > - bs = 0x08; > > pllbuf[0] = 0x61; > pllbuf[1] = div >> 8; Hmm... I *suspect* that "bs" here controls the frequency range for the tuner. Analog tuners have separate frequency regions that are controlled via a register, into a 4 or 5 bytes I2C sequence. They're all somewhat a clone of an old Philips design. It should be safe to convert the "BS" sequence on something like: if (c->frequency < 173000000) bs = 0x01; else if (c->frequency < 470000000) bs = 0x02; else bs = 0x08; > diff -u -p drivers/media/usb/dvb-usb/dib0700_devices.c /tmp/nothing/media/usb/dvb-usb/dib0700_devices.c > --- drivers/media/usb/dvb-usb/dib0700_devices.c > +++ /tmp/nothing/media/usb/dvb-usb/dib0700_devices.c > @@ -1741,13 +1741,6 @@ static int dib809x_tuner_attach(struct d > struct dib0700_adapter_state *st = adap->priv; > struct i2c_adapter *tun_i2c = st->dib8000_ops.get_i2c_master(adap->fe_adap[0].fe, DIBX000_I2C_INTERFACE_TUNER, 1); > > - if (adap->id == 0) { > - if (dvb_attach(dib0090_register, adap->fe_adap[0].fe, tun_i2c, &dib809x_dib0090_config) == NULL) > - return -ENODEV; > - } else { > - if (dvb_attach(dib0090_register, adap->fe_adap[0].fe, tun_i2c, &dib809x_dib0090_config) == NULL) > - return -ENODEV; > - } I'm almost sure that, on the second if, it should be adap->fe_adap[1].fe. I tried in the past to check this, but didn't got an answer from the one that wrote the code. Maybe we could add a /* FIXME: check if it is fe_adap[1] */ on the second clause. > > st->set_param_save = adap->fe_adap[0].fe->ops.tuner_ops.set_params; > adap->fe_adap[0].fe->ops.tuner_ops.set_params = dib8096_set_param_override; > diff -u -p drivers/media/dvb-frontends/mb86a16.c /tmp/nothing/media/dvb-frontends/mb86a16.c > --- drivers/media/dvb-frontends/mb86a16.c > +++ /tmp/nothing/media/dvb-frontends/mb86a16.c > @@ -1466,9 +1466,7 @@ static int mb86a16_set_fe(struct mb86a16 > wait_t = (1572864 + state->srate / 2) / state->srate; > if (state->srate < 5000) > /* FIXME ! , should be a long wait ! */ > - msleep_interruptible(wait_t); > else > - msleep_interruptible(wait_t); I suspect that the goal here is to point that sleeping for (1572864 + state->srate / 2) / state->srate when srate is low will mean that it will take a lot of time to converge (probably causing timeout at userspace). Basically, if srate is < 5000, the sleep time will be between 314 and 1575364 ms. The worse case scenario - although not realistic, in practice - is to wait up to 26 seconds. This is a very long time! Probably, the right fix here would be to check if wait_t is bigger than a certain amount of time. If so, return an error. I'm not against removing the if, but, if so, better to add a /* FIXME */ block explaining that. That's said, this is an old device. I doubt anyone would fix it. > > if (sync_chk(state, &junk) == 0) { > iq_vt_set(state, 1); > diff -u -p drivers/media/dvb-frontends/au8522_decoder.c /tmp/nothing/media/dvb-frontends/au8522_decoder.c > --- drivers/media/dvb-frontends/au8522_decoder.c > +++ /tmp/nothing/media/dvb-frontends/au8522_decoder.c > @@ -280,14 +280,9 @@ static void setup_decoder_defaults(struc > AU8522_TOREGAAGC_REG0E5H_CVBS); > au8522_writereg(state, AU8522_REG016H, AU8522_REG016H_CVBS); > > - if (is_svideo) { > /* Despite what the table says, for the HVR-950q we still need > to be in CVBS mode for the S-Video input (reason unknown). */ > /* filter_coef_type = 3; */ > - filter_coef_type = 5; > - } else { > - filter_coef_type = 5; > - } Better ask Devin about this (c/c). > > /* Load the Video Decoder Filter Coefficients */ > for (i = 0; i < NUM_FILTER_COEF; i++) { > > > I wonder if some of the cases above were intentionally coded that way or some code needs to be removed. > > Thanks > -- > Gustavo Thanks, Mauro
>> diff -u -p drivers/media/dvb-frontends/au8522_decoder.c /tmp/nothing/media/dvb-frontends/au8522_decoder.c >> --- drivers/media/dvb-frontends/au8522_decoder.c >> +++ /tmp/nothing/media/dvb-frontends/au8522_decoder.c >> @@ -280,14 +280,9 @@ static void setup_decoder_defaults(struc >> AU8522_TOREGAAGC_REG0E5H_CVBS); >> au8522_writereg(state, AU8522_REG016H, AU8522_REG016H_CVBS); >> >> - if (is_svideo) { >> /* Despite what the table says, for the HVR-950q we still need >> to be in CVBS mode for the S-Video input (reason unknown). */ >> /* filter_coef_type = 3; */ >> - filter_coef_type = 5; >> - } else { >> - filter_coef_type = 5; >> - } > > Better ask Devin about this (c/c). This was a case where the implementation didn't match the datasheet, and it wasn't clear why the filter coefficients weren't working properly. Essentially I should have labeled that as a TODO or FIXME when I disabled the "right" value and forced it to always be five. It was also likely that the filter coefficients would need to differ if taking video over the IF interface as opposed to CVBS/S-video, which is why I didn't want to get rid of the logic entirely. That said, the only product I've ever seen with the tda18271 mated to the au8522 will likely never be supported for analog video under Linux for unrelated reasons. That said, it's worked "good enough" since I wrote the code nine years ago, so if somebody wants to submit a patch to either get rid of the if() statement or mark it as a FIXME that will likely never actually get fixed, I wouldn't have an objection to either. Devin
Mauro, I already sent some patches based on your comments. Thanks! -- Gustavo On 05/21/2018 03:14 PM, Mauro Carvalho Chehab wrote: > Em Mon, 21 May 2018 14:39:51 -0500 > "Gustavo A. R. Silva" <gustavo@embeddedor.com> escreveu: > >> Hi Mauro, >> >> I found some duplicate code with the help of Coccinelle and Coverity. Notice that these are not code patches, they only point out the duplicate code in some media drivers: >> >> diff -u -p drivers/media/pci/bt8xx/dvb-bt8xx.c /tmp/nothing/media/pci/bt8xx/dvb-bt8xx.c >> --- drivers/media/pci/bt8xx/dvb-bt8xx.c >> +++ /tmp/nothing/media/pci/bt8xx/dvb-bt8xx.c >> @@ -389,9 +389,7 @@ static int advbt771_samsung_tdtc9251dh0_ >> else if (c->frequency < 600000000) >> bs = 0x08; >> else if (c->frequency < 730000000) >> - bs = 0x08; >> else >> - bs = 0x08; >> >> pllbuf[0] = 0x61; >> pllbuf[1] = div >> 8; > > > Hmm... I *suspect* that "bs" here controls the frequency range for the > tuner. Analog tuners have separate frequency regions that are controlled > via a register, into a 4 or 5 bytes I2C sequence. They're all somewhat > a clone of an old Philips design. > > It should be safe to convert the "BS" sequence on something like: > > if (c->frequency < 173000000) > bs = 0x01; > else if (c->frequency < 470000000) > bs = 0x02; > else > bs = 0x08; > > > >> diff -u -p drivers/media/usb/dvb-usb/dib0700_devices.c /tmp/nothing/media/usb/dvb-usb/dib0700_devices.c >> --- drivers/media/usb/dvb-usb/dib0700_devices.c >> +++ /tmp/nothing/media/usb/dvb-usb/dib0700_devices.c >> @@ -1741,13 +1741,6 @@ static int dib809x_tuner_attach(struct d >> struct dib0700_adapter_state *st = adap->priv; >> struct i2c_adapter *tun_i2c = st->dib8000_ops.get_i2c_master(adap->fe_adap[0].fe, DIBX000_I2C_INTERFACE_TUNER, 1); >> >> - if (adap->id == 0) { >> - if (dvb_attach(dib0090_register, adap->fe_adap[0].fe, tun_i2c, &dib809x_dib0090_config) == NULL) >> - return -ENODEV; >> - } else { >> - if (dvb_attach(dib0090_register, adap->fe_adap[0].fe, tun_i2c, &dib809x_dib0090_config) == NULL) >> - return -ENODEV; >> - } > > I'm almost sure that, on the second if, it should be adap->fe_adap[1].fe. > I tried in the past to check this, but didn't got an answer from the one > that wrote the code. > > Maybe we could add a /* FIXME: check if it is fe_adap[1] */ on the > second clause. > >> >> st->set_param_save = adap->fe_adap[0].fe->ops.tuner_ops.set_params; >> adap->fe_adap[0].fe->ops.tuner_ops.set_params = dib8096_set_param_override; >> diff -u -p drivers/media/dvb-frontends/mb86a16.c /tmp/nothing/media/dvb-frontends/mb86a16.c >> --- drivers/media/dvb-frontends/mb86a16.c >> +++ /tmp/nothing/media/dvb-frontends/mb86a16.c >> @@ -1466,9 +1466,7 @@ static int mb86a16_set_fe(struct mb86a16 >> wait_t = (1572864 + state->srate / 2) / state->srate; >> if (state->srate < 5000) >> /* FIXME ! , should be a long wait ! */ >> - msleep_interruptible(wait_t); >> else >> - msleep_interruptible(wait_t); > > I suspect that the goal here is to point that sleeping for > (1572864 + state->srate / 2) / state->srate when srate is low will mean > that it will take a lot of time to converge (probably causing timeout at > userspace). > > Basically, if srate is < 5000, the sleep time will be between > 314 and 1575364 ms. The worse case scenario - although not realistic, > in practice - is to wait up to 26 seconds. This is a very long time! > > Probably, the right fix here would be to check if wait_t is bigger than > a certain amount of time. If so, return an error. > > I'm not against removing the if, but, if so, better to add a /* FIXME */ > block explaining that. > > That's said, this is an old device. I doubt anyone would fix it. > > >> >> if (sync_chk(state, &junk) == 0) { >> iq_vt_set(state, 1); >> diff -u -p drivers/media/dvb-frontends/au8522_decoder.c /tmp/nothing/media/dvb-frontends/au8522_decoder.c >> --- drivers/media/dvb-frontends/au8522_decoder.c >> +++ /tmp/nothing/media/dvb-frontends/au8522_decoder.c >> @@ -280,14 +280,9 @@ static void setup_decoder_defaults(struc >> AU8522_TOREGAAGC_REG0E5H_CVBS); >> au8522_writereg(state, AU8522_REG016H, AU8522_REG016H_CVBS); >> >> - if (is_svideo) { >> /* Despite what the table says, for the HVR-950q we still need >> to be in CVBS mode for the S-Video input (reason unknown). */ >> /* filter_coef_type = 3; */ >> - filter_coef_type = 5; >> - } else { >> - filter_coef_type = 5; >> - } > > Better ask Devin about this (c/c). > >> >> /* Load the Video Decoder Filter Coefficients */ >> for (i = 0; i < NUM_FILTER_COEF; i++) { >> >> >> I wonder if some of the cases above were intentionally coded that way or some code needs to be removed. >> >> Thanks >> -- >> Gustavo > > > > Thanks, > Mauro >
On 05/21/2018 03:44 PM, Devin Heitmueller wrote: >>> diff -u -p drivers/media/dvb-frontends/au8522_decoder.c /tmp/nothing/media/dvb-frontends/au8522_decoder.c >>> --- drivers/media/dvb-frontends/au8522_decoder.c >>> +++ /tmp/nothing/media/dvb-frontends/au8522_decoder.c >>> @@ -280,14 +280,9 @@ static void setup_decoder_defaults(struc >>> AU8522_TOREGAAGC_REG0E5H_CVBS); >>> au8522_writereg(state, AU8522_REG016H, AU8522_REG016H_CVBS); >>> >>> - if (is_svideo) { >>> /* Despite what the table says, for the HVR-950q we still need >>> to be in CVBS mode for the S-Video input (reason unknown). */ >>> /* filter_coef_type = 3; */ >>> - filter_coef_type = 5; >>> - } else { >>> - filter_coef_type = 5; >>> - } >> >> Better ask Devin about this (c/c). > > This was a case where the implementation didn't match the datasheet, > and it wasn't clear why the filter coefficients weren't working > properly. Essentially I should have labeled that as a TODO or FIXME > when I disabled the "right" value and forced it to always be five. It > was also likely that the filter coefficients would need to differ if > taking video over the IF interface as opposed to CVBS/S-video, which > is why I didn't want to get rid of the logic entirely. That said, the > only product I've ever seen with the tda18271 mated to the au8522 will > likely never be supported for analog video under Linux for unrelated > reasons. > > That said, it's worked "good enough" since I wrote the code nine years > ago, so if somebody wants to submit a patch to either get rid of the > if() statement or mark it as a FIXME that will likely never actually > get fixed, I wouldn't have an objection to either. > Devin, I've sent a patch based on your feedback. Thanks! -- Gustavo
diff -u -p drivers/media/pci/bt8xx/dvb-bt8xx.c /tmp/nothing/media/pci/bt8xx/dvb-bt8xx.c --- drivers/media/pci/bt8xx/dvb-bt8xx.c +++ /tmp/nothing/media/pci/bt8xx/dvb-bt8xx.c @@ -389,9 +389,7 @@ static int advbt771_samsung_tdtc9251dh0_ else if (c->frequency < 600000000) bs = 0x08; else if (c->frequency < 730000000) - bs = 0x08; else - bs = 0x08; pllbuf[0] = 0x61; pllbuf[1] = div >> 8; diff -u -p drivers/media/usb/dvb-usb/dib0700_devices.c /tmp/nothing/media/usb/dvb-usb/dib0700_devices.c --- drivers/media/usb/dvb-usb/dib0700_devices.c +++ /tmp/nothing/media/usb/dvb-usb/dib0700_devices.c @@ -1741,13 +1741,6 @@ static int dib809x_tuner_attach(struct d struct dib0700_adapter_state *st = adap->priv; struct i2c_adapter *tun_i2c = st->dib8000_ops.get_i2c_master(adap->fe_adap[0].fe, DIBX000_I2C_INTERFACE_TUNER, 1); - if (adap->id == 0) { - if (dvb_attach(dib0090_register, adap->fe_adap[0].fe, tun_i2c, &dib809x_dib0090_config) == NULL) - return -ENODEV; - } else { - if (dvb_attach(dib0090_register, adap->fe_adap[0].fe, tun_i2c, &dib809x_dib0090_config) == NULL) - return -ENODEV; - } st->set_param_save = adap->fe_adap[0].fe->ops.tuner_ops.set_params; adap->fe_adap[0].fe->ops.tuner_ops.set_params = dib8096_set_param_override; diff -u -p drivers/media/dvb-frontends/mb86a16.c /tmp/nothing/media/dvb-frontends/mb86a16.c --- drivers/media/dvb-frontends/mb86a16.c +++ /tmp/nothing/media/dvb-frontends/mb86a16.c @@ -1466,9 +1466,7 @@ static int mb86a16_set_fe(struct mb86a16 wait_t = (1572864 + state->srate / 2) / state->srate; if (state->srate < 5000) /* FIXME ! , should be a long wait ! */ - msleep_interruptible(wait_t); else - msleep_interruptible(wait_t); if (sync_chk(state, &junk) == 0) { iq_vt_set(state, 1); diff -u -p drivers/media/dvb-frontends/au8522_decoder.c /tmp/nothing/media/dvb-frontends/au8522_decoder.c --- drivers/media/dvb-frontends/au8522_decoder.c +++ /tmp/nothing/media/dvb-frontends/au8522_decoder.c @@ -280,14 +280,9 @@ static void setup_decoder_defaults(struc AU8522_TOREGAAGC_REG0E5H_CVBS); au8522_writereg(state, AU8522_REG016H, AU8522_REG016H_CVBS); - if (is_svideo) { /* Despite what the table says, for the HVR-950q we still need to be in CVBS mode for the S-Video input (reason unknown). */ /* filter_coef_type = 3; */ - filter_coef_type = 5; - } else { - filter_coef_type = 5; - } /* Load the Video Decoder Filter Coefficients */ for (i = 0; i < NUM_FILTER_COEF; i++) {