[media] duplicate code in media drivers

[media] duplicate code in media drivers

[Gustavo A. R. Silva]

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


I wonder if some of the cases above were intentionally coded that way or some code needs to be removed.

Thanks
--
Gustavo

Re: [media] duplicate code in media drivers

[Mauro Carvalho Chehab]

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

Re: [media] duplicate code in media drivers

[Devin Heitmueller]

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

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [media] duplicate code in media drivers

[Gustavo A. R. Silva]

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

Re: [media] duplicate code in media drivers

[Gustavo A. R. Silva]

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
>