Re: [PATCH] media: rtl28xxu: add type-detection instrumentation

[Johan Hovold <johan@kernel.org>]

On Mon, May 31, 2021 at 12:08:20PM +0300, Eero Lehtinen wrote:
> On Mon, May 31, 2021 at 11:42 AM Johan Hovold <johan@kernel.org> wrote:

> > Ok, the driver just wants to know if the i2c-read vendor request exists,
> > and actually reading the register will not work since the register may
> > not even exist (e.g. depending on the demodulator).
> >
> > So it seems we need to keep this zero-length read request and only
> > update the pipe argument to suppress the new WARN() in
> > usb_submit_urb().
> >
> > Eero, could you try applying the below on top of -next and confirm that
> > it suppresses the warning without messing up the type detection?

> > From 2cec8fa000152bcb997dd7aeeb0917ebf744a7bd Mon Sep 17 00:00:00 2001
> > From: Johan Hovold <johan@kernel.org>
> > Date: Mon, 24 May 2021 10:55:19 +0200
> > Subject: [PATCH v2] media: rtl28xxu: fix zero-length control request
> >
> > The direction of the pipe argument must match the request-type direction
> > bit or control requests may fail depending on the host-controller-driver
> > implementation.
> >
> > Control transfers without a data stage are treated as OUT requests by
> > the USB stack and should be using usb_sndctrlpipe(). Failing to do so
> > will now trigger a warning.
> >
> > The driver uses a zero-length i2c-read request for type detection so
> > update the control-request code to use usb_sndctrlpipe() in this case.
> >
> > Note that actually trying to read the i2c register in question does not
> > work as the register might not exist (e.g. depending on the demodulator)
> > as reported by Eero Lehtinen <debiangamer2@gmail.com>.
> >
> > Reported-by: syzbot+faf11bbadc5a372564da@syzkaller.appspotmail.com
> > Reported-by: Eero Lehtinen <debiangamer2@gmail.com>
> > Fixes: d0f232e823af ("[media] rtl28xxu: add heuristic to detect chip type")
> > Cc: stable@vger.kernel.org      # 4.0
> > Cc: Antti Palosaari <crope@iki.fi>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >  drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
> > index 97ed17a141bb..a6124472cb06 100644
> > --- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
> > +++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
> > @@ -37,7 +37,16 @@ static int rtl28xxu_ctrl_msg(struct dvb_usb_device *d, struct rtl28xxu_req *req)
> >         } else {
> >                 /* read */
> >                 requesttype = (USB_TYPE_VENDOR | USB_DIR_IN);
> > -               pipe = usb_rcvctrlpipe(d->udev, 0);
> > +
> > +               /*
> > +                * Zero-length transfers must use usb_sndctrlpipe() and
> > +                * rtl28xxu_identify_state() uses a zero-length i2c read
> > +                * command to determine the chip type.
> > +                */
> > +               if (req->size)
> > +                       pipe = usb_rcvctrlpipe(d->udev, 0);
> > +               else
> > +                       pipe = usb_sndctrlpipe(d->udev, 0);
> >         }
> >
> >         ret = usb_control_msg(d->udev, pipe, 0, requesttype, req->value,
> > --
> > 2.31.1

> I confirm that it suppresses the warning without messing up the type
> detection.

Thanks for confirming. Is it ok if I add also a tested-by tag for you to
the commit message when I send this to the media people?

Johan