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

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

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

> On Mon, Aug 19, 2024 at 10:15:11AM +0200, Mauro Carvalho Chehab wrote:
> > This patch is duplicated of this one:
> > 
> > https://patchwork.linuxtv.org/project/linux-media/patch/20240409143634.33230-1-n.zhandarovich@fintech.ru/
> > 
> > The part I didn't like with such approach is that it checks only for
> > bulk endpoints. Most media devices have also isoc. Now, I'm not sure
> > about Siano devices. There are 3 different major chipsets supported
> > by this driver (sm1000, sm11xx, sm2xxx). Among them, sm1000 has one
> > USB ID for cold boot, and, once firmware is loaded, it gains another
> > USB ID for a a warm boot.  
> 
> Are you sure about all this?  The one source file in 
> drivers/media/usb/siano refers only to bulk endpoints, and the files in 
> drivers/media/common/siano don't refer to endpoints or URBs at all.  
> Nothing in either directory refers to isochronous endpoints.  Is there 
> some other place I should be looking?
> Also, while there are many constants in those files whose names start 
> with SMS1, there aren't any whose names start with SMS2 or SM2 (or their 
> lowercase equivalents).  And the Kconfig help text mentions only Siano 
> SMS1xxx.
> >
> > Your patch and the previously submitted one are not only checking
> > for the direction, but it is also discarding isoc endpoints.
> > Applying a change like that without testing with real hardware of
> > those three types just to make fuzz testing happy, sounded a little 
> > bit risky to my taste.
> > 
> > I would be more willing to pick it if the check would either be
> > tested on real hardware or if the logic would be changed to
> > accept either bulk or isoc endpoints, just like the current code.  
> 
> If the driver did apply to devices that used isochronous transfers, the 
> ideal approach would be to check the endpoint type against the device 
> type.  However, as it stands this doesn't seem to be necessary.

The initial driver had support only for SM1000 and SM11xx. There is a small 
note there about the sm1000 devices there (I guess this is the chipset
number of Stellar, but my memories might be tricking me), but without
a real association with the chipset number:

	/* This device is only present before firmware load */
	{ USB_DEVICE(0x187f, 0x0010),
		.driver_info = SMS1XXX_BOARD_SIANO_STELLAR_ROM },
	/* This device pops up after firmware load */
	{ USB_DEVICE(0x187f, 0x0100),
		.driver_info = SMS1XXX_BOARD_SIANO_STELLAR },

Years later, support for sm2xxx was added.

Those two boards, for instance (see drivers/media/common/siano/sms-cards.c)
are variants of sm2xxx (one of them is sm2270, if I'm not mistaken) that
supports Brazilian TV standard:

	[SMS1XXX_BOARD_SIANO_PELE] = {
		.name = "Siano Pele Digital Receiver",
		.type = SMS_PELE,
		.default_mode = DEVICE_MODE_ISDBT_BDA,
	},
	[SMS1XXX_BOARD_SIANO_RIO] = {
		.name = "Siano Rio Digital Receiver",
		.type = SMS_RIO,
		.default_mode = DEVICE_MODE_ISDBT_BDA,
	},

There are some boards there with a different version of sm22xx
that supports only DVB (can't remember anymore what boards).

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.

Thanks,
Mauro