atomic_thread_fence_ is fine with me, I just have to figure out which one means "wmb".

The conditions are both guarding against the host exposing 2 or more virtio-random devices. The driver will happily attach to both, but we want to reference only the first one from the global pointer and take care to only remove it when the same device is removed / detached (possible with devctl(8)).

Ah, I have it in my mind that I wrote code to avoid multiple virtio_random devices from attaching.

In the context of this change I think it would more sense to actually add that: why allow the attachment devices that will not be used? And avoid potential unexpected behavior: if the active one is detached, the remaining attached devices won't take over.

It doesn't fit the usual driver pattern to make policy decisions about limiting multiple instances of the same driver from attaching, if the backing devices are present. Sure, attaching a 2nd random device doesn't do much today (other than show up in devinfo); there's also little cost (IMO).

I think the unexpected behavior scenario above is so unlikely as to not be worth worrying about. The first unlikely thing that has to happen is for a hypervisor to expose >1 virtio-random devices to the guest. The second unlikely thing that has to happen is for the guest to manually detach one, but not all, of the virtio-rand devices. And the 3rd perhaps unlikely step is that a user who has created such a scenario would expect the remaining instances to take over random source responsibility.

I have some thoughts about how we might improve the random source API for modules such that they don't have to worry about these details, one way or the other. I'd prefer to leave that concern/policy to random(4), instead of spreading it across N drivers. In particular, it'd be nice if it was actually safe to unregister a random source at runtime.

I think a reasonable expected behavior would be all attached devices are used as sources. I agree that detaching one is unlikely but I guess I'd prefer to be cautious around anything security'ish related.

I think a reasonable expected behavior would be all attached devices are used as sources.

I agree in general, with some nuance. In this particular case, where the device only exists to provide entropy, I totally agree and will plan to make the change you proposed earlier.

The nuance bit is: I think that we might reasonably fail to meet this expectation for some devices under the current source-registration model. There exist drivers which are both a random source and provide some other functionality. For those specific drivers, it may make sense to attach even if the 2nd device will not be able to provide entropy to random(4) today.

A good redesign of the entropy registration model would allow all devices of the same "source" to input entropy sanely, and come and go without impacting other instances of the same device/source.

No. In practice HARVESTSIZE == 2 and sz == 8.

In a future revision, I plan to remove value entirely, shift the alignment responsibility into the random(4) code, and output directly into buf. But anything that touches random(4) has to go through secteam, so I wanted to keep that change separate and as small as possible.

Good catch, thanks!