Re: [PATCH] media/usb/siano: Fix endpoint type checking in smsusb

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

Em Mon, 19 Aug 2024 13:14:19 -0400
Alan Stern <stern@rowland.harvard.edu> escreveu:

> On Mon, Aug 19, 2024 at 06:24:56PM +0200, Mauro Carvalho Chehab wrote:
> > Basically, the actual SMS device type is given by this enum:
> > 
> > 	enum sms_device_type_st {
> > 		SMS_UNKNOWN_TYPE = -1,
> > 
> > 		SMS_STELLAR = 0,
> > 		SMS_NOVA_A0,
> > 		SMS_NOVA_B0,
> > 		SMS_VEGA,
> > 		SMS_VENICE,
> > 		SMS_MING,
> > 		SMS_PELE,
> > 		SMS_RIO,
> > 		SMS_DENVER_1530,
> > 		SMS_DENVER_2160,
> > 
> > 		SMS_NUM_OF_DEVICE_TYPES	/* This is just a count */
> > 	};
> > 
> > But I dunno if there are a 1:1 mapping between type and chipset 
> > number. The above type names probably match some vendor internal 
> > names, but we never had any tables associating them to a device number,
> > as the vendor never provided us such information.
> > 
> > Btw I vaguely remember I heard about a newer Siano chipsets (sm3xxx), 
> > but never saw such devices.
> > 
> > -
> > 
> > Now, I'm not sure about what endpoints this specific driver exports, as
> > I'm lacking vendor's documentation. What I said is that almost all DVB 
> > devices have isoc endpoints, but I dunno if this is the case of Siano.  
> 
> 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.

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.


Thanks,
Mauro