Page MenuHomeFreeBSD
Differential D22493 Diff 85265 sys/arm/broadcom/bcm2835/bcm2835_rng.c

Changeset View

Standalone View

View Options

sys/arm/broadcom/bcm2835/bcm2835_rng.c

Show First 20 Lines • Show All 63 Lines • ▼ Show 20 Lines
#define RNG_JCLK_BYP_DIV_CNT 0x0000ff00 /* Jitter clk bypass divider #define RNG_JCLK_BYP_DIV_CNT 0x0000ff00 /* Jitter clk bypass divider
count */ count */
#define RNG_JCLK_BYP_DIV_CNT_SHIFT 8 #define RNG_JCLK_BYP_DIV_CNT_SHIFT 8
#define RNG_JCLK_BYP_SRC 0x00000020 /* Jitter clk bypass source */ #define RNG_JCLK_BYP_SRC 0x00000020 /* Jitter clk bypass source */
#define RNG_JCLK_BYP_SEL 0x00000010 /* Jitter clk bypass select */ #define RNG_JCLK_BYP_SEL 0x00000010 /* Jitter clk bypass select */
#define RNG_RBG2X 0x00000002 /* RBG 2X SPEED */ #define RNG_RBG2X 0x00000002 /* RBG 2X SPEED */
#define RNG_RBGEN_BIT 0x00000001 /* Enable RNG bit */ #define RNG_RBGEN_BIT 0x00000001 /* Enable RNG bit */
#define RNG_STATUS 0x04 /* RNG status register */ #define BCM2835_RNG_STATUS 0x04 /* BCM2835 RNG status register */
#define RND_VAL_SHIFT 24 /* Shift for valid words */ #define BCM2838_RNG_STATUS 0x18 /* BCM2838 RNG status register */
#define RND_VAL_MASK 0x000000ff /* Number valid words mask */
#define RND_VAL_WARM_CNT 0x40000 /* RNG Warm Up count */
#define RND_WARM_CNT 0xfffff /* RNG Warm Up Count mask */
#define RNG_DATA 0x08 /* RNG Data Register */ #define BCM2838_RNG_COUNT 0x24 /* How many values available */
#define BCM2838_COUNT_VAL_MASK 0x000000ff
#define BCM2835_RND_VAL_SHIFT 24 /* Shift for valid words */
cemUnsubmitted
Not Done
Inline Actions

Usually we put const before the underlying type name (const struct bcm_rng_conf *). No functional difference. Style(9) also wants a space before the *.

cem: Usually we put `const` before the underlying type name (`const struct bcm_rng_conf *`). No…
#define BCM2835_RND_VAL_MASK 0x000000ff /* Number valid words mask */
#define BCM2835_RND_VAL_WARM_CNT 0x40000 /* RNG Warm Up count */
#define BCM2835_RND_WARM_CNT 0xfffff /* RNG Warm Up Count mask */
#define BCM2835_RNG_DATA 0x08 /* RNG Data Register */
#define BCM2838_RNG_DATA 0x20
#define RNG_FF_THRES 0x0c #define RNG_FF_THRES 0x0c
#define RNG_FF_THRES_MASK 0x0000001f #define RNG_FF_THRES_MASK 0x0000001f
#define RNG_INT_MASK 0x10 #define BCM2835_RNG_INT_MASK 0x10
#define RNG_INT_OFF_BIT 0x00000001 #define BCM2835_RNG_INT_OFF_BIT 0x00000001
#define RNG_FF_DEFAULT 0x10 /* FIFO threshold default */ #define RNG_FF_DEFAULT 0x10 /* FIFO threshold default */
#define RNG_FIFO_WORDS (RNG_FF_DEFAULT / sizeof(uint32_t)) #define RNG_FIFO_WORDS (RNG_FF_DEFAULT / sizeof(uint32_t))
#define RNG_NUM_OSCILLATORS 6 #define RNG_NUM_OSCILLATORS 6
#define RNG_STALL_COUNT_DEFAULT 10 #define RNG_STALL_COUNT_DEFAULT 10
#define RNG_CALLOUT_TICKS (hz * 4) #define RNG_CALLOUT_TICKS (hz * 4)
struct bcm_rng_conf {
bus_size_t control_reg;
bus_size_t status_reg;
bus_size_t count_reg;
bus_size_t data_reg;
bus_size_t intr_mask_reg;
uint32_t intr_disable_bit;
uint32_t count_value_shift;
uint32_t count_value_mask;
uint32_t warmup_count;
bool allow_2x_mode;
bool can_diagnose;
/* XXX diag regs */
};
static const struct bcm_rng_conf bcm2835_rng_conf = {
.control_reg = RNG_CTRL,
.status_reg = BCM2835_RNG_STATUS,
delphijUnsubmitted
Not Done
Inline Actions

Please make this static.

delphij: Please make this static.
cemUnsubmitted
Not Done
Inline Actions

And perhaps const

cem: And perhaps `const`
.count_reg = BCM2835_RNG_STATUS, /* Same register */
.data_reg = BCM2835_RNG_DATA,
.intr_mask_reg = BCM2835_RNG_INT_MASK,
.intr_disable_bit = BCM2835_RNG_INT_OFF_BIT,
.count_value_shift = BCM2835_RND_VAL_SHIFT,
.count_value_mask = BCM2835_RND_VAL_MASK,
.warmup_count = BCM2835_RND_VAL_WARM_CNT,
.allow_2x_mode = true,
.can_diagnose = true
};
static const struct bcm_rng_conf bcm2838_rng_conf = {
.control_reg = RNG_CTRL,
.status_reg = BCM2838_RNG_STATUS,
delphijUnsubmitted
Not Done
Inline Actions

Ditto.

delphij: Ditto.
.count_reg = BCM2838_RNG_COUNT,
.data_reg = BCM2838_RNG_DATA,
.intr_mask_reg = 0,
.intr_disable_bit = 0,
.count_value_shift = 0,
.count_value_mask = BCM2838_COUNT_VAL_MASK,
.warmup_count = 0,
.allow_2x_mode = false,
.can_diagnose = false
};
struct bcm2835_rng_softc { struct bcm2835_rng_softc {
device_t sc_dev; device_t sc_dev;
struct resource * sc_mem_res; struct resource * sc_mem_res;
struct resource * sc_irq_res; struct resource * sc_irq_res;
void * sc_intr_hdl; void * sc_intr_hdl;
struct bcm_rng_conf const* conf;
cemUnsubmitted
Done
Inline Actions

This probably needs to be a const pointer, or I doubt this will compile.

cem: This probably needs to be a `const` pointer, or I doubt this will compile.
uint32_t sc_buf[RNG_FIFO_WORDS]; uint32_t sc_buf[RNG_FIFO_WORDS];
struct callout sc_rngto; struct callout sc_rngto;
int sc_stall_count; int sc_stall_count;
int sc_rbg2x; int sc_rbg2x;
long sc_underrun; long sc_underrun;
}; };
static struct ofw_compat_data compat_data[] = { static struct ofw_compat_data compat_data[] = {
{"broadcom,bcm2835-rng", 1}, {"broadcom,bcm2835-rng", (uintptr_t)&bcm2835_rng_conf},
{"brcm,bcm2835-rng", 1}, {"brcm,bcm2835-rng", (uintptr_t)&bcm2835_rng_conf},
{"brcm,bcm2711-rng200", (uintptr_t)&bcm2838_rng_conf},
{"brcm,bcm2838-rng", (uintptr_t)&bcm2838_rng_conf},
{"brcm,bcm2838-rng200", (uintptr_t)&bcm2838_rng_conf},
{"brcm,bcm7211-rng", (uintptr_t)&bcm2838_rng_conf},
{"brcm,bcm7278-rng", (uintptr_t)&bcm2838_rng_conf},
{"brcm,iproc-rng200", (uintptr_t)&bcm2838_rng_conf},
{NULL, 0} {NULL, 0}
}; };
static __inline void static __inline void
bcm2835_rng_stat_inc_underrun(struct bcm2835_rng_softc *sc) bcm2835_rng_stat_inc_underrun(struct bcm2835_rng_softc *sc)
{ {
atomic_add_long(&sc->sc_underrun, 1); atomic_add_long(&sc->sc_underrun, 1);
} }
static __inline uint32_t static __inline uint32_t
cemUnsubmitted
Not Done
Inline Actions

style(9) nit: space between void and *

cem: style(9) nit: space between `void` and `*`
bcm2835_rng_read4(struct bcm2835_rng_softc *sc, bus_size_t off) bcm2835_rng_read4(struct bcm2835_rng_softc *sc, bus_size_t off)
{ {
return bus_read_4(sc->sc_mem_res, off); return bus_read_4(sc->sc_mem_res, off);
} }
static __inline void static __inline void
bcm2835_rng_read_multi4(struct bcm2835_rng_softc *sc, bus_size_t off, bcm2835_rng_read_multi4(struct bcm2835_rng_softc *sc, bus_size_t off,
Show All 11 Lines
} }
static void static void
bcm2835_rng_dump_registers(struct bcm2835_rng_softc *sc, struct sbuf *sbp) bcm2835_rng_dump_registers(struct bcm2835_rng_softc *sc, struct sbuf *sbp)
{ {
uint32_t comblk2_osc, comblk1_osc, jclk_byp_div, val; uint32_t comblk2_osc, comblk1_osc, jclk_byp_div, val;
int i; int i;
if (!sc->conf->can_diagnose)
/* Not implemented. */
return;
/* Display RNG control register contents */ /* Display RNG control register contents */
val = bcm2835_rng_read4(sc, RNG_CTRL); val = bcm2835_rng_read4(sc, sc->conf->control_reg);
sbuf_printf(sbp, "RNG_CTRL (%08x)\n", val); sbuf_printf(sbp, "RNG_CTRL (%08x)\n", val);
comblk2_osc = (val & RNG_COMBLK2_OSC) >> RNG_COMBLK2_OSC_SHIFT; comblk2_osc = (val & RNG_COMBLK2_OSC) >> RNG_COMBLK2_OSC_SHIFT;
sbuf_printf(sbp, " RNG_COMBLK2_OSC (%02x)\n", comblk2_osc); sbuf_printf(sbp, " RNG_COMBLK2_OSC (%02x)\n", comblk2_osc);
for (i = 0; i < RNG_NUM_OSCILLATORS; i++) for (i = 0; i < RNG_NUM_OSCILLATORS; i++)
if ((comblk2_osc & (1 << i)) == 0) if ((comblk2_osc & (1 << i)) == 0)
sbuf_printf(sbp, " Oscillator %d enabled\n", i + 1); sbuf_printf(sbp, " Oscillator %d enabled\n", i + 1);
Show All 9 Lines sbuf_printf(sbp,
" RNG_JCLK_BYP_DIV_CNT (%02x)\n APB clock frequency / %d\n", " RNG_JCLK_BYP_DIV_CNT (%02x)\n APB clock frequency / %d\n",
jclk_byp_div, 2 * (jclk_byp_div + 1)); jclk_byp_div, 2 * (jclk_byp_div + 1));
sbuf_printf(sbp, " RNG_JCLK_BYP_SRC:\n %s\n", sbuf_printf(sbp, " RNG_JCLK_BYP_SRC:\n %s\n",
(val & RNG_JCLK_BYP_SRC) ? "Use divided down APB clock" : (val & RNG_JCLK_BYP_SRC) ? "Use divided down APB clock" :
"Use RNG clock (APB clock)"); "Use RNG clock (APB clock)");
sbuf_printf(sbp, " RNG_JCLK_BYP_SEL:\n %s\n", sbuf_printf(sbp, " RNG_JCLK_BYP_SEL:\n %s\n",
(val & RNG_JCLK_BYP_SEL) ? "Bypass internal jitter clock" : (val & RNG_JCLK_BYP_SEL) ? "Bypass internal jitter clock" :
cemUnsubmitted
Not Done
Inline Actions

Style nit: const void *

cem: Style nit: `const void *`
"Use internal jitter clock"); "Use internal jitter clock");
if ((val & RNG_RBG2X) != 0) if ((val & RNG_RBG2X) != 0)
sbuf_cat(sbp, " RNG_RBG2X: RNG 2X SPEED enabled\n"); sbuf_cat(sbp, " RNG_RBG2X: RNG 2X SPEED enabled\n");
if ((val & RNG_RBGEN_BIT) != 0) if ((val & RNG_RBGEN_BIT) != 0)
sbuf_cat(sbp, " RNG_RBGEN_BIT: RBG enabled\n"); sbuf_cat(sbp, " RNG_RBGEN_BIT: RBG enabled\n");
/* Display RNG status register contents */ /* Display RNG status register contents */
val = bcm2835_rng_read4(sc, RNG_STATUS); val = bcm2835_rng_read4(sc, sc->conf->status_reg);
sbuf_printf(sbp, "RNG_CTRL (%08x)\n", val); sbuf_printf(sbp, "RNG_CTRL (%08x)\n", val);
sbuf_printf(sbp, " RND_VAL: %02x\n", sbuf_printf(sbp, " RND_VAL: %02x\n",
(val >> RND_VAL_SHIFT) & RND_VAL_MASK); (val >> sc->conf->count_value_shift) & sc->conf->count_value_mask);
sbuf_printf(sbp, " RND_WARM_CNT: %05x\n", val & RND_WARM_CNT); sbuf_printf(sbp, " RND_WARM_CNT: %05x\n", val & sc->conf->warmup_count);
cemUnsubmitted
Not Done
Inline Actions

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

cem: Should the RND_WARM_CNT mask be part of `sc->conf`?
kevansUnsubmitted
Not Done
Inline Actions

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.

kevans: I'd be tempted to make it part of `sc->conf` and s/`!sc->conf->can_diagnose`/`sc->conf…
/* Display FIFO threshold register contents */ /* Display FIFO threshold register contents */
val = bcm2835_rng_read4(sc, RNG_FF_THRES); val = bcm2835_rng_read4(sc, RNG_FF_THRES);
sbuf_printf(sbp, "RNG_FF_THRES: %05x\n", val & RNG_FF_THRES_MASK); sbuf_printf(sbp, "RNG_FF_THRES: %05x\n", val & RNG_FF_THRES_MASK);
/* Display interrupt mask register contents */ /* Display interrupt mask register contents */
val = bcm2835_rng_read4(sc, RNG_INT_MASK); val = bcm2835_rng_read4(sc, sc->conf->intr_mask_reg);
sbuf_printf(sbp, "RNG_INT_MASK: interrupt %s\n", sbuf_printf(sbp, "RNG_INT_MASK: interrupt %s\n",
((val & RNG_INT_OFF_BIT) != 0) ? "disabled" : "enabled"); ((val & sc->conf->intr_disable_bit) != 0) ? "disabled" : "enabled");
} }
static void static void
bcm2835_rng_disable_intr(struct bcm2835_rng_softc *sc) bcm2835_rng_disable_intr(struct bcm2835_rng_softc *sc)
{ {
uint32_t mask; uint32_t mask;
/* Set the interrupt off bit in the interrupt mask register */ /* Set the interrupt off bit in the interrupt mask register */
mask = bcm2835_rng_read4(sc, RNG_INT_MASK); mask = bcm2835_rng_read4(sc, sc->conf->intr_mask_reg);
mask |= RNG_INT_OFF_BIT; mask |= sc->conf->intr_disable_bit;
bcm2835_rng_write4(sc, RNG_INT_MASK, mask); bcm2835_rng_write4(sc, sc->conf->intr_mask_reg, mask);
} }
static void static void
bcm2835_rng_start(struct bcm2835_rng_softc *sc) bcm2835_rng_start(struct bcm2835_rng_softc *sc)
{ {
uint32_t ctrl; uint32_t ctrl;
/* Disable the interrupt */ /* Disable the interrupt */
if (sc->conf->intr_mask_reg)
bcm2835_rng_disable_intr(sc); bcm2835_rng_disable_intr(sc);
/* Set the warmup count */ /* Set the warmup count */
bcm2835_rng_write4(sc, RNG_STATUS, RND_VAL_WARM_CNT); if (sc->conf->warmup_count > 0)
bcm2835_rng_write4(sc, sc->conf->status_reg,
sc->conf->warmup_count);
/* Enable the RNG */ /* Enable the RNG */
ctrl = bcm2835_rng_read4(sc, RNG_CTRL); ctrl = bcm2835_rng_read4(sc, sc->conf->control_reg);
ctrl |= RNG_RBGEN_BIT; ctrl |= RNG_RBGEN_BIT;
if (sc->sc_rbg2x) if (sc->sc_rbg2x && sc->conf->allow_2x_mode)
ctrl |= RNG_RBG2X; ctrl |= RNG_RBG2X;
bcm2835_rng_write4(sc, RNG_CTRL, ctrl); bcm2835_rng_write4(sc, sc->conf->control_reg, ctrl);
} }
static void static void
bcm2835_rng_stop(struct bcm2835_rng_softc *sc) bcm2835_rng_stop(struct bcm2835_rng_softc *sc)
{ {
uint32_t ctrl; uint32_t ctrl;
/* Disable the RNG */ /* Disable the RNG */
ctrl = bcm2835_rng_read4(sc, RNG_CTRL); ctrl = bcm2835_rng_read4(sc, sc->conf->control_reg);
ctrl &= ~RNG_RBGEN_BIT; ctrl &= ~RNG_RBGEN_BIT;
bcm2835_rng_write4(sc, RNG_CTRL, ctrl); bcm2835_rng_write4(sc, sc->conf->control_reg, ctrl);
} }
static void static void
bcm2835_rng_enqueue_harvest(struct bcm2835_rng_softc *sc, uint32_t nread)
{
char *sc_buf_chunk;
uint32_t chunk_size;
uint32_t cnt;
chunk_size = sizeof(((struct harvest_event *)0)->he_entropy);
cnt = nread * sizeof(uint32_t);
sc_buf_chunk = (void*)sc->sc_buf;
while (cnt > 0) {
uint32_t size;
cemUnsubmitted
Done
Inline Actions

Probably makes sense to use uint32_t for size, too, given it is assigned either cnt or chunk_size (both uint32_t). (Yes, these are the same size on all FreeBSD archs, but I think it is best to be consistent.)

cem: Probably makes sense to use `uint32_t` for `size`, too, given it is assigned either `cnt` or…
size = MIN(cnt, chunk_size);
cemUnsubmitted
Not Done
Inline Actions

sometimes spelled MIN(cnt, chunk_size) (sys/param.h).

cem: sometimes spelled `MIN(cnt, chunk_size)` (sys/param.h).
random_harvest_queue(sc_buf_chunk, size, RANDOM_PURE_BROADCOM);
sc_buf_chunk += size;
cemUnsubmitted
Done
Inline Actions

This math is wrong. Due to pointer arithmetic rules, this will increment sc_buf_chunk by a factor of sizeof(uint32_t) more than you intend.

I would suggest just using char *sc_buf_chunk and perhaps initializing it with a (void *)sc->sc_buf cast.

cem: This math is wrong. Due to pointer arithmetic rules, this will increment sc_buf_chunk by a…
me_jamesrm.comAuthorUnsubmitted
Done
Inline Actions

Oh, great catch! Thanks.

me_jamesrm.com: Oh, great catch! Thanks.
cnt -= size;
}
}
static void
bcm2835_rng_harvest(void *arg) bcm2835_rng_harvest(void *arg)
{ {
uint32_t *dest; uint32_t *dest;
uint32_t status; uint32_t hwcount;
u_int cnt, nread, num_avail, num_words; u_int cnt, nread, num_avail, num_words;
int seen_underrun, num_stalls; int seen_underrun, num_stalls;
struct bcm2835_rng_softc *sc = arg; struct bcm2835_rng_softc *sc = arg;
dest = sc->sc_buf; dest = sc->sc_buf;
nread = num_words = 0; nread = num_words = 0;
seen_underrun = num_stalls = 0; seen_underrun = num_stalls = 0;
for (cnt = sizeof(sc->sc_buf) / sizeof(uint32_t); cnt > 0; for (cnt = sizeof(sc->sc_buf) / sizeof(uint32_t); cnt > 0;
cnt -= num_words) { cnt -= num_words) {
/* Read status register to find out how many words available */ /* Read count register to find out how many words available */
status = bcm2835_rng_read4(sc, RNG_STATUS); hwcount = bcm2835_rng_read4(sc, sc->conf->count_reg);
num_avail = (status >> RND_VAL_SHIFT) & RND_VAL_MASK; num_avail = (hwcount >> sc->conf->count_value_shift) &
sc->conf->count_value_mask;
/* If we have none... */ /* If we have none... */
if (num_avail == 0) { if (num_avail == 0) {
bcm2835_rng_stat_inc_underrun(sc); bcm2835_rng_stat_inc_underrun(sc);
if (++seen_underrun >= sc->sc_stall_count) { if (++seen_underrun >= sc->sc_stall_count) {
if (num_stalls++ > 0) { if (num_stalls++ > 0) {
device_printf(sc->sc_dev, device_printf(sc->sc_dev,
"RNG stalled, disabling device\n"); "RNG stalled, disabling device\n");
bcm2835_rng_stop(sc); bcm2835_rng_stop(sc);
cemUnsubmitted
Not Done
Inline Actions

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

cem: I'm not sure this is ever what we want?
break; break;
} else { } else {
device_printf(sc->sc_dev, device_printf(sc->sc_dev,
"Too many underruns, resetting\n"); "Too many underruns, resetting\n");
bcm2835_rng_stop(sc); bcm2835_rng_stop(sc);
bcm2835_rng_start(sc); bcm2835_rng_start(sc);
cemUnsubmitted
Not Done
Inline Actions

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?

cem: Is this something that can happen, and do we have reason to believe this procedure will fix the…
crowston_protonmail.comUnsubmitted
Done
Inline Actions

I don't know, that was the original algorithm. I haven't seen it in action.

crowston_protonmail.com: I don't know, that was the original algorithm. I haven't seen it in action.
cemUnsubmitted
Not Done
Inline Actions

Original?

cem: Original?
seen_underrun = 0; seen_underrun = 0;
} }
} }
/* Try again */ /* Try again */
continue; continue;
} }
CTR2(KTR_DEV, "%s: %d words available in RNG FIFO", CTR2(KTR_DEV, "%s: %d words available in RNG FIFO",
device_get_nameunit(sc->sc_dev), num_avail); device_get_nameunit(sc->sc_dev), num_avail);
/* Pull MIN(num_avail, cnt) words from the FIFO */ /* Pull MIN(num_avail, cnt) words from the FIFO */
num_words = (num_avail > cnt) ? cnt : num_avail; num_words = (num_avail > cnt) ? cnt : num_avail;
bcm2835_rng_read_multi4(sc, RNG_DATA, dest, bcm2835_rng_read_multi4(sc, sc->conf->data_reg, dest,
num_words); num_words);
dest += num_words; dest += num_words;
nread += num_words; nread += num_words;
} }
cnt = nread * sizeof(uint32_t); bcm2835_rng_enqueue_harvest(sc, nread);
cemUnsubmitted
Not Done
Inline Actions

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.)

cem: I'd suggest wrapping this in a subroutine that submits multiple random_harvest_queue()…
crowston_protonmail.comUnsubmitted
Done
Inline Actions

Are you saying that random_harvest_queue() only takes some of the random data supplied? If I use the API to submit 10 MB of random data, it effectively only extracts 8 bytes of randomness?

crowston_protonmail.com: Are you saying that `random_harvest_queue()` only takes some of the random data supplied? If I…
cemUnsubmitted
Not Done
Inline Actions

Correct.

cem: Correct.
cemUnsubmitted
Not Done
Inline Actions

To be clear, it is a bad API. I'm not defending it, just telling you where the foot-gun is so you can avoid it.

cem: To be clear, it is a bad API. I'm not defending it, just telling you where the foot-gun is so…
if (cnt > 0)
random_harvest_queue(sc->sc_buf, cnt, RANDOM_PURE_BROADCOM);
callout_reset(&sc->sc_rngto, RNG_CALLOUT_TICKS, bcm2835_rng_harvest, sc); callout_reset(&sc->sc_rngto, RNG_CALLOUT_TICKS, bcm2835_rng_harvest, sc);
} }
static int static int
sysctl_bcm2835_rng_2xspeed(SYSCTL_HANDLER_ARGS) sysctl_bcm2835_rng_2xspeed(SYSCTL_HANDLER_ARGS)
{ {
struct bcm2835_rng_softc *sc = arg1; struct bcm2835_rng_softc *sc = arg1;
Show All 40 Lines
{ {
if (!ofw_bus_status_okay(dev)) if (!ofw_bus_status_okay(dev))
return (ENXIO); return (ENXIO);
if (ofw_bus_search_compatible(dev, compat_data)->ocd_data == 0) if (ofw_bus_search_compatible(dev, compat_data)->ocd_data == 0)
return (ENXIO); return (ENXIO);
device_set_desc(dev, "Broadcom BCM2835 RNG"); device_set_desc(dev, "Broadcom BCM2835/BCM2838 RNG");
return (BUS_PROBE_DEFAULT); return (BUS_PROBE_DEFAULT);
} }
static int static int
bcm2835_rng_attach(device_t dev) bcm2835_rng_attach(device_t dev)
{ {
struct bcm2835_rng_softc *sc; struct bcm2835_rng_softc *sc;
struct sysctl_ctx_list *sysctl_ctx; struct sysctl_ctx_list *sysctl_ctx;
struct sysctl_oid *sysctl_tree; struct sysctl_oid *sysctl_tree;
int error, rid; int error, rid;
error = 0; error = 0;
sc = device_get_softc(dev); sc = device_get_softc(dev);
sc->sc_dev = dev; sc->sc_dev = dev;
sc->conf = (void const*)ofw_bus_search_compatible(dev, compat_data)->ocd_data;
cemUnsubmitted
Not Done
Inline Actions

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

cem: Maybe just `(void *)`, especially if it unwraps the line.
KASSERT(sc->conf != NULL, ("bcm2835_rng_attach: sc->conf == NULL"));
cemUnsubmitted
Done
Inline Actions

I think the message text might be less confusing if it said "sc->conf == NULL". Not a big deal.

cem: I think the message text might be less confusing if it said "sc->conf == NULL". Not a big deal.
delphijUnsubmitted
Not Done
Inline Actions

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

delphij: Maybe assert sc->conf != NULL here instead? (the probe method already returned ENXIO).
cemUnsubmitted
Not Done
Inline Actions

+1

cem: +1
sc->sc_stall_count = RNG_STALL_COUNT_DEFAULT; sc->sc_stall_count = RNG_STALL_COUNT_DEFAULT;
/* Initialize callout */ /* Initialize callout */
callout_init(&sc->sc_rngto, CALLOUT_MPSAFE); callout_init(&sc->sc_rngto, CALLOUT_MPSAFE);
TUNABLE_INT_FETCH("bcmrng.2xspeed", &sc->sc_rbg2x);
TUNABLE_INT_FETCH("bcmrng.stall_count", &sc->sc_stall_count); TUNABLE_INT_FETCH("bcmrng.stall_count", &sc->sc_stall_count);
if (sc->conf->allow_2x_mode)
TUNABLE_INT_FETCH("bcmrng.2xspeed", &sc->sc_rbg2x);
cemUnsubmitted
Not Done
Inline Actions

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)?

cem: Should we default to 2x mode (but allow it to be disabled with the tunable? Is there some…
crowston_protonmail.comUnsubmitted
Done
Inline Actions

I don't know, you'd have to ask the original author. It doesn't seem to exist on the newer 2838 chip, at least, as far as I could figure out. I agree the toggle seems like a complication that probably offers zero value.

crowston_protonmail.com: I don't know, you'd have to ask the original author. It doesn't seem to exist on the newer 2838…
cemUnsubmitted
Not Done
Inline Actions

I mean if you aren’t the original author, do you even have the rights to submit this patch? Can you get in contact with the original author?

cem: I mean if you aren’t the original author, do you even have the rights to submit this patch?
crowston_protonmail.comUnsubmitted
Not Done
Inline Actions

I honestly did not realize that only the original author of a file was entitled to submit a patch. I'm happy to withdraw this if that's the case.

crowston_protonmail.com: I honestly did not realize that only the original author of a file was entitled to submit a…
impUnsubmitted
Not Done
Inline Actions

I think conrad meant original author of the patch...

imp: I think conrad meant original author of the patch...
crowston_protonmail.comUnsubmitted
Not Done
Inline Actions

I wrote this patch, which edits the existing BCM2835 driver from mainline FreeBSD, to add support for the BCM2838 chip. I did not write that original driver. This patch does not intend to affect the existing behaviour of the "bcmrng.2xspeed" mode on the BCM2835 chip. The "2x speed" mode seems like a weird quirk, but I don't want to comment authoritatively on whether it should exist or not, nor whether it should be default enabled. The "2x speed" mode does not appear to exist on the BCM2838 chip, which is why I moved it under an if-block in this patch.

crowston_protonmail.com: I wrote this patch, which edits the existing BCM2835 driver from mainline FreeBSD, to add…
cemUnsubmitted
Not Done
Inline Actions

(It's a red herring here but: yes, I meant the original author of the patch, not the file. I believe you may be able to submit patches on behalf of others; you obviously need at least their permission and license of copyright.)

I was confused when you referred me to the "original author" for questions about what I perceived as changes you had authored. Your latest comment makes clear that the revision is just selectively disabling the existing behavior on the new chip, which doesn't have that functionality. That's perfectly fine! (Probably I should have noticed that when I was first reading the change — my bad.) It also would have been a less confusing answer to the original (confused) question.

Thanks.

cem: (It's a red herring here but: yes, I meant the original author of the patch, not the file. I…
/* Allocate memory resources */ /* Allocate memory resources */
rid = 0; rid = 0;
sc->sc_mem_res = bus_alloc_resource_any(dev, SYS_RES_MEMORY, &rid, sc->sc_mem_res = bus_alloc_resource_any(dev, SYS_RES_MEMORY, &rid,
RF_ACTIVE); RF_ACTIVE);
if (sc->sc_mem_res == NULL) { if (sc->sc_mem_res == NULL) {
bcm2835_rng_detach(dev); bcm2835_rng_detach(dev);
return (ENXIO); return (ENXIO);
Show All 16 Lines if (bootverbose) {
sbuf_delete(&sb); sbuf_delete(&sb);
} }
sysctl_ctx = device_get_sysctl_ctx(dev); sysctl_ctx = device_get_sysctl_ctx(dev);
sysctl_tree = device_get_sysctl_tree(dev); sysctl_tree = device_get_sysctl_tree(dev);
SYSCTL_ADD_LONG(sysctl_ctx, SYSCTL_CHILDREN(sysctl_tree), OID_AUTO, SYSCTL_ADD_LONG(sysctl_ctx, SYSCTL_CHILDREN(sysctl_tree), OID_AUTO,
"underrun", CTLFLAG_RD, &sc->sc_underrun, "underrun", CTLFLAG_RD, &sc->sc_underrun,
"Number of FIFO underruns"); "Number of FIFO underruns");
if (sc->conf->allow_2x_mode)
SYSCTL_ADD_PROC(sysctl_ctx, SYSCTL_CHILDREN(sysctl_tree), OID_AUTO, SYSCTL_ADD_PROC(sysctl_ctx, SYSCTL_CHILDREN(sysctl_tree), OID_AUTO,
"2xspeed", CTLTYPE_INT | CTLFLAG_RW | CTLFLAG_NEEDGIANT, sc, 0, "2xspeed", CTLTYPE_INT | CTLFLAG_RW | CTLFLAG_NEEDGIANT, sc, 0,
sysctl_bcm2835_rng_2xspeed, "I", "Enable RBG 2X SPEED"); sysctl_bcm2835_rng_2xspeed, "I", "Enable RBG 2X SPEED");
SYSCTL_ADD_INT(sysctl_ctx, SYSCTL_CHILDREN(sysctl_tree), OID_AUTO, SYSCTL_ADD_INT(sysctl_ctx, SYSCTL_CHILDREN(sysctl_tree), OID_AUTO,
"stall_count", CTLFLAG_RW, &sc->sc_stall_count, "stall_count", CTLFLAG_RW, &sc->sc_stall_count,
RNG_STALL_COUNT_DEFAULT, "Number of underruns to assume RNG stall"); RNG_STALL_COUNT_DEFAULT, "Number of underruns to assume RNG stall");
#ifdef BCM2835_RNG_DEBUG_REGISTERS #ifdef BCM2835_RNG_DEBUG_REGISTERS
SYSCTL_ADD_PROC(sysctl_ctx, SYSCTL_CHILDREN(sysctl_tree), OID_AUTO, SYSCTL_ADD_PROC(sysctl_ctx, SYSCTL_CHILDREN(sysctl_tree), OID_AUTO,
"dumpregs", CTLTYPE_STRING | CTLFLAG_RD | CTLFLAG_NEEDGIANT, sc, 0, "dumpregs", CTLTYPE_STRING | CTLFLAG_RD | CTLFLAG_NEEDGIANT, sc, 0,
sysctl_bcm2835_rng_dump, "S", "Dump RNG registers"); sysctl_bcm2835_rng_dump, "S", "Dump RNG registers");
#endif #endif
/* /*
* Schedule the initial harvesting one second from now, which should give the * Schedule the initial harvesting one second from now, which should give the
* hardware RNG plenty of time to generate the first random bytes. * hardware RNG plenty of time to generate the first random bytes.
*/ */
callout_reset(&sc->sc_rngto, hz, bcm2835_rng_harvest, sc); callout_reset(&sc->sc_rngto, hz, bcm2835_rng_harvest, sc);
return (0); return (0);
} }
▲ Show 20 Lines • Show All 42 Lines • Show Last 20 Lines