On Mon, May 31, 2021 at 11:42 AM Johan Hovold <johan@xxxxxxxxxx> wrote:
>
> On Mon, May 31, 2021 at 09:52:18AM +0200, Johan Hovold wrote:
>
> > On Mon, May 31, 2021 at 09:58:09AM +0300, Eero Lehtinen wrote:
> > > Hi,
> > >
> > > I found dev_info messages from /var/log/messages.
> > >
> > > May 30 18:41:19 optipc kernel: [ 3.143433] dvb_usb_rtl28xxu
> > > 1-1:1.0: rtl28xxu_identify_state - ret1 = 0
> > > May 30 18:41:19 optipc kernel: [ 3.147688] dvb_usb_rtl28xxu
> > > 1-1:1.0: rtl28xxu_identify_state - ret2 = -32
> >
> > Ok, thanks. So this explains how things go wrong.
> >
> > ret = rtl28xxu_ctrl_msg(d, &req_demod_i2c);
> > if (ret == -EPIPE) {
> > dev->chip_id = CHIP_ID_RTL2831U;
> > } else if (ret == 0) {
> > dev->chip_id = CHIP_ID_RTL2832U;
> >
> > The chip used to be identified as RTL2832U but after my change it is
> > now detected as RTL2831U and the driver uses a separate implementation
> > with different hardcoded defaults.
> >
> > Commit d0f232e823af ("[media] rtl28xxu: add heuristic to detect chip
> > type") added this code and claimed that the i2c register in question
> > would only be found on newer RTL2832U models. Yet, actually reading the
> > register returns an error in your setup.
> >
> > So, something is fishy here. Has anyone verified that the chip-type
> > detection works as expected for older RTL2831U?
>
> 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?
>
> Johan
>
>
> From 2cec8fa000152bcb997dd7aeeb0917ebf744a7bd Mon Sep 17 00:00:00 2001
> From: Johan Hovold <johan@xxxxxxxxxx>
> 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@xxxxxxxxx>.
>
> Reported-by: syzbot+faf11bbadc5a372564da@xxxxxxxxxxxxxxxxxxxxxxxxx
> Reported-by: Eero Lehtinen <debiangamer2@xxxxxxxxx>
> Fixes: d0f232e823af ("[media] rtl28xxu: add heuristic to detect chip type")
> Cc: stable@xxxxxxxxxxxxxxx # 4.0
> Cc: Antti Palosaari <crope@xxxxxx>
> Signed-off-by: Johan Hovold <johan@xxxxxxxxxx>
> ---
> 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
>
Hi,
I confirm that it suppresses the warning without messing up the type detection.
