Martin Pieuchot <[email protected]> writes: > On 14/04/15(Tue) 07:40, attila wrote: >> [...] >> Feedback most welcome. > > See below. > >> /* -*- mode:c; tab-width:8; indent-tabs-mode:t; c-basic-offset:8 -*- */ > > We do not include editor settings in files, the first line should > contain: > > /* $OpenBSD$ */ > > Which will be expanded by CVS. >
Done. >> /* >> * Copyright (c) 2006 Alexander Yurchenko <[email protected]> >> * Copyright (c) 2007 Marc Balmer <[email protected]> >> * Copyright (C) 2015 attila <[email protected]> >> * >> * Permission to use, copy, modify, and distribute this software for any >> * purpose with or without fee is hereby granted, provided that the above >> * copyright notice and this permission notice appear in all copies. >> * >> * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES >> * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF >> * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR >> * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES >> * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN >> * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF >> * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. >> */ >> >> /* >> * Alea II TRNG. Produces 100kbit/sec of entropy by black magic >> * >> * Product information in English can be found here: >> * http://www.araneus.fi/products/alea2/en/ >> * >> * I only have an Alea II to play with but the documentation says >> * that the Alea I is the same, so they should also work. >> * >> * I cribbed liberally from both the uow and umbg drivers, both of >> * which are similar to this situation in different ways. > > The two last paragraphs do not add much information, think about what will > help you when you'll have to read this code again in a couple of years > :) > Ixnay to excess verbiage :-). Gone. >> */ >> >> #include <sys/param.h> >> #include <sys/systm.h> >> #include <sys/device.h> >> #include <sys/kernel.h> >> #include <sys/time.h> > > I believe you need <sys/time.h> just to make sure your driver works as > expected. You kept this code as #ifdef ALEA_DEBUG but does it really > help to debug something? Do you think it's worth keeping this code? > We try to not add too much verbosity to driver code. > The only DEBUG code I actually cared about was the bit that measured kbps actually delivered, and that was just to convince myself that the device did as the mfgr said. All ALEA_DEBUG gone now (and it was subtly wrong anyway :-). >> #include <sys/timeout.h> >> >> #include <dev/usb/usb.h> >> #include <dev/usb/usbdevs.h> >> #include <dev/usb/usbdi.h> >> #include <dev/usb/usbdi_util.h> >> >> #include <dev/rndvar.h> >> >> #define ALEA_IFACE 0 >> #define ALEA_MSECS 10 > > How did you choose 10msec? > Empirically, by cranking it down until I was seeing 100kbps all the time, however my method for doing this was wrong so my numbers were a little bogus. I did some more digging and it turns out each read from the ibulk pipe completes in just under 1000msec (990 or so). I've changed the timeout values to reflect this. >> #define ALEA_READ_TOUT 1100 > > Compared to a transfer timeout of 1,1 second is seems very short. By > the way we generally spell timeout TIMEOUT :o) > Spelling fixed everywhere :-). >> #define ALEA_BUFSIZ ((1024/8)*100) /* 100 kbits */ >> /*#define ALEA_DEBUG 1*/ /* comment out */ >> >> #define OURNAME(x) x->sc_dev.dv_xname > > We generally use DEVNAME(), look at how it is defined :) > Fixed. >> >> struct ualea_softc { >> struct device sc_dev; >> struct usbd_device *sc_udev; >> struct usbd_interface *sc_iface; >> struct usbd_pipe *sc_ibulk; >> struct timeout sc_tout; >> struct usb_task sc_task; >> #ifdef ALEA_DEBUG >> struct timespec sc_tattach; >> u_int32_t sc_nbits; >> #endif >> }; >> >> int ualea_match(struct device *, void *, void *); >> void ualea_attach(struct device *, struct device *, void *); >> int ualea_detach(struct device *, int); >> void ualea_task(void *); >> void ualea_intr(void *); >> >> struct cfdriver ualea_cd = { >> NULL, "ualea", DV_DULL >> }; >> >> const struct cfattach ualea_ca = { >> sizeof(struct ualea_softc), >> ualea_match, >> ualea_attach, >> ualea_detach >> }; >> >> static const struct usb_devno ualea_devs[] = { >> { USB_VENDOR_ARANEUS, USB_PRODUCT_ARANEUS_ALEA } >> }; > > Is it possible to match your device based on the content of the device > descriptor instead of whitelisting IDs? Whitelisting means that if the > company produce a compatible device with a new ID we'll need to modify > the driver. > Sadly, I don't think it is possible... you mean by looking at bDeviceClass/bDeviceSubClass/bDeviceProtocol? He only gives me zeroes there: ======================================================================== Bus 005 Device 002: ID 12d8:0001 Device Descriptor: bLength 18 bDescriptorType 1 bcdUSB 2.00 bDeviceClass 0 (Defined at Interface level) bDeviceSubClass 0 bDeviceProtocol 0 bMaxPacketSize0 64 idVendor 0x12d8 idProduct 0x0001 bcdDevice 2.04 iManufacturer 1 Araneus iProduct 2 Alea II TRNG iSerial 3 003709 bNumConfigurations 1 Configuration Descriptor: bLength 9 bDescriptorType 2 wTotalLength 32 bNumInterfaces 1 bConfigurationValue 1 iConfiguration 0 bmAttributes 0x80 (Bus Powered) MaxPower 50mA Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 0 bAlternateSetting 0 bNumEndpoints 2 bInterfaceClass 255 Vendor Specific Class bInterfaceSubClass 0 bInterfaceProtocol 255 iInterface 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x81 EP 1 IN bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0040 1x 64 bytes bInterval 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x82 EP 2 IN bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0040 1x 64 bytes bInterval 0 Device Status: 0x0000 (Bus Powered) ======================================================================== Perhaps I am misunderstanding; is there something else in there I could/should match on? I've changed the attach routine in the updated version to check vendor/product/iface, at least. >> int >> ualea_match(struct device *parent, void *match, void *aux) >> { >> struct usb_attach_arg *uaa = aux; >> >> if (uaa->iface != NULL) >> return (UMATCH_NONE); > > This line means that you're waiting for the USB stack to set the > first valid configuration for you, so you don't need most of the > code in *_attach(). > > I didn't get this at first, so I went and looked at the stuff in usb_subr.c (usbd_probe_and_attach()) and now I think I grok what happens a bit better. >> #ifdef ALEA_DEBUG >> if (uaa->vendor == USB_VENDOR_ARANEUS) >> printf("ualea: vendor 0x%x (%d) (ARANEUS) product 0x%x (%d)\n", >> uaa->vendor, uaa->vendor, uaa->product, uaa->product); >> #endif > > Please kill this debug chunk, you have the same information with > usbdevs(8). > > >> return ((usb_lookup(ualea_devs, uaa->vendor, uaa->product) != NULL) ? >> UMATCH_VENDOR_PRODUCT : UMATCH_NONE); >> } >> >> void >> ualea_attach(struct device *parent, struct device *self, void *aux) >> { >> struct ualea_softc *sc = (struct ualea_softc *)self; >> struct usb_attach_arg *uaa = aux; >> usb_interface_descriptor_t *id; >> usb_endpoint_descriptor_t *ed; >> int ep_ibulk = -1; >> usbd_status error; >> int i; >> >> sc->sc_udev = uaa->device; >> error = usbd_set_config_index(sc->sc_udev, 0, 1); > > This has already been done by the stack. If you're hardcoding the > interface index you'd better match it in *match(). > > How does your device descriptor looks like? Does it have multiple > configurations? > No, it doesn't, and I see why I shouldn't do this now. Gone. >> if (error != 0) { >> printf("%s: failed to set config: %s\n", >> OURNAME(sc), usbd_errstr(error)); > > If the previous call fail you're completely fucked because you already > attached the device, that's why usbd_set_config_index(9) should not be > called in *attach(). Sadly a lot of drivers do that, that's something > we should clean. > >> } >> error = usbd_device2interface_handle( >> sc->sc_udev, ALEA_IFACE, &sc->sc_iface); >> if (error != 0) { >> printf("%s: failed to get iface %d: %s\n", >> OURNAME(sc), ALEA_IFACE, usbd_errstr(error)); >> return; >> } > > Since you're checking for (uaa->iface != NULL) you do not need to do > that. Instead make sure that (uaa->ifaceno == ALEA_IFACE) in *match(), > then you can simply use "uaa->iface" in *attach() that's all you need > for: > Got it, fixed. >> /* Get endpoint (there can be only one) */ >> id = usbd_get_interface_descriptor(sc->sc_iface); >> for (i = 0; i < id->bNumEndpoints; i++) { >> ed = usbd_interface2endpoint_descriptor(sc->sc_iface, i); >> if (ed == NULL) { >> printf("%s: failed to get endpoint %d descriptor\n", >> OURNAME(sc), i); >> return; >> } >> >> #ifdef ALEA_DEBUG >> printf("%s: iface#%d endpoint#%d type %d\n", >> OURNAME(sc), i, UE_GET_DIR(ed->bEndpointAddress), >> UE_GET_XFERTYPE(ed->bmAttributes)); >> #endif /* ALEA_DEBUG */ > > Please remove this DEBUG chunk, you can have this information userland > tools like lsusb(1). > >> if (UE_GET_DIR(ed->bEndpointAddress) == UE_DIR_IN && >> UE_GET_XFERTYPE(ed->bmAttributes) == UE_BULK && >> ep_ibulk == -1) { >> ep_ibulk = ed->bEndpointAddress; >> #ifdef ALEA_DEBUG >> printf("%s: chosing ibulk endpoint#%d\n", >> OURNAME(sc), ep_ibulk); >> #else > > Please remove this DEBUG chunk too, it doesn't give any useful > information :) > >> break; >> #endif /* ALEA_DEBUG */ >> } >> } >> if (ep_ibulk == -1) { >> printf("%s: missing ibulk endpoint - cannot initialize\n", >> OURNAME(sc)); >> return; > > Can you simply print "%s: missing endpoint\n" so we share the string > with other drivers? > Done. >> } >> /* Open pipe */ > > This comment is redundant with the code. > >> error = usbd_open_pipe(sc->sc_iface, ep_ibulk, USBD_EXCLUSIVE_USE, >> &sc->sc_ibulk); > > Since you have only one pipe, I'd suggest name your variable "sc_pipe". > Also done. >> if (error) { >> printf("%s: failed to open bulk-in pipe: %s\n", >> OURNAME(sc), usbd_errstr(error)); >> return; >> } >> /* Get our task going */ >> usb_init_task(&sc->sc_task, ualea_task, sc, USB_TASK_TYPE_GENERIC); >> timeout_set(&sc->sc_tout, ualea_intr, sc); > > I'd rename "sc_tout" into "sc_timeout" and "ualea_intr" in > "ualea_timeout". > Done. >> timeout_add_msec(&sc->sc_tout, ALEA_MSECS); >> #ifdef ALEA_DEBUG >> nanotime(&sc->sc_tattach); >> sc->sc_nbits = 0; >> #endif /* ALEA_DEBUG */ >> } >> >> int >> ualea_detach(struct device *self, int flags) >> { >> struct ualea_softc *sc = (struct ualea_softc *)self; > > First thing first, please call usb_rem_task() here otherwise you might > still have a task on the queue when you unplug the device. > Done. >> >> if (timeout_initialized(&sc->sc_tout)) >> timeout_del(&sc->sc_tout); >> if (sc->sc_ibulk != NULL) { >> usbd_abort_pipe(sc->sc_ibulk); >> usbd_close_pipe(sc->sc_ibulk); > > Close implies abort, so it is safe to only call close. > Yes, okay. >> sc->sc_ibulk = NULL; I looked and it appears that M_ZERO is used when allocating the memory for all of these structures, so I take it that explicit zeroing of things when tearing down is neither required nor desired? I removed this kind of thing from ualea.c because it looked like it wasn't necessary. >> } >> >> return 0; >> } >> >> void >> ualea_task(void *arg) >> { >> struct ualea_softc *sc = (struct ualea_softc *)arg; >> usbd_status error; >> u_int32_t n, i; >> int *buf; >> struct usbd_xfer *xfer; >> #ifdef ALEA_DEBUG >> u_int32_t kbps, dt, nbits; >> struct timespec now; >> #endif /* ALEA_DEBUG */ >> >> xfer = usbd_alloc_xfer(sc->sc_udev); >> if (xfer == NULL) { >> printf("%s: could not alloc xfer\n", OURNAME(sc)); >> goto bail; >> } > > You'd better allocate your xfer in *attach() an reuse it every time. > >> /* Is there any chance this buffer is not at least int-aligned? */ > > Why such a question? > >> buf = usbd_alloc_buffer(xfer, ALEA_BUFSIZ); > > Same thing about your buffer (sc_buf), no need for a new allocation every > 10ms. > Allocations moved to attach. > This should be done every time. >> usbd_setup_xfer(xfer, sc->sc_ibulk, NULL, (void *)buf, >> ALEA_BUFSIZ, USBD_SHORT_XFER_OK | USBD_SYNCHRONOUS, >> ALEA_READ_TOUT, NULL); >> error = usbd_transfer(xfer); >> if (error) { >> printf("%s: xfer failed: %s\n", OURNAME(sc), >> usbd_errstr(error)); >> goto bail; >> } >> usbd_get_xfer_status(xfer, NULL, NULL, &n, NULL); > > What about using "len" for the length like the rest of the tree? > Done. >> if (n < sizeof(int)) { >> printf("%s: xfer too short (%u bytes) - dropping\n", >> OURNAME(sc), n); >> goto bail; >> } >> #ifdef ALEA_DEBUG >> else if (n != ALEA_BUFSIZ) >> printf("%s: short read got %u bytes (exp. %u)\n", OURNAME(sc), >> n, ALEA_BUFSIZ); >> #endif /* ALEA_DEBUG */ > > This chunk like the one below is more developing code, not really debug code, > please drop it :) > >> n /= sizeof(int); >> /* >> * A random(|ness) koan: >> * children chug entropy like thirsty rhinos >> * surfing at the mall >> * privacy died in their hands >> */ >> for (i = 0; i < n; i++) >> add_true_randomness(buf[i]); >> #ifdef ALEA_DEBUG >> nanotime(&now); >> dt = now.tv_sec - sc->sc_tattach.tv_sec; >> nbits = n * sizeof(int) * 8; >> sc->sc_nbits += nbits; >> kbps = dt ? ((sc->sc_nbits / 1024) / dt) : 0; >> printf("%s: added %d bits: %dkbit/sec\n", OURNAME(sc), nbits, kbps); >> #endif /* ALEA_DEBUG */ >> bail: >> if (xfer) >> usbd_free_xfer(xfer); >> timeout_add_msec(&sc->sc_tout, ALEA_MSECS); >> } >> >> void >> ualea_intr(void *arg) >> { >> struct ualea_softc *sc = arg; >> >> usb_add_task(sc->sc_udev, &sc->sc_task); >> } I'm attaching the updated driver. Thank you for the critique. I suppose I need to write a man page for this as well... working on it. Pax, -A -- [email protected] | http://trac.haqistan.net/~attila keyid E6CC1EDB | 4D91 1B98 A210 1D71 2A0E AC29 9677 D0A6 E6CC 1EDB
/* $OpenBSD$ */ /* * Copyright (c) 2006 Alexander Yurchenko <[email protected]> * Copyright (c) 2007 Marc Balmer <[email protected]> * Copyright (C) 2015 attila <[email protected]> * * Permission to use, copy, modify, and distribute this software for any * purpose with or without fee is hereby granted, provided that the above * copyright notice and this permission notice appear in all copies. * * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ /* * Alea II TRNG. Produces 100kbit/sec of entropy by black magic * * Product information in English can be found here: * http://www.araneus.fi/products/alea2/en/ */ #include <sys/param.h> #include <sys/systm.h> #include <sys/device.h> #include <sys/kernel.h> #include <sys/timeout.h> #include <dev/usb/usb.h> #include <dev/usb/usbdevs.h> #include <dev/usb/usbdi.h> #include <dev/usb/usbdi_util.h> #include <dev/rndvar.h> #define ALEA_IFACE 0 #define ALEA_MSECS 100 #define ALEA_READ_TIMEOUT 1000 #define ALEA_BUFSIZ ((1024/8)*100) /* 100 kbits */ #define DEVNAME(_sc) ((_sc)->sc_dev.dv_xname) struct ualea_softc { struct device sc_dev; struct usbd_device *sc_udev; struct usbd_pipe *sc_pipe; struct timeout sc_timeout; struct usb_task sc_task; struct usbd_xfer *sc_xfer; int *sc_buf; }; int ualea_match(struct device *, void *, void *); void ualea_attach(struct device *, struct device *, void *); int ualea_detach(struct device *, int); void ualea_task(void *); void ualea_timeout(void *); struct cfdriver ualea_cd = { NULL, "ualea", DV_DULL }; const struct cfattach ualea_ca = { sizeof(struct ualea_softc), ualea_match, ualea_attach, ualea_detach }; int ualea_match(struct device *parent, void *match, void *aux) { struct usb_attach_arg *uaa = aux; if (uaa->iface == NULL) return (UMATCH_NONE); if ((uaa->vendor == USB_VENDOR_ARANEUS) && (uaa->product == USB_PRODUCT_ARANEUS_ALEA) && (uaa->ifaceno == ALEA_IFACE)) return (UMATCH_VENDOR_PRODUCT); return (UMATCH_NONE); } void ualea_attach(struct device *parent, struct device *self, void *aux) { struct ualea_softc *sc = (struct ualea_softc *)self; struct usb_attach_arg *uaa = aux; usb_interface_descriptor_t *id; usb_endpoint_descriptor_t *ed; int ep_ibulk = -1; usbd_status error; int i; sc->sc_udev = uaa->device; id = usbd_get_interface_descriptor(uaa->iface); for (i = 0; i < id->bNumEndpoints; i++) { ed = usbd_interface2endpoint_descriptor(uaa->iface, i); if (ed == NULL) { printf("%s: failed to get endpoint %d descriptor\n", DEVNAME(sc), i); return; } if (UE_GET_DIR(ed->bEndpointAddress) == UE_DIR_IN && UE_GET_XFERTYPE(ed->bmAttributes) == UE_BULK) { ep_ibulk = ed->bEndpointAddress; break; } } if (ep_ibulk == -1) { printf("%s: missing endpoint\n", DEVNAME(sc)); return; } error = usbd_open_pipe(uaa->iface, ep_ibulk, USBD_EXCLUSIVE_USE, &sc->sc_pipe); if (error) { printf("%s: failed to open bulk-in pipe: %s\n", DEVNAME(sc), usbd_errstr(error)); return; } sc->sc_xfer = usbd_alloc_xfer(sc->sc_udev); if (sc->sc_xfer == NULL) { printf("%s: could not alloc xfer\n", DEVNAME(sc)); return; } sc->sc_buf = usbd_alloc_buffer(sc->sc_xfer, ALEA_BUFSIZ); if (sc->sc_buf == NULL) { printf("%s: could not alloc %d-byte buffer\n", DEVNAME(sc), ALEA_BUFSIZ); return; } usb_init_task(&sc->sc_task, ualea_task, sc, USB_TASK_TYPE_GENERIC); timeout_set(&sc->sc_timeout, ualea_timeout, sc); usb_add_task(sc->sc_udev, &sc->sc_task); } int ualea_detach(struct device *self, int flags) { struct ualea_softc *sc = (struct ualea_softc *)self; usb_rem_task(sc->sc_udev, &sc->sc_task); if (timeout_initialized(&sc->sc_timeout)) timeout_del(&sc->sc_timeout); if (sc->sc_xfer) usbd_free_xfer(sc->sc_xfer); if (sc->sc_pipe != NULL) usbd_close_pipe(sc->sc_pipe); return (0); } void ualea_task(void *arg) { struct ualea_softc *sc = (struct ualea_softc *)arg; usbd_status error; u_int32_t len, i; usbd_setup_xfer(sc->sc_xfer, sc->sc_pipe, NULL, sc->sc_buf, ALEA_BUFSIZ, USBD_SHORT_XFER_OK | USBD_SYNCHRONOUS, ALEA_READ_TIMEOUT, NULL); error = usbd_transfer(sc->sc_xfer); if (error) { printf("%s: xfer failed: %s\n", DEVNAME(sc), usbd_errstr(error)); goto bail; } usbd_get_xfer_status(sc->sc_xfer, NULL, NULL, &len, NULL); if (len < sizeof(int)) { printf("%s: xfer too short (%u bytes) - dropping\n", DEVNAME(sc), len); goto bail; } len /= sizeof(int); /* * A random(|ness) koan: * children chug entropy like thirsty rhinos * surfing at the mall * privacy died in their hands */ for (i = 0; i < len; i++) add_true_randomness(sc->sc_buf[i]); bail: timeout_add_msec(&sc->sc_timeout, ALEA_MSECS); } void ualea_timeout(void *arg) { struct ualea_softc *sc = arg; usb_add_task(sc->sc_udev, &sc->sc_task); }
