| Message ID | c7f67d3b-f1e6-4d68-99aa-e462fdcb315f@rowland.harvard.edu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,resend] media: dvb: usb: Fix WARNING in dib0700_i2c_xfer/usb_submit_urb | expand |
Alan, > Fix the problem by adding the I2C_AQ_NO_ZERO_LEN_READ adapter quirk > flag to all the USB I2C adapter devices managed by dvb-usb-i2c.c, > following Wolfram Sang's suggestion. This tells the I2C core not to > allow length-0 read messages. Thank you again for fixing this issue. There are some USB-I2C bridges in drivers/i2c/busses which also do not prevent zero len reads. Would it make sense to put a protection into the I2C core? Can we reliably detect that an adapter sits on a USB (maybe via the parent device), so that we can then check if I2C_AQ_NO_ZERO_LEN_READ is set, and take action if not? Appreciating your input, Wolfram
On Fri, Mar 28, 2025 at 04:45:10PM +0100, Wolfram Sang wrote: > Alan, > > > Fix the problem by adding the I2C_AQ_NO_ZERO_LEN_READ adapter quirk > > flag to all the USB I2C adapter devices managed by dvb-usb-i2c.c, > > following Wolfram Sang's suggestion. This tells the I2C core not to > > allow length-0 read messages. > > Thank you again for fixing this issue. There are some USB-I2C bridges in > drivers/i2c/busses which also do not prevent zero len reads. Would it > make sense to put a protection into the I2C core? Can we reliably detect > that an adapter sits on a USB (maybe via the parent device), so that we > can then check if I2C_AQ_NO_ZERO_LEN_READ is set, and take action if > not? This really should be handled by someone who knows how those bridges work. In the case of dib0700, it was clear from the source code that the driver uses USB Control transfers to tell the hardware about I2C messages. I don't know if other bridges work in the same way. In theory a bridge could use USB Bulk transfers instead; they aren't subject to this restriction on length-0 reads. Or a bridge could use a Control read transfer but include extra header material along with the actual data, so that a length-0 message wouldn't end up generating a length-0 read. So the short answer is that you would need to find someone who really understands what's going on here -- which I don't. Sorry. Alan Stern
> In the case of dib0700, it was clear from the source code that the > driver uses USB Control transfers to tell the hardware about I2C > messages. I don't know if other bridges work in the same way. In > theory a bridge could use USB Bulk transfers instead; they aren't > subject to this restriction on length-0 reads. Or a bridge could use a > Control read transfer but include extra header material along with the > actual data, so that a length-0 message wouldn't end up generating a > length-0 read. Fully understood, thanks for your explanation. > So the short answer is that you would need to find someone who really > understands what's going on here -- which I don't. Sorry. No worries. There are only 5 drivers or so, I will manually check if they use a control_read and have no own header. Doesn't sound hard.
On Sat, Mar 29, 2025 at 07:05:49AM +0100, Wolfram Sang wrote: > > > In the case of dib0700, it was clear from the source code that the > > driver uses USB Control transfers to tell the hardware about I2C > > messages. I don't know if other bridges work in the same way. In > > theory a bridge could use USB Bulk transfers instead; they aren't > > subject to this restriction on length-0 reads. Or a bridge could use a > > Control read transfer but include extra header material along with the > > actual data, so that a length-0 message wouldn't end up generating a > > length-0 read. > > Fully understood, thanks for your explanation. > > > So the short answer is that you would need to find someone who really > > understands what's going on here -- which I don't. Sorry. > > No worries. There are only 5 drivers or so, I will manually check if > they use a control_read and have no own header. Doesn't sound hard. Good... Feel free to ask me if you have any questions or need any other help. Alan Stern
Index: usb-devel/drivers/media/usb/dvb-usb/dvb-usb-i2c.c =================================================================== --- usb-devel.orig/drivers/media/usb/dvb-usb/dvb-usb-i2c.c +++ usb-devel/drivers/media/usb/dvb-usb/dvb-usb-i2c.c @@ -8,6 +8,10 @@ */ #include "dvb-usb-common.h" +static const struct i2c_adapter_quirks i2c_usb_quirks = { + .flags = I2C_AQ_NO_ZERO_LEN_READ, +}; + int dvb_usb_i2c_init(struct dvb_usb_device *d) { int ret = 0; @@ -24,6 +28,7 @@ int dvb_usb_i2c_init(struct dvb_usb_devi strscpy(d->i2c_adap.name, d->desc->name, sizeof(d->i2c_adap.name)); d->i2c_adap.algo = d->props.i2c_algo; d->i2c_adap.algo_data = NULL; + d->i2c_adap.quirks = &i2c_usb_quirks; d->i2c_adap.dev.parent = &d->udev->dev; i2c_set_adapdata(&d->i2c_adap, d);