Page MenuHomeFreeBSD

Support for bcm2838 RNG
Needs ReviewPublic

Authored by crowston_protonmail.com on Nov 22 2019, 12:11 AM.

Details

Reviewers
kevans
markm
stevek
Group Reviewers
csprng
Summary

The hardware RNG on the Raspberry Pi 4 differs slightly from the version found in the Pi 3. This patch extends the existing bcm2835_rng.c driver to function on the Pi4. However, I do not expose all of the register debug facilities that are available on the pi 3 driver.

Test Plan

Tested on my Pi 4, the numbers given appear random, but I have not had time to generate a statistical analysis of the collected numbers. @kevans stated he would check the driver still functions correctly on a Pi 3.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 28609
Build 26643: arc lint + arc unit

Event Timeline

markm added a comment.Nov 22 2019, 7:47 AM

Please upload diff with full context.

delphij updated this revision to Diff 66620.Sat, Jan 11, 9:30 AM
delphij added a subscriber: delphij.

Refresh diff with svn head.

Looks mostly fine to me.

sys/arm/broadcom/bcm2835/bcm2835_rng.c
116

Please make this static.

130

Ditto.

434

Maybe assert sc->conf != NULL here instead? (the probe method already returned ENXIO).

LGTM, modulo delphij's comments.

cem added a subscriber: cem.Sat, Jan 11, 9:10 PM

No real objections from me either; I'm unfamiliar with the hardware and the random(4) integration seems mostly good. Some nits and suggestions below.

sys/arm/broadcom/bcm2835/bcm2835_rng.c
116

And perhaps const

249

Should the RND_WARM_CNT mask be part of sc->conf?

327–331

I'm not sure this is ever what we want?

334–337

Is this something that can happen, and do we have reason to believe this procedure will fix the RNG?

Is it possible the RNG might continue producing output after we've observed zero bytes available? I.e., we're just consuming it too fast but it'll continue to produce entropy bytes at some rate?

356–358

I'd suggest wrapping this in a subroutine that submits multiple random_harvest_queue() invocations with at most HARVESTSIZE * sizeof(uint32_t) (i.e., 8) bytes per invocation. Otherwise, the high quality random data will be squashed together into at most 8 bytes, which is a pretty big waste if we're pulling e.g. 32 bytes from this device. Maybe random(4) should provide an API for high quality sources instead, I don't know.

(Or, uh, sizeof(((struct harvest_event *)0)->he_entropy), but that's kind of a big ugly expression.)

432

Maybe just (void *), especially if it unwraps the line.

434

+1

443–444

Should we default to 2x mode (but allow it to be disabled with the tunable? Is there some reason to believe 2x mode will be a worse default (higher power draw, perhaps)?

emaste added a subscriber: emaste.Wed, Jan 15, 8:20 PM

Who'll commit it?

kevans added inline comments.Wed, Jan 15, 8:24 PM
sys/arm/broadcom/bcm2835/bcm2835_rng.c
249

I'd be tempted to make it part of sc->conf and s/!sc->conf->can_diagnose/sc->conf->rnd_warm_cnt_mask != 0/ up above, removing can_diagnose.