linux-kernel - Re: [PATCH] media: gp8psk: initialize stats at power control logic

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: <CAA7C2qgAC8vnxu4xVhdi2CsMXyx85J44yUxGk00JhoQ6j5U3tw@mail.gmail.com>
Date:   Mon, 30 Nov 2020 18:07:37 -0800
From:   VDRU VDRU <user.vdr@...il.com>
To:     Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        linuxarm@...wei.com, mauro.chehab@...wei.com,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        syzbot <syzkaller@...glegroups.com>
Subject: Re: [PATCH] media: gp8psk: initialize stats at power control logic

Hi Mauro,

After many attempts ret was always 0. Please let me know if further
testing is needed.

Best regards,
Derek

On Mon, Nov 30, 2020 at 9:09 AM Mauro Carvalho Chehab
<mchehab+huawei@...nel.org> wrote:
>
> Hi Derek,
>
> Em Mon, 30 Nov 2020 08:04:31 -0800
> VDRU VDRU <user.vdr@...il.com> escreveu:
>
> > I have hardware that uses this driver and can conduct a test if it
> > will help resolve any confusion/assumption. I'd also like to suggest
> > that making changes to drivers with no means of testing those changes
> > is bad. This has happened in the past and resulted in unnecessarily
> > breaking drivers for those who use it. No patch should be merged
> > without testing!
>
> It helps a lot if you could test it.
>
> The current situation is that, if the I2C read fails, the
> driver will randomly power up only partially, which could
> cause issues.
>
> The original proposed approach:
>
>         https://lore.kernel.org/linux-media/20190627222020.45909-1-willemdebruijn.kernel@gmail.com/
>
> Will just give up trying to powering it up, while the
> patch I'm proposing will force the device to power up
> all parts of it. So, it seems safer than the original
> one.
>
> Please test with the enclosed patch. It is basically
> the same as the one I proposed, although this one will
> print a message at dlog, due to this:
>
>         pr_info("ret returned %d\n", ret);
>
> This could happen when the device got plugged and/or if
> you put the machine into suspend mode, when resuming it
> while streaming[1]
>
> Regards,
> Mauro
>
> [1] not sure if dvb-usb supports it. One of the rationales
> behind dvb-usb-v2 were to be able of properly do
> suspend/resumes.
>
>
>
> diff --git a/drivers/media/usb/dvb-usb/gp8psk.c b/drivers/media/usb/dvb-usb/gp8psk.c
> index c07f46f5176e..be55496cc717 100644
> --- a/drivers/media/usb/dvb-usb/gp8psk.c
> +++ b/drivers/media/usb/dvb-usb/gp8psk.c
> @@ -182,11 +182,16 @@ static int gp8psk_load_bcm4500fw(struct dvb_usb_device *d)
>
>  static int gp8psk_power_ctrl(struct dvb_usb_device *d, int onoff)
>  {
> -       u8 status, buf;
> +       u8 status = 0, buf;
> +       int ret;
>         int gp_product_id = le16_to_cpu(d->udev->descriptor.idProduct);
>
>         if (onoff) {
> -               gp8psk_usb_in_op(d, GET_8PSK_CONFIG,0,0,&status,1);
> +               ret = gp8psk_usb_in_op(d, GET_8PSK_CONFIG,0,0,&status,1);
> +               // Just to check if the condition happens in practice
> +               if (ret < 0)
> +                       pr_info("ret returned %d\n", ret);
> +
>                 if (! (status & bm8pskStarted)) {  /* started */
>                         if(gp_product_id == USB_PID_GENPIX_SKYWALKER_CW3K)
>                                 gp8psk_usb_out_op(d, CW3K_INIT, 1, 0, NULL, 0);
>