On Mon, Mar 06, 2017 at 02:14:51PM +0100, Oliver Neukum wrote:
> Am Montag, den 06.03.2017, 12:27 +0100 schrieb Johan Hovold:
> > On Mon, Mar 06, 2017 at 10:54:44AM +0100, Oliver Neukum wrote:
> > > Yes, it would be wrong to see this as an attribute per driver.
> > > It needs to apply when you match. AFAICS every serial subdriver
> > > has a device table. Requirements need to go there.
> >
> > Every USB-serial-bus driver has a device table, but devices are
> > matched against these tables by USB-serial core and not USB core.
> > And then
>
> Are you refering to
>
> static struct usb_serial_driver * const serial_drivers[] = {
> &whiteheat_fake_device, &whiteheat_device, NULL
> };
>
> versus
>
> static const struct usb_device_id id_table_combined[] = {
> { USB_DEVICE(CONNECT_TECH_VENDOR_ID,
> CONNECT_TECH_WHITE_HEAT_ID) },
> { USB_DEVICE(CONNECT_TECH_VENDOR_ID,
> CONNECT_TECH_FAKE_WHITE_HEAT_ID) },
> { } /* Terminating
> entry */
> };
>
> I was taking about the latter. As far as I can tell it goes to usbcore.
Yes, but since the latter combines the two classes of devices, each
which may have different endpoint requirements, you'd instead have to
encode the requirements in each and every USB_DEVICE entry rather than
once in every struct usb_serial_driver.
> > there's also a "combined" device table, for all USB-serial bus
> > drivers implemented by the same USB interface driver.
> >
> > Are you suggesting the requirements should be specified per device,
> > rather than per driver (device table)? That does not seem right to
> > me, as it is really the driver which needs to verify the resources
> > it needs for its implementation (e.g. before allocating a read urb).
> > This means that all devices in a table would have the same (minimum)
> > constraints anyway.
>
> No. A driver may be fine with 2 input and 2 output bulk endpoints
> or 4 of both kinds but nothing in between.
> The core can do that match if you have matching entries for
> both device types (and IDs).
So this would be represented by two struct usb_serial_driver in my
scheme, but you instead suggest doing this on a device-id level, and also
put multiple entries in for every device-id when more than one endpoint
configuration is supported (with the same id).
This seems like a lot of work, especially when all devices handled by a
driver have the same constraints, or if the (constraint) classes instead
can, as in the USB serial case, be represented by a subdriver type (e.g.
struct usb_serial_driver) and looked-up from the device id.
> > So I do think this should be a driver attribute, but in the USB-
> > serial case, several classes of devices can be handled by the same
> > USB driver, so the constraints then needs to be specified at the
> > USB-serial-bus driver level instead.
>
> And that driver would save even more logic if the core already told it
> for which pattern it matches.
True, but my aim here is primarily to make it easier to add the
too-often forgotten but still required sanity checks. In the USB-serial
case, matching on device id is basically sufficient to be able to
identify the device classes.
It also seems what you're proposing would require exact rather than
minimum constraints to identify the classes. I fear there are just too
many variants of devices out there for this too be practical, and
I suspect neither is the exact configuration of the 7.5k USB device-id
entries we already have today known. Minimum constraints could be
derived from the code and specified per struct usb_driver though.
Core providing the endpoint type counts and descriptors per type would
still allow for much logic to be removed also in your 2+2 or 4+4 bulk
endpoints with same device-id case.
> > > Yes, but could you explain how this is connected to filtering at
> > > probe time other than happening at probe time?
> >
> > Only in that this functionality will still be needed in USB-serial
> > to implement the port-endpoint remap functionality even if USB core
> > were to gain something similar.
>
> But why do remap and filtering need to go to the same place?
They don't, USB core could do a basic sanity check, and USB-serial allow
for the remapping. The point is that USB-serial requires larger arrays
(MAX_NUM_PORTS = 16) for the remapping, than what a generic
implementation does (15), so we'd still need to construct the arrays in
USB serial.
But that is just to cover two drivers that could be dealt with
differently if needed to enable a generic implementation of this.
> > > True, but again not specific to serial drivers. DVB-T will face
> > > the same issues.
> >
> > Certainly, and I should have elaborated a bit more on the
> > differences here. For USB-serial endpoint-resources are allocated
> > only after subdriver probe returns, while for most (all) other USB
> > driver this would need to be done in the USB probe callback
> > directly. In that case, checking before probe might be too soon (fw
> > download, altsetting) and checking after would be too late (endpoint
> > resources would already need to have been setup).
>
> Now that argues for usbserial to trigger the check itself, but not for
> the code doing the check living in usbserial. In other words put
>
> struct usb_serial_endpoints and find_endpoints into usbcore and
> call it from usbserial. Add a flag to tell usbcore to not check
> this by itself and you are done. (With some generic name changes)
>
> That way everybody can use it almost for free.
An idea would be to always construct the endpoint-data struct (as a
member of usb_interface) when setting an alternate setting (or always
keep it around in usb_host_interface), and provide minimal constraints
per usb_driver which core verified before probe unless a flag is set.
Would the overhead for this be acceptable? I guess some more memory
would be required for control and isochronous endpoints too.
> > I realise that it may look like a duplicated effort, but I really
> > don't think it is for the reasons given in this thread.
>
> IMHO the reasons are valid but they can be overcome resulting in more
> code being shared.
I'm all for generalising this where possible, but there are corner cases
(e.g. like cdc-acm needing to do sanity checks of two interfaces) and
doing this for USB serial first could be an intermediate step while
identifying the type of constraints needed by other subsystems.
But let me look at this some more.
Also Greg, how does the above fit with what you had in mind?
Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
