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

[Johan Hovold <johan@kernel.org>]

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@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