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

[Antti Palosaari <crope@iki.fi>]

On 5/31/21 12:13 PM, Johan Hovold wrote:
> 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?


I can also confirm it works for both rtl2831u and rtl2832u.

regards
Antti