On Tue, Jan 04, 2011 at 02:13:05PM +0900, Yojiro UO wrote: > as the ugen(4) is too flexible (and unsafe) than other usb device drivers, > i don't like this work to extend ugen(4)'s area. > > I know many userland applications which supports multiple platform using > ugen type interface (because they usually use libusb or ugen apis), but > it is no reason to support them without considerations. > > ..... > > >From usb developer's view (include me), this work is seems to the so > useful to use "unsupported" / "partially supported" device. hmm... > > -- Yojiro UO
well, when ugen(4) is attached as a secondary device, it is not allowed to change the configuration, and is only allowed access to the unclaimed interfaces. this is essentially the same as other class/interface drivers. normal ugen(4) attachments and drivers that attach by matching vendor/pid are given full access to the device. there is no defined class for scanners. so, I don't see how usb scanners will be supported without a generic usb device driver, except maybe something like uscanner ... this saves me from having to disable ulpt, so that I can use my mfp as a ugen, which then forces me to use CUPS for printing. now I don't have to fiddle in ukc or use CUPS. I don't disagree that the wrong device attaching as ugen could have unforseen negative consequences. you wouldn't want to allow unrestricted use of a ugen attached to a umass interface that is controlling a filesystem, for example. but we attach umass there, so that shouldn't happen. > > > On 2011/01/04, at 8:18, Jacob Meuser wrote: > > > when a USB device is inserted, usbd_probe_and_attach() first tries to > > find a driver that claims to support the device by matching the > > vendor/product IDs. if no such driver is found, usbd_probe_and_attach() > > loops through each interface of each of the devices configurations, > > trying to match "interface" (aka class) drivers to an interface. if > > the device has more than one interface *in a single configuration* > > that matches drivers, then drivers are allowed to attach to each > > interface. for example: > > > > uvideo0 at uhub5 port 1 configuration 1 interface 0 "Logitech product > > 0x09a2" rev 2.00/0.08 addr 4 > > video0 at uvideo0 > > uaudio0 at uhub5 port 1 configuration 1 interface 2 "Logitech product > > 0x09a2" rev 2.00/0.08 addr 4 > > uaudio0: audio rev 1.00, 2 mixer controls > > audio1 at uaudio0 > > > > note how they are both attached "at uhub5 port 1 configuration 1". > > > > and: > > > > uhidev0 at uhub1 port 2 configuration 1 interface 0 "Logitech USB Receiver" > > rev 2.00/16.00 addr 2 > > uhidev0: iclass 3/1 > > ums0 at uhidev0: 16 buttons, Z dir > > wsmouse1 at ums0 mux 0 > > uhidev1 at uhub1 port 2 configuration 1 interface 1 "Logitech USB Receiver" > > rev 2.00/16.00 addr 2 > > uhidev1: iclass 3/0, 17 report ids > > uhid0 at uhidev1 reportid 3: input=4, output=0, feature=0 > > uhid1 at uhidev1 reportid 16: input=6, output=6, feature=0 > > uhid2 at uhidev1 reportid 17: input=19, output=19, feature=0 > > > > note how both uhidev(4)s are attached "at uhub1 port 2 configuration 1". > > > > currently, usbd_probe_and_attach() makes a copy of the array of pointers > > to the interface descriptors and passes these to the match/attach functions > > in struct usb_attach_args (uaa). when an interface is matched, the pointer > > to the interface descriptor in uaa is set to NULL. and, if a driver uses > > other interfaces, the driver sets the pointer to those interface > > descriptors in the uaa to NULL. this way, what interfaces are available > > is known, but this only really works at attach time. > > > > currently, ugen(4) assumes it can use any interface in any configuration. > > for this reason, ugen(4) is only attached if no interfaces have drivers > > attached. > > > > the following allows ugen(4) to be safely attached when there are unused > > interfaces. this is accomplished by: > > > > * instead of NULLing pointers to interface descriptors in the uaa, mark > > interfaces as being claimed in the usbd_device's copy of the interface > > descriptors > > * allow ugen(4) to be attached if there are unused interfaces in a > > configuration that has had drivers attached > > * make ugen(4) aware that it may be sharing a device with (an)other > > driver(s), and if so: > > * do not let ugen(4) change the configuration > > * do not let ugen(4) access the already claimed interfaces > > > > this allows, for example: > > > > ulpt0 at uhub0 port 1 configuration 1 interface 1 "HP Officejet 4500 > > G510g-m" rev 2.00/1.00 addr 2 > > ulpt0: using bi-directional mode > > ugen0 at uhub0 port 1 configuration 1 "HP Officejet 4500 G510g-m" rev > > 2.00/1.00 addr 2 > > > > which means this MFP can be used by both lpd and sane, simultaneously. > > this also requires a patch to libusb, to let it know that ugen(4) might > > not be the first driver attached to the device. that patch is at > > http://jakemsr.trancell.org/libusb.diff > > > > thoughts? oks? > > > > -- > > jake...@sdf.lonestar.org > > SDF Public Access UNIX System - http://sdf.lonestar.org > > > > Index: if_cdce.c > > =================================================================== > > RCS file: /cvs/src/sys/dev/usb/if_cdce.c,v > > retrieving revision 1.47 > > diff -u -p if_cdce.c > > --- if_cdce.c 27 Oct 2010 17:51:11 -0000 1.47 > > +++ if_cdce.c 3 Jan 2011 21:52:27 -0000 > > @@ -224,12 +224,12 @@ cdce_attach(struct device *parent, struct device *self > > DPRINTF(("cdce_attach: union interface: ctl=%d, data=%d\n", > > ctl_ifcno, data_ifcno)); > > for (i = 0; i < uaa->nifaces; i++) { > > - if (uaa->ifaces[i] == NULL) > > + if (usbd_iface_claimed(sc->cdce_udev, i)) > > continue; > > id = usbd_get_interface_descriptor(uaa->ifaces[i]); > > if (id != NULL && id->bInterfaceNumber == data_ifcno) { > > sc->cdce_data_iface = uaa->ifaces[i]; > > - uaa->ifaces[i] = NULL; > > + usbd_claim_iface(sc->cdce_udev, i); > > } > > } > > } > > Index: if_urndis.c > > =================================================================== > > RCS file: /cvs/src/sys/dev/usb/if_urndis.c,v > > retrieving revision 1.27 > > diff -u -p if_urndis.c > > --- if_urndis.c 27 Oct 2010 17:51:11 -0000 1.27 > > +++ if_urndis.c 3 Jan 2011 21:52:27 -0000 > > @@ -1389,12 +1389,12 @@ urndis_attach(struct device *parent, struct device > > *se > > DPRINTF(("urndis_attach: union interface: ctl %u, data %u\n", > > if_ctl, if_data)); > > for (i = 0; i < uaa->nifaces; i++) { > > - if (uaa->ifaces[i] == NULL) > > + if (usbd_iface_claimed(sc->sc_udev, i)) > > continue; > > id = usbd_get_interface_descriptor(uaa->ifaces[i]); > > if (id && id->bInterfaceNumber == if_data) { > > sc->sc_iface_data = uaa->ifaces[i]; > > - uaa->ifaces[i] = NULL; > > + usbd_claim_iface(sc->sc_udev, i); > > } > > } > > } > > Index: uaudio.c > > =================================================================== > > RCS file: /cvs/src/sys/dev/usb/uaudio.c,v > > retrieving revision 1.89 > > diff -u -p uaudio.c > > --- uaudio.c 18 Aug 2010 22:54:58 -0000 1.89 > > +++ uaudio.c 3 Jan 2011 21:52:27 -0000 > > @@ -210,7 +210,6 @@ struct uaudio_softc { > > struct device sc_dev; /* base device */ > > usbd_device_handle sc_udev; /* USB device */ > > int sc_ac_iface; /* Audio Control interface */ > > - usbd_interface_handle sc_ac_ifaceh; > > struct chan sc_playchan; /* play channel */ > > struct chan sc_recchan; /* record channel */ > > int sc_nullalt; > > @@ -520,10 +519,9 @@ uaudio_attach(struct device *parent, struct device *se > > return; > > } > > > > - sc->sc_ac_ifaceh = uaa->iface; > > /* Pick up the AS interface. */ > > for (i = 0; i < uaa->nifaces; i++) { > > - if (uaa->ifaces[i] == NULL) > > + if (usbd_iface_claimed(sc->sc_udev, i)) > > continue; > > id = usbd_get_interface_descriptor(uaa->ifaces[i]); > > if (id == NULL) > > @@ -537,7 +535,7 @@ uaudio_attach(struct device *parent, struct device *se > > } > > } > > if (found) > > - uaa->ifaces[i] = NULL; > > + usbd_claim_iface(sc->sc_udev, i); > > } > > > > for (j = 0; j < sc->sc_nalts; j++) { > > Index: ugen.c > > =================================================================== > > RCS file: /cvs/src/sys/dev/usb/ugen.c,v > > retrieving revision 1.62 > > diff -u -p ugen.c > > --- ugen.c 24 Sep 2010 08:33:59 -0000 1.62 > > +++ ugen.c 3 Jan 2011 21:52:28 -0000 > > @@ -103,6 +103,7 @@ struct ugen_softc { > > > > int sc_refcnt; > > u_char sc_dying; > > + u_char sc_secondary; > > }; > > > > void ugenintr(usbd_xfer_handle xfer, usbd_private_handle addr, > > @@ -166,13 +167,18 @@ ugen_attach(struct device *parent, struct device *self > > > > sc->sc_udev = udev = uaa->device; > > > > - /* First set configuration index 0, the default one for ugen. */ > > - err = usbd_set_config_index(udev, 0, 0); > > - if (err) { > > - printf("%s: setting configuration index 0 failed\n", > > - sc->sc_dev.dv_xname); > > - sc->sc_dying = 1; > > - return; > > + if (usbd_get_devcnt(udev) > 0) > > + sc->sc_secondary = 1; > > + > > + if (!sc->sc_secondary) { > > + /* First set configuration index 0, the default one for ugen. */ > > + err = usbd_set_config_index(udev, 0, 0); > > + if (err) { > > + printf("%s: setting configuration index 0 failed\n", > > + sc->sc_dev.dv_xname); > > + sc->sc_dying = 1; > > + return; > > + } > > } > > conf = usbd_get_config_descriptor(udev)->bConfigurationValue; > > > > @@ -220,9 +226,15 @@ ugen_set_config(struct ugen_softc *sc, int configno) > > /* Avoid setting the current value. */ > > cdesc = usbd_get_config_descriptor(dev); > > if (!cdesc || cdesc->bConfigurationValue != configno) { > > - err = usbd_set_config_no(dev, configno, 1); > > - if (err) > > - return (err); > > + if (sc->sc_secondary) { > > + printf("%s: secondary, not changing config to %d\n", > > + __func__, configno); > > + return (USBD_IN_USE); > > + } else { > > + err = usbd_set_config_no(dev, configno, 1); > > + if (err) > > + return (err); > > + } > > } > > > > err = usbd_interface_count(dev, &niface); > > @@ -231,6 +243,10 @@ ugen_set_config(struct ugen_softc *sc, int configno) > > memset(sc->sc_endpoints, 0, sizeof sc->sc_endpoints); > > for (ifaceno = 0; ifaceno < niface; ifaceno++) { > > DPRINTFN(1,("ugen_set_config: ifaceno %d\n", ifaceno)); > > + if (usbd_iface_claimed(dev, ifaceno)) { > > + DPRINTF(("%s: iface %d claimed\n", __func__, ifaceno)); > > + continue; > > + } > > err = usbd_device2interface_handle(dev, ifaceno, &iface); > > if (err) > > return (err); > > Index: umodem.c > > =================================================================== > > RCS file: /cvs/src/sys/dev/usb/umodem.c,v > > retrieving revision 1.39 > > diff -u -p umodem.c > > --- umodem.c 2 Dec 2010 01:37:45 -0000 1.39 > > +++ umodem.c 3 Jan 2011 21:52:28 -0000 > > @@ -280,12 +280,12 @@ umodem_attach(struct device *parent, struct device *se > > > > /* Get the data interface too. */ > > for (i = 0; i < uaa->nifaces; i++) { > > - if (uaa->ifaces[i] != NULL) { > > + if (!usbd_iface_claimed(sc->sc_udev, i)) { > > id = usbd_get_interface_descriptor(uaa->ifaces[i]); > > if (id != NULL && > > id->bInterfaceNumber == data_iface_no) { > > sc->sc_data_iface = uaa->ifaces[i]; > > - uaa->ifaces[i] = NULL; > > + usbd_claim_iface(sc->sc_udev, i); > > } > > } > > } > > ? usb-n.diff > > Index: usb_subr.c > > =================================================================== > > RCS file: /cvs/src/sys/dev/usb/usb_subr.c,v > > retrieving revision 1.77 > > diff -u -p usb_subr.c > > --- usb_subr.c 17 Dec 2010 22:38:54 -0000 1.77 > > +++ usb_subr.c 3 Jan 2011 21:52:28 -0000 > > @@ -777,7 +777,7 @@ usbd_set_config_index(usbd_device_handle dev, int inde > > /* Allocate and fill interface data. */ > > nifc = cdp->bNumInterface; > > dev->ifaces = malloc(nifc * sizeof(struct usbd_interface), > > - M_USB, M_NOWAIT); > > + M_USB, M_NOWAIT | M_ZERO); > > if (dev->ifaces == NULL) { > > err = USBD_NOMEM; > > goto bad; > > @@ -859,14 +859,13 @@ usbd_getnewaddr(usbd_bus_handle bus) > > return (-1); > > } > > > > - > > usbd_status > > usbd_probe_and_attach(struct device *parent, usbd_device_handle dev, int > > port, > > int addr) > > { > > struct usb_attach_arg uaa; > > usb_device_descriptor_t *dd = &dev->ddesc; > > - int found, i, confi, nifaces, len; > > + int i, confi, nifaces, len; > > usbd_status err; > > struct device *dv; > > usbd_interface_handle *ifaces; > > @@ -895,8 +894,8 @@ usbd_probe_and_attach(struct device *parent, usbd_devi > > err = USBD_NOMEM; > > goto fail; > > } > > - dev->subdevs[0] = dv; > > - dev->subdevs[1] = 0; > > + dev->subdevs[dev->ndevs++] = dv; > > + dev->subdevs[dev->ndevs] = 0; > > err = USBD_NORMAL_COMPLETION; > > goto fail; > > } > > @@ -934,7 +933,9 @@ usbd_probe_and_attach(struct device *parent, usbd_devi > > ifaces[i] = &dev->ifaces[i]; > > uaa.ifaces = ifaces; > > uaa.nifaces = nifaces; > > - len = (nifaces+1) * sizeof dv; > > + > > + /* add 1 for possible ugen and 1 for NULL terminator */ > > + len = (nifaces + 2) * sizeof dv; > > dev->subdevs = malloc(len, M_USB, M_NOWAIT | M_ZERO); > > if (dev->subdevs == NULL) { > > free(ifaces, M_USB); > > @@ -942,25 +943,31 @@ usbd_probe_and_attach(struct device *parent, usbd_devi > > goto fail; > > } > > > > - found = 0; > > for (i = 0; i < nifaces; i++) { > > - if (ifaces[i] == NULL) > > - continue; /* interface already claimed */ > > + if (usbd_iface_claimed(dev, i)) > > + continue; > > uaa.iface = ifaces[i]; > > uaa.ifaceno = ifaces[i]->idesc->bInterfaceNumber; > > dv = config_found_sm(parent, &uaa, usbd_print, > > usbd_submatch); > > - > > if (dv != NULL) { > > - dev->subdevs[found++] = dv; > > - ifaces[i] = NULL; /* consumed */ > > + dev->subdevs[dev->ndevs++] = dv; > > + usbd_claim_iface(dev, i); > > } > > } > > free(ifaces, M_USB); > > - if (found != 0) { > > - err = USBD_NORMAL_COMPLETION; > > - goto fail; > > + > > + if (dev->ndevs > 0) { > > + for (i = 0; i < nifaces; i++) { > > + if (!usbd_iface_claimed(dev, i)) > > + break; > > + } > > + if (i < nifaces) > > + goto generic; > > + else > > + goto fail; > > } > > + > > free(dev->subdevs, M_USB); > > dev->subdevs = 0; > > } > > @@ -971,20 +978,24 @@ usbd_probe_and_attach(struct device *parent, usbd_devi > > > > DPRINTF(("usbd_probe_and_attach: no interface drivers found\n")); > > > > +generic: > > /* Finally try the generic driver. */ > > uaa.iface = NULL; > > uaa.usegeneric = 1; > > - uaa.configno = UHUB_UNK_CONFIGURATION; > > + uaa.configno = dev->ndevs == 0 ? UHUB_UNK_CONFIGURATION : > > + dev->cdesc->bConfigurationValue; > > uaa.ifaceno = UHUB_UNK_INTERFACE; > > dv = config_found_sm(parent, &uaa, usbd_print, usbd_submatch); > > if (dv != NULL) { > > - dev->subdevs = malloc(2 * sizeof dv, M_USB, M_NOWAIT); > > - if (dev->subdevs == 0) { > > - err = USBD_NOMEM; > > - goto fail; > > + if (dev->ndevs == 0) { > > + dev->subdevs = malloc(2 * sizeof dv, M_USB, M_NOWAIT); > > + if (dev->subdevs == NULL) { > > + err = USBD_NOMEM; > > + goto fail; > > + } > > } > > - dev->subdevs[0] = dv; > > - dev->subdevs[1] = 0; > > + dev->subdevs[dev->ndevs++] = dv; > > + dev->subdevs[dev->ndevs] = 0; > > err = USBD_NORMAL_COMPLETION; > > goto fail; > > } > > Index: usbdi.c > > =================================================================== > > RCS file: /cvs/src/sys/dev/usb/usbdi.c,v > > retrieving revision 1.42 > > diff -u -p usbdi.c > > --- usbdi.c 30 Dec 2010 05:10:35 -0000 1.42 > > +++ usbdi.c 3 Jan 2011 21:52:28 -0000 > > @@ -117,6 +117,24 @@ usbd_ref_wait(usbd_device_handle dev) > > tsleep(&dev->ref_cnt, PWAIT, "usbref", hz * 60); > > } > > > > +int > > +usbd_get_devcnt(usbd_device_handle dev) > > +{ > > + return (dev->ndevs); > > +} > > + > > +void > > +usbd_claim_iface(usbd_device_handle dev, int ifaceidx) > > +{ > > + dev->ifaces[ifaceidx].claimed = 1; > > +} > > + > > +int > > +usbd_iface_claimed(usbd_device_handle dev, int ifaceidx) > > +{ > > + return (dev->ifaces[ifaceidx].claimed); > > +} > > + > > static __inline int > > usbd_xfer_isread(usbd_xfer_handle xfer) > > { > > Index: usbdi.h > > =================================================================== > > RCS file: /cvs/src/sys/dev/usb/usbdi.h,v > > retrieving revision 1.37 > > diff -u -p usbdi.h > > --- usbdi.h 30 Dec 2010 05:10:35 -0000 1.37 > > +++ usbdi.h 3 Jan 2011 21:52:28 -0000 > > @@ -165,6 +165,10 @@ usbd_status usbd_reload_device_desc(usbd_device_handle > > > > int usbd_ratecheck(struct timeval *last); > > > > +int usbd_get_devcnt(usbd_device_handle); > > +void usbd_claim_iface(usbd_device_handle, int); > > +int usbd_iface_claimed(usbd_device_handle, int); > > + > > int usbd_is_dying(usbd_device_handle); > > void usbd_deactivate(usbd_device_handle); > > > > Index: usbdivar.h > > =================================================================== > > RCS file: /cvs/src/sys/dev/usb/usbdivar.h,v > > retrieving revision 1.41 > > diff -u -p usbdivar.h > > --- usbdivar.h 30 Dec 2010 05:10:35 -0000 1.41 > > +++ usbdivar.h 3 Jan 2011 21:52:28 -0000 > > @@ -147,6 +147,7 @@ struct usbd_device { > > const struct usbd_quirks *quirks; /* device quirks, always set */ > > struct usbd_hub *hub; /* only if this is a hub */ > > struct device **subdevs; /* sub-devices, 0 terminated */ > > + int ndevs; /* # of subdevs */ > > }; > > > > struct usbd_interface { > > @@ -157,6 +158,7 @@ struct usbd_interface { > > struct usbd_endpoint *endpoints; > > void *priv; > > LIST_HEAD(, usbd_pipe) pipes; > > + u_int8_t claimed; > > }; > > > > struct usbd_pipe { > > Index: uvideo.c > > =================================================================== > > RCS file: /cvs/src/sys/dev/usb/uvideo.c,v > > retrieving revision 1.147 > > diff -u -p uvideo.c > > --- uvideo.c 24 Nov 2010 19:53:07 -0000 1.147 > > +++ uvideo.c 3 Jan 2011 21:52:28 -0000 > > @@ -468,13 +468,24 @@ uvideo_attach(struct device *parent, struct device *se > > { > > struct uvideo_softc *sc = (struct uvideo_softc *)self; > > struct usb_attach_arg *uaa = aux; > > + usb_interface_descriptor_t *id; > > + int i; > > > > sc->sc_udev = uaa->device; > > sc->sc_nifaces = uaa->nifaces; > > - sc->sc_ifaces = malloc(uaa->nifaces * sizeof(usbd_interface_handle), > > - M_USB, M_WAITOK); > > - bcopy(uaa->ifaces, sc->sc_ifaces, > > - uaa->nifaces * sizeof(usbd_interface_handle)); > > + /* > > + * Claim all video interfaces. Interfaces must be claimed during > > + * attach, during attach hooks is too late. > > + */ > > + for (i = 0; i < sc->sc_nifaces; i++) { > > + if (usbd_iface_claimed(sc->sc_udev, i)) > > + continue; > > + id = usbd_get_interface_descriptor(&sc->sc_udev->ifaces[i]); > > + if (id == NULL) > > + continue; > > + if (id->bInterfaceClass == UICLASS_VIDEO) > > + usbd_claim_iface(sc->sc_udev, i); > > + } > > > > /* maybe the device has quirks */ > > sc->sc_quirk = uvideo_lookup(uaa->vendor, uaa->product); > > @@ -557,8 +568,6 @@ uvideo_detach(struct device *self, int flags) > > struct uvideo_softc *sc = (struct uvideo_softc *)self; > > int rv = 0; > > > > - free(sc->sc_ifaces, M_USB); > > - > > /* Wait for outstanding requests to complete */ > > usbd_delay_ms(sc->sc_udev, UVIDEO_NFRAMES_MAX); > > > > @@ -837,12 +846,13 @@ uvideo_vs_parse_desc(struct uvideo_softc *sc, usb_conf > > for (i = 0; i < sc->sc_desc_vc_header.fix->bInCollection; i++) { > > iface = sc->sc_desc_vc_header.baInterfaceNr[i]; > > > > - id = usbd_get_interface_descriptor(sc->sc_ifaces[iface]); > > + id = usbd_get_interface_descriptor(&sc->sc_udev->ifaces[iface]); > > if (id == NULL) { > > printf("%s: can't get VS interface %d!\n", > > DEVNAME(sc), iface); > > return (USBD_INVAL); > > } > > + usbd_claim_iface(sc->sc_udev, iface); > > > > numalts = usbd_get_no_alts(cdesc, id->bInterfaceNumber); > > > > @@ -1197,7 +1207,7 @@ uvideo_vs_parse_desc_alt(struct uvideo_softc *sc, int > > > > /* save endpoint with largest bandwidth */ > > if (UGETW(ed->wMaxPacketSize) > vs->psize) { > > - vs->ifaceh = sc->sc_ifaces[iface]; > > + vs->ifaceh = &sc->sc_udev->ifaces[iface]; > > vs->endpoint = ed->bEndpointAddress; > > vs->numalts = numalts; > > vs->curalt = id->bAlternateSetting; > > > -- jake...@sdf.lonestar.org SDF Public Access UNIX System - http://sdf.lonestar.org
