Am Sonntag, 28. Dezember 2008 21:25:49 schrieb Tilman Schmidt:
> On Sun, 28 Dec 2008 15:38:25 +0100, Oliver Neukum wrote:
> > You cannot do this this way. Sharing an interface is against the spec.
>
> I see. Thanks for that information. So a device advertising an
> interface with:
>
> bInterfaceNumber 2
> bNumEndpoints 3
> bInterfaceClass 2 Communications
> bInterfaceSubClass 2 Abstract (modem)
> bInterfaceProtocol 1 AT-commands (v.25ter)
> CDC Union:
> bMasterInterface 2
> bSlaveInterface 2
>
> would not be conforming to the USB standard?
Quote from CDC spec:
Although this specification defines both the Communications Interface Class and Data Interface Class,
they are two different classes. All communications devices shall have an interface using the
Communications Class to manage the device and optionally specify themselves as communications
devices by using the Communications Device Class code. Additionally, the device has some number of
other interfaces used for actual data transmission.
----
"Other interfaces" is pretty clear.
> > If you do a generic work around you need to verify the endpoints are
> > correct.
>
> You mean something like this?
Yes.
> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> index d50a99f..7ce549a 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -896,8 +896,8 @@ static int acm_probe (struct usb_interface *intf,
> struct usb_interface *control_interface;
> struct usb_interface *data_interface;
> struct usb_endpoint_descriptor *epctrl;
> - struct usb_endpoint_descriptor *epread;
> - struct usb_endpoint_descriptor *epwrite;
> + struct usb_endpoint_descriptor *epread = NULL;
> + struct usb_endpoint_descriptor *epwrite = NULL;
> struct usb_device *usb_dev = interface_to_usbdev(intf);
> struct acm *acm;
> int minor;
> @@ -1028,29 +1028,38 @@ skip_normal_probe:
> if (intf != control_interface)
> return -ENODEV;
>
> - if (usb_interface_claimed(data_interface)) { /* valid in this context */
> + if (intf != data_interface && usb_interface_claimed(data_interface)) { /* valid in this context */
"intf != data_interface" is very non-obvious.
Formulate this some other way. Like you do below.
> + /* find data endpoints */
> + i = 0;
> + if (data_interface == control_interface) {
> + dev_dbg(&intf->dev,
> + "Shared interface. That's against the spec.\n");
> + i++; /* skip control EP */
What's wrong with "i = 1"?
> + }
> + while (i < data_interface->cur_altsetting->desc.bNumEndpoints) {
> + struct usb_endpoint_descriptor *ep
> + = &data_interface->cur_altsetting->endpoint[i++].desc;
> + if (usb_endpoint_xfer_bulk(ep)) {
> + if (epread == NULL && usb_endpoint_dir_in(ep))
We should bail out on superfluous endpoints.
The rest looks good.
Regards
Oliver
--
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
