linux-kernel - Re: Scanning for TV channels over DVB-T and DVB-T2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAA0YaJRs-HHO8NnoiVBujmcYXtJ_5CzrwBqkJ3vLiy1zWK+ruA@mail.gmail.com>
Date:   Fri, 26 Jun 2020 12:54:43 -0500
From:   Bradford Love <brad@...tdimension.cc>
To:     Marc Gonzalez <marc.w.gonzalez@...e.fr>
Cc:     linux-media <linux-media@...r.kernel.org>,
        Sean Young <sean@...s.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Jan Pieter van Woerkom <jp@...w.nl>,
        Antti Palosaari <crope@....fi>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: Scanning for TV channels over DVB-T and DVB-T2

Hi Marc,


On Fri, Jun 19, 2020 at 3:15 PM Marc Gonzalez <marc.w.gonzalez@...e.fr> wrote:
>
> On 10/06/2020 17:22, Marc Gonzalez wrote:
>
> > FTR, on IRC, Brad pointed out this patch of his:
> > https://patchwork.kernel.org/patch/10744999/
>
> I suggest the following patch on top of Brad's patch:
>
> Author: Marc Gonzalez <marc.w.gonzalez@...e.fr>
> Date:   Fri Jun 19 22:09:26 2020 +0200
>
>     si2168: wait for carrier lock before next step
>
> diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c
> index 31d3dc0216c2..e127e842f671 100644
> --- a/drivers/media/dvb-frontends/si2168.c
> +++ b/drivers/media/dvb-frontends/si2168.c
> @@ -152,6 +152,11 @@ static int si2168_ts_bus_ctrl(struct dvb_frontend *fe, int acquire)
>         return ret;
>  }
>
> +static bool carrier_locked(struct si2168_cmd *cmd)
> +{
> +       return cmd->args[2] & BIT(1);
> +}
> +
>  static int si2168_read_status(struct dvb_frontend *fe, enum fe_status *status)
>  {
>         struct i2c_client *client = fe->demodulator_priv;
> @@ -180,6 +185,9 @@ static int si2168_read_status(struct dvb_frontend *fe, enum fe_status *status)
>                 if (ret)
>                         goto err;
>
> +               if (!carrier_locked(&cmd))
> +                       goto parse_response;
> +


My original patch has been well tested and is currently deployed in
many thousands of assorted systems across Europe. Unless you can
guarantee that the frontend switchover race condition will *never*
happen *ever* across any system, including a large amount of
architectures and array of cpu types and speeds, I don't think it's
beneficial to remove it.

Hence, I'm very hesitant to deploy your patch and break this auto plp
detection for someone, just to save <=10ms.

Regards,

Brad



>                 if ((cmd.args[3] & 0x0f) == 7)
>                         sys = SYS_DVBT2;
>         }
> @@ -206,27 +214,10 @@ static int si2168_read_status(struct dvb_frontend *fe, enum fe_status *status)
>         }
>
>         ret = si2168_cmd_execute(client, &cmd);
> -       if (dvbt_auto_plp && (ret == -EREMOTEIO)) {
> -               /* In auto-PLP mode it is possible to read 0x8701 while
> -                * the frontend is in switchover transition. This causes
> -                * a status read failure, due to incorrect system. Check
> -                * the other sys if we hit this race condition.
> -                */
> -               if (sys == SYS_DVBT) {
> -                       memcpy(cmd.args, "\x50\x01", 2); /* DVB-T2 */
> -                       cmd.wlen = 2;
> -                       cmd.rlen = 14;
> -                       ret = si2168_cmd_execute(client, &cmd);
> -               } else if (sys == SYS_DVBT2) {
> -                       memcpy(cmd.args, "\xa0\x01", 2); /* DVB-T */
> -                       cmd.wlen = 2;
> -                       cmd.rlen = 13;
> -                       ret = si2168_cmd_execute(client, &cmd);
> -               }
> -       }
>         if (ret)
>                 goto err;
>
> +parse_response:
>         switch ((cmd.args[2] >> 1) & 0x03) {
>         case 0x01:
>                 *status = FE_HAS_SIGNAL | FE_HAS_CARRIER;
>