| Message ID | d0ac1a26ed5943127cb0156148735f5f52a07075.1606459576.git.mchehab+huawei@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: gp8psk: initialize stats at power control logic | expand |
On Fri, Nov 27, 2020 at 1:46 AM Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > As reported on: > https://lore.kernel.org/linux-media/20190627222020.45909-1-willemdebruijn.kernel@gmail.com/ > > if gp8psk_usb_in_op() returns an error, the status var is not > initialized. Yet, this var is used later on, in order to > identify: > - if the device was already started; > - if firmware has loaded; > - if the LNBf was powered on. > > Using status = 0 seems to ensure that everything will be > properly powered up. > > So, instead of the proposed solution, let's just set > status = 0. > > Reported-by: syzbot <syzkaller@googlegroups.com> > Reported-by: Willem de Bruijn <willemb@google.com> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > --- > drivers/media/usb/dvb-usb/gp8psk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/usb/dvb-usb/gp8psk.c b/drivers/media/usb/dvb-usb/gp8psk.c > index c07f46f5176e..b4f661bb5648 100644 > --- a/drivers/media/usb/dvb-usb/gp8psk.c > +++ b/drivers/media/usb/dvb-usb/gp8psk.c > @@ -182,7 +182,7 @@ 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 gp_product_id = le16_to_cpu(d->udev->descriptor.idProduct); > > if (onoff) { > -- > 2.28.0 Is it okay to ignore the return value of gp8psk_usb_in_op here?
Em Fri, 27 Nov 2020 09:20:53 -0500 Willem de Bruijn <willemdebruijn.kernel@gmail.com> escreveu: > On Fri, Nov 27, 2020 at 1:46 AM Mauro Carvalho Chehab > <mchehab+huawei@kernel.org> wrote: > > > > As reported on: > > https://lore.kernel.org/linux-media/20190627222020.45909-1-willemdebruijn.kernel@gmail.com/ > > > > if gp8psk_usb_in_op() returns an error, the status var is not > > initialized. Yet, this var is used later on, in order to > > identify: > > - if the device was already started; > > - if firmware has loaded; > > - if the LNBf was powered on. > > > > Using status = 0 seems to ensure that everything will be > > properly powered up. > > > > So, instead of the proposed solution, let's just set > > status = 0. > > > > Reported-by: syzbot <syzkaller@googlegroups.com> > > Reported-by: Willem de Bruijn <willemb@google.com> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > --- > > drivers/media/usb/dvb-usb/gp8psk.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/media/usb/dvb-usb/gp8psk.c b/drivers/media/usb/dvb-usb/gp8psk.c > > index c07f46f5176e..b4f661bb5648 100644 > > --- a/drivers/media/usb/dvb-usb/gp8psk.c > > +++ b/drivers/media/usb/dvb-usb/gp8psk.c > > @@ -182,7 +182,7 @@ 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 gp_product_id = le16_to_cpu(d->udev->descriptor.idProduct); > > > > if (onoff) { > > -- > > 2.28.0 > > > Is it okay to ignore the return value of gp8psk_usb_in_op here? Well, I don't have this specific hardware in my hands, but, if you follow the logic there, it sounds ok to ignore. It should be noticed that, on some devices, some I2C commands will only return after having the device powered up and its firmware loaded. As this code is at the powerup part of the code, it sound reasonable to assume that the I2C read might fail. So, this change is less aggressive than just returning, as the driver may be relying on a fail read already. --- If you follow the logic of this routine, a fail to read means that the hardware is not able to return to this specific I2C command, either because it was physically (or logically) removed or because it was not properly powered up. If it was removed, trying to send I2C commands to power it up will return errors, so the first attempt of writing data to it will return an error. If, on the other hand, the hardware was not properly powered up, status = 0 will mean that all parts of the chipset should be powered on. As this is the only place at the driver where a read is not checked for its success, I'm assuming that this is the original intent of the driver's author. Thanks, Mauro
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! Best regards, Derek On Mon, Nov 30, 2020 at 1:48 AM Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > Em Fri, 27 Nov 2020 09:20:53 -0500 > Willem de Bruijn <willemdebruijn.kernel@gmail.com> escreveu: > > > On Fri, Nov 27, 2020 at 1:46 AM Mauro Carvalho Chehab > > <mchehab+huawei@kernel.org> wrote: > > > > > > As reported on: > > > https://lore.kernel.org/linux-media/20190627222020.45909-1-willemdebruijn.kernel@gmail.com/ > > > > > > if gp8psk_usb_in_op() returns an error, the status var is not > > > initialized. Yet, this var is used later on, in order to > > > identify: > > > - if the device was already started; > > > - if firmware has loaded; > > > - if the LNBf was powered on. > > > > > > Using status = 0 seems to ensure that everything will be > > > properly powered up. > > > > > > So, instead of the proposed solution, let's just set > > > status = 0. > > > > > > Reported-by: syzbot <syzkaller@googlegroups.com> > > > Reported-by: Willem de Bruijn <willemb@google.com> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > > --- > > > drivers/media/usb/dvb-usb/gp8psk.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/usb/dvb-usb/gp8psk.c b/drivers/media/usb/dvb-usb/gp8psk.c > > > index c07f46f5176e..b4f661bb5648 100644 > > > --- a/drivers/media/usb/dvb-usb/gp8psk.c > > > +++ b/drivers/media/usb/dvb-usb/gp8psk.c > > > @@ -182,7 +182,7 @@ 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 gp_product_id = le16_to_cpu(d->udev->descriptor.idProduct); > > > > > > if (onoff) { > > > -- > > > 2.28.0 > > > > > > Is it okay to ignore the return value of gp8psk_usb_in_op here? > > > Well, I don't have this specific hardware in my hands, but, if you > follow the logic there, it sounds ok to ignore. > > It should be noticed that, on some devices, some I2C commands > will only return after having the device powered up and its > firmware loaded. As this code is at the powerup part of the code, > it sound reasonable to assume that the I2C read might fail. > > So, this change is less aggressive than just returning, as > the driver may be relying on a fail read already. > > --- > > If you follow the logic of this routine, a fail to read means > that the hardware is not able to return to this specific > I2C command, either because it was physically (or logically) > removed or because it was not properly powered up. > > If it was removed, trying to send I2C commands to > power it up will return errors, so the first attempt of > writing data to it will return an error. > > If, on the other hand, the hardware was not properly powered up, > status = 0 will mean that all parts of the chipset should > be powered on. > > As this is the only place at the driver where a read is > not checked for its success, I'm assuming that this is the > original intent of the driver's author. > > Thanks, > Mauro
Hi Derek, Em Mon, 30 Nov 2020 08:04:31 -0800 VDRU VDRU <user.vdr@gmail.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);
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@kernel.org> wrote: > > Hi Derek, > > Em Mon, 30 Nov 2020 08:04:31 -0800 > VDRU VDRU <user.vdr@gmail.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); >
diff --git a/drivers/media/usb/dvb-usb/gp8psk.c b/drivers/media/usb/dvb-usb/gp8psk.c index c07f46f5176e..b4f661bb5648 100644 --- a/drivers/media/usb/dvb-usb/gp8psk.c +++ b/drivers/media/usb/dvb-usb/gp8psk.c @@ -182,7 +182,7 @@ 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 gp_product_id = le16_to_cpu(d->udev->descriptor.idProduct); if (onoff) {
As reported on: https://lore.kernel.org/linux-media/20190627222020.45909-1-willemdebruijn.kernel@gmail.com/ if gp8psk_usb_in_op() returns an error, the status var is not initialized. Yet, this var is used later on, in order to identify: - if the device was already started; - if firmware has loaded; - if the LNBf was powered on. Using status = 0 seems to ensure that everything will be properly powered up. So, instead of the proposed solution, let's just set status = 0. Reported-by: syzbot <syzkaller@googlegroups.com> Reported-by: Willem de Bruijn <willemb@google.com> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> --- drivers/media/usb/dvb-usb/gp8psk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)