Thanks for your feedback, Martin. See below.
--On Monday, January 04, 2016 08:02:34 PM +0100 Martin Pieuchot
<[email protected]> wrote:
On 04/01/16(Mon) 10:34, Devin Reade wrote:
This patch adds kernel support for the OneRNG hardware random number
generator, which is similar to ualea(4). onerng(4) is configured to
deliver ~319kb/s of true randomness.
[snip]
+/*
+ * If ONERNG_MEASURE_RATE is defined, then we measure the rate at which
+ * we're able to provide random data to the kernel. ONERNG_RATE_SECONDS
+ * is the sampling period at which we print the results.
+ */
+#ifdef ONERNG_MEASURE_RATE
+#define ONERNG_RATE_SECONDS 30
+#endif
Could you also keep the softc variables used for measuring the rate
under the same define?
I thought I did. Those are sc_start, sc_cur, sc_counted_bytes, and
sc_timer_running. The sc_discarded_bytes is not related to the
measuring logic, so it is outside of the #ifdef. Did I miss something?
+/*
+ * OneRNG commands:
+ * cmdO (cap-oh) Turn the device on
+ * cmdo (lower-oh) Turn the device off
+ * cmdw Flush entropy pool
+ * cmdX Extract firmware image (the cmd4 opmode should
+ * be in effect during this operation)
+ *
+ * OneRNG operation modes:
+ * cmd0 (zero) Avalanche noise with whitener (default)
+ * cmd1 Raw avalanche noise
+ * cmd2 Avalanche noise and RF noise with whitener
+ * cmd3 Raw avalanche noise and RF noise
+ * cmd4 No noise
+ * cmd5 No noise
+ * cmd6 RF noise with whitener
+ * cmd7 Raw RF noise
+ */
Rather than having a big comment you could do:
/*
* Operation modes
*/
# define OP_RANDOM "cmd0\n" /* Avalanche noise with whitener (default) */
# define OP_RAW "cmd1\n"
...
Or simply include a link to the documentation and just define what you
need.
Unfortunately there isn't an obvious document at the original site
that lays out the various commands. I needed to read the Linux source
to derive the command set. That limits the usability of linking to the
original site for this purpose.
Since I don't give the option, under OpenBSD, of running in different
operating modes I decided that documenting it in the man page probably
wasn't appropriate, so that's why I left the command set description
in the source.
I could do a #define for each command with an inline comment, but that
would leave a bunch of unused #defines, which I thought would be more
evil than a single large comment.
I figured that documenting the entire set *somehow* was worthwhile.
With that additional context, what are your thoughts?
+/*
+ * Determine the index of data interface. Place it into data_iface_idx
+ * (or -1 if it isn't found)
+ */
+void
+onerng_get_caps(struct usb_attach_arg *uaa, int ctl_iface_no,
+ int *data_iface_idx /*, int *cm_cap, int *acm_cap*/)
+{
Drivers generally start with match, then attach,
ok
plus [onerng_get_caps()] seems useless, see below.
It gets invoked twice, once during match and once during attach.
During match, it acts as a concheck to ensure that both the control
and data interface exist, following the logic in umodem(4) from which
that function is derived. During the attach phase, it is used to
determine the index of the data interface, again following the logic
in umodem(4).
See also the response for the onerng_match feedback, below.
+int
+onerng_match(struct device *parent, void *match, void *aux)
+{
[...]
+ /* There are two interfaces; only match on the control interface. */
If you want to use the two interfaces, match the whole device, but to me
it seems that you're not using the two of them. What's the output of
"lsusb -v" for this device?
See the end of the email for the lsusb output.
Both are used. The control interface is used to clear RTS in
onerng_set_line_state(). The data interface is used to both set the
operating mode and to retrieve data.
An earlier version of the code (not submitted) matched on only vendor
and product, but I was seeing onerng_match being invoked twice; one for
the control interface and one for the data interface, so I was getting
two devices (onerng0 and onerng1). By matching on the control interface
and then using it to retrieve the data interface from the parent device
(and thus returning UMATCH_VENDOR_PRODUCT), we only get one onerng0
device.
+int
+onerng_enable(struct onerng_softc *sc)
+{
+ onerng_rts(sc, 0);
What happens if the transfer fail?
In this case, the OneRNG won't produce output, but point taken; the
underlying result should be checked and returned. If it fails, it
*should* deactivate the device.
+ /* Open pipes */
+ err = usbd_open_pipe(sc->sc_data_iface, ep_ibulk,
+ USBD_EXCLUSIVE_USE, &sc->sc_inpipe);
+ if (err) {
+ printf("%s: failed to open bulk-in pipe: %s\n",
+ DEVNAME(sc), usbd_errstr(err));
+ return;
return or goto fail?
fail.
ok for the rest of the observations; thanks.
lsusb -v follows:
Bus 001 Device 006: ID 1d50:6086 OpenMoko, Inc.
Device Descriptor:
bLength 18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass 2 Communications
bDeviceSubClass 0
bDeviceProtocol 0
bMaxPacketSize0 32
idVendor 0x1d50 OpenMoko, Inc.
idProduct 0x6086
bcdDevice 0.09
iManufacturer 1
iProduct 3
iSerial 3
bNumConfigurations 1
Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength 67
bNumInterfaces 2
bConfigurationValue 1
iConfiguration 0
bmAttributes 0x80
(Bus Powered)
MaxPower 200mA
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 0
bNumEndpoints 1
bInterfaceClass 2 Communications
bInterfaceSubClass 2 Abstract (modem)
bInterfaceProtocol 1 AT-commands (v.25ter)
iInterface 0
CDC Header:
bcdCDC 1.10
CDC ACM:
bmCapabilities 0x06
sends break
line coding and serial state
CDC Union:
bMasterInterface 0
bSlaveInterface 1
CDC Call Management:
bmCapabilities 0x00
bDataInterface 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0020 1x 32 bytes
bInterval 64
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 0
bNumEndpoints 2
bInterfaceClass 10 CDC Data
bInterfaceSubClass 0 Unused
bInterfaceProtocol 0
iInterface 4
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x85 EP 5 IN
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0040 1x 64 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x05 EP 5 OUT
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0040 1x 64 bytes
bInterval 1
