On Mon, Jan 20, 2020 at 01:36:31PM +0100, Oliver Neukum wrote:
> Hi Johan,
>
> I have looked at your heroic efforts at sanity checking
> and I cannot help myself wondering whether this is a winning
> strategy. Shall we really specify for each device how many
> endpoints it is suposed to have in the probe() method?
>
> Could we extend the matching by a minimum and maximum number
> of endpoints and masks for permissible endpoint types?
>
> For class devices this is impossible, but the majority of
> drivers are for vendor specific devices.
I have considered it, but I don't think that encoding this in the device
id table would work. Just take something like the option driver with
about a thousand entries. Encoding what is essentially a driver
requirement would just waste a lot of space.
It's also not clear how to encode the various ways that drivers look up
endpoints, such as by number or by descriptor order, and often in
various combinations (e.g. an endpoint can be optional or the driver can
handle both interrupt and bulk types). For this to work generally, you'd
also need to deal with drivers switching altsettings (e.g. which
altsetting should core verify?).
Any change in this direction would also risk introducing regressions as
we don't have access to the descriptors for devices that have already
been added.
Instead I came to the conclusion that we should treat this just like any
other resource allocation and to provide helpers for drivers to use (I
have some more work in this area coming).
Just like it's a bug to not check the return value from kmalloc() or
platform_get_irq() so should trying to use an endpoint before making
sure it is available be considered a bug even in cases where it doesn't
outright crash the kernel currently. And we should of course always
sanity check external input.
One way of enforcing this would be to eventually require drivers to
initialise URBs using a struct usb_host_endpoint which they would have
lookup up using common helpers. There's even a comment by Alan in
usb_submit_urb() documenting this intention (see 5b653c79c04c ("USB: add
urb->ep")).
Johan
