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. > /* > * Copyright (c) 2006 Alexander Yurchenko <gra...@openbsd.org> > * Copyright (c) 2007 Marc Balmer <mbal...@openbsd.org> > * Copyright (C) 2015 attila <att...@stalphonsos.com> > * > * 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 :) > */ > > #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. > #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? > #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) > #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 :) > > 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. > 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(). > #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? > 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: > /* 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? > } > /* 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". > 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". > 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. > > 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. > sc->sc_ibulk = NULL; > } > > 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. 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? > 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); > }
