On Tue, Aug 20, 2024 at 01:10:33AM +0200, Mauro Carvalho Chehab wrote:
> Em Mon, 19 Aug 2024 13:14:19 -0400
> Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> escreveu:
> > Currently the driver exports only bulk endpoints, even though it doesn't
> > check the endpoint type. You can tell because the only routine in it
> > that calls usb_submit_urb() is smsusb_submit_urb(), and that routine
> > calls usb_fill_bulk_urb() before doing the submission.
> >
> > Given this, I suggest merging the earlier patch submission from Nikita
> > Zhandarovich as-is. If the driver ever evolves to include support for
> > isochronous endpoints, the probe function can be modified then.
>
> I'll see if I can try his patch and see if device keeps working. The
> logic indeed use endpoints in bulk mode, but I'm not sure if, for all the
> BIOS files at drivers/media/common/siano/smscoreapi.[ch], the endpoints
> are properly reported as bulk.
>
> What happens if an endpoint is reported as ISOC, but the URB submit
> is called without URB_ISO_ASAP? On a quick check, the code at usb_submit_urb()
> seems to not complain about that.
It _does_ complain if a driver submits a bulk URB to an isochronous
endpoint. See the usb_pipe_type_check() function and the dev_WARN() on
line 503 of drivers/usb/core/urb.c. (In any case, the URB_ISO_ASAP flag
is optional, so of course there is no complaint if the flag is missing.)
Furthermore, if an endpoint really is isochronous but the driver uses
usb_fill_bulk_urb(), the transfer won't work at all. URBs for those two
endpoint types use completely different ways of specifying their data
buffers and transfer lengths. See the paragraph starting with
"Isochronous URBs have a different data transfer model" in the kerneldoc
for the definition of struct urb in include/linux/usb.h.
> I would be a lot more comfortable if the patch were using just
>
> if (usb_endpoint_dir_in(desc))
> ...
> if (usb_endpoint_dir_out(desc))
> ...
>
> or something like this (to accept both isoc and bulk):
>
> if (!usb_endpoint_xfer_int(epd)) {
> if (usb_endpoint_dir_in(desc))
> ...
> if (usb_endpoint_dir_out(desc))
> ...
> }
>
>
> instead of calling usb_endpoint_xfer_bulk(desc) to check if type
> is bulk.
>
> I'll try to do some tests, but not sure when, as I'm traveling abroad
> this week.
Instead of going to the trouble of running a test, you could simply run
"lsusb -v" and check whether or not all the endpoints are bulk.
Alan Stern
