| Message ID | 20220215162812.195716-1-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ath9k: use hw_random API instead of directly dumping into random.c | expand |
Hi, Jason, On Tue, 15 Feb 2022 at 22:44, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > Hardware random number generators are supposed to use the hw_random > framework. This commit turns ath9k's kthread-based design into a proper > hw_random driver. > > This compiles, but I have no hardware or other ability to determine > whether it works. I'll leave further development up to the ath9k > and hw_random maintainers. > > Cc: Toke Høiland-Jørgensen <toke@redhat.com> > Cc: Kalle Valo <kvalo@kernel.org> > Cc: Dominik Brodowski <linux@dominikbrodowski.net> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> [patch snipped] On my laptop, with a… 02:00.0 Network controller: Qualcomm Atheros AR9462 Wireless Network Adapter (rev 01) … I have the following… rui@arrandale:~$ cat /sys/devices/virtual/misc/hw_random/rng_available ath9k rui@arrandale:~$ cat /sys/devices/virtual/misc/hw_random/rng_current ath9k rui@arrandale:~$ … and sure enough, /dev/hwrng is created and outputs a stream of random data, as expected. I haven't done any serious randomness quality testing, but it should be the same as the one produced by the original code. I consider this patch thus Tested-by: Rui Salvaterra <rsalvaterra@gmail.com> Thanks, Rui
"Jason A. Donenfeld" <Jason@zx2c4.com> writes: > Hardware random number generators are supposed to use the hw_random > framework. This commit turns ath9k's kthread-based design into a proper > hw_random driver. > > This compiles, but I have no hardware or other ability to determine > whether it works. I'll leave further development up to the ath9k > and hw_random maintainers. > > Cc: Toke Høiland-Jørgensen <toke@redhat.com> > Cc: Kalle Valo <kvalo@kernel.org> > Cc: Dominik Brodowski <linux@dominikbrodowski.net> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > --- > drivers/net/wireless/ath/ath9k/ath9k.h | 2 +- > drivers/net/wireless/ath/ath9k/rng.c | 62 +++++++++----------------- > 2 files changed, 23 insertions(+), 41 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h > index ef6f5ea06c1f..142f472903dc 100644 > --- a/drivers/net/wireless/ath/ath9k/ath9k.h > +++ b/drivers/net/wireless/ath/ath9k/ath9k.h > @@ -1072,7 +1072,7 @@ struct ath_softc { > > #ifdef CONFIG_ATH9K_HWRNG > u32 rng_last; > - struct task_struct *rng_task; > + struct hwrng rng_ops; > #endif > }; > > diff --git a/drivers/net/wireless/ath/ath9k/rng.c b/drivers/net/wireless/ath/ath9k/rng.c > index aae2bd3cac69..369b222908ba 100644 > --- a/drivers/net/wireless/ath/ath9k/rng.c > +++ b/drivers/net/wireless/ath/ath9k/rng.c > @@ -22,9 +22,6 @@ > #include "hw.h" > #include "ar9003_phy.h" > > -#define ATH9K_RNG_BUF_SIZE 320 > -#define ATH9K_RNG_ENTROPY(x) (((x) * 8 * 10) >> 5) /* quality: 10/32 */ So this comment says "quality: 10/32" but below you're setting "quality" as 320. No idea what the units are supposed to be, but is this right? > static DECLARE_WAIT_QUEUE_HEAD(rng_queue); > > static int ath9k_rng_data_read(struct ath_softc *sc, u32 *buf, u32 buf_size) This function takes buf as a *u32, and interprets buf_size as a number of u32s... > @@ -72,61 +69,46 @@ static u32 ath9k_rng_delay_get(u32 fail_stats) > return delay; > } > > -static int ath9k_rng_kthread(void *data) > +static int ath9k_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) > { > + struct ath_softc *sc = container_of(rng, struct ath_softc, rng_ops); > int bytes_read; > - struct ath_softc *sc = data; > - u32 *rng_buf; > - u32 delay, fail_stats = 0; > - > - rng_buf = kmalloc_array(ATH9K_RNG_BUF_SIZE, sizeof(u32), GFP_KERNEL); > - if (!rng_buf) > - goto out; > - > - while (!kthread_should_stop()) { > - bytes_read = ath9k_rng_data_read(sc, rng_buf, > - ATH9K_RNG_BUF_SIZE); > - if (unlikely(!bytes_read)) { > - delay = ath9k_rng_delay_get(++fail_stats); > - wait_event_interruptible_timeout(rng_queue, > - kthread_should_stop(), > - msecs_to_jiffies(delay)); > - continue; > - } > - > - fail_stats = 0; > - > - /* sleep until entropy bits under write_wakeup_threshold */ > - add_hwgenerator_randomness((void *)rng_buf, bytes_read, > - ATH9K_RNG_ENTROPY(bytes_read)); > - } > + u32 fail_stats = 0; > > - kfree(rng_buf); > -out: > - sc->rng_task = NULL; > +retry: > + bytes_read = ath9k_rng_data_read(sc, buf, max); ... but AFAICT here you're calling it with a buffer size from hw_random that's in bytes? -Toke
On 2/16/22, Toke Høiland-Jørgensen <toke@toke.dk> wrote: > "Jason A. Donenfeld" <Jason@zx2c4.com> writes: > >> Hardware random number generators are supposed to use the hw_random >> framework. This commit turns ath9k's kthread-based design into a proper >> hw_random driver. >> >> This compiles, but I have no hardware or other ability to determine >> whether it works. I'll leave further development up to the ath9k >> and hw_random maintainers. >> >> Cc: Toke Høiland-Jørgensen <toke@redhat.com> >> Cc: Kalle Valo <kvalo@kernel.org> >> Cc: Dominik Brodowski <linux@dominikbrodowski.net> >> Cc: Herbert Xu <herbert@gondor.apana.org.au> >> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> >> --- >> drivers/net/wireless/ath/ath9k/ath9k.h | 2 +- >> drivers/net/wireless/ath/ath9k/rng.c | 62 +++++++++----------------- >> 2 files changed, 23 insertions(+), 41 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h >> b/drivers/net/wireless/ath/ath9k/ath9k.h >> index ef6f5ea06c1f..142f472903dc 100644 >> --- a/drivers/net/wireless/ath/ath9k/ath9k.h >> +++ b/drivers/net/wireless/ath/ath9k/ath9k.h >> @@ -1072,7 +1072,7 @@ struct ath_softc { >> >> #ifdef CONFIG_ATH9K_HWRNG >> u32 rng_last; >> - struct task_struct *rng_task; >> + struct hwrng rng_ops; >> #endif >> }; >> >> diff --git a/drivers/net/wireless/ath/ath9k/rng.c >> b/drivers/net/wireless/ath/ath9k/rng.c >> index aae2bd3cac69..369b222908ba 100644 >> --- a/drivers/net/wireless/ath/ath9k/rng.c >> +++ b/drivers/net/wireless/ath/ath9k/rng.c >> @@ -22,9 +22,6 @@ >> #include "hw.h" >> #include "ar9003_phy.h" >> >> -#define ATH9K_RNG_BUF_SIZE 320 >> -#define ATH9K_RNG_ENTROPY(x) (((x) * 8 * 10) >> 5) /* quality: 10/32 */ > > So this comment says "quality: 10/32" but below you're setting "quality" > as 320. No idea what the units are supposed to be, but is this right? I think the unit is supposed to be how many entropic bits there are out of 1024 bits? These types of estimates are always BS, so keeping it on the lower end as before seemed right. Herbert can jump in here if he has a better idea; that's his jig. > >> static DECLARE_WAIT_QUEUE_HEAD(rng_queue); >> >> static int ath9k_rng_data_read(struct ath_softc *sc, u32 *buf, u32 >> buf_size) > > This function takes buf as a *u32, and interprets buf_size as a number > of u32s... Oh my... Nice catch. I'll send a v2 shortly. I wonder how this managed to work for Rui. > >> @@ -72,61 +69,46 @@ static u32 ath9k_rng_delay_get(u32 fail_stats) >> return delay; >> } >> >> -static int ath9k_rng_kthread(void *data) >> +static int ath9k_rng_read(struct hwrng *rng, void *buf, size_t max, bool >> wait) >> { >> + struct ath_softc *sc = container_of(rng, struct ath_softc, rng_ops); >> int bytes_read; >> - struct ath_softc *sc = data; >> - u32 *rng_buf; >> - u32 delay, fail_stats = 0; >> - >> - rng_buf = kmalloc_array(ATH9K_RNG_BUF_SIZE, sizeof(u32), GFP_KERNEL); >> - if (!rng_buf) >> - goto out; >> - >> - while (!kthread_should_stop()) { >> - bytes_read = ath9k_rng_data_read(sc, rng_buf, >> - ATH9K_RNG_BUF_SIZE); >> - if (unlikely(!bytes_read)) { >> - delay = ath9k_rng_delay_get(++fail_stats); >> - wait_event_interruptible_timeout(rng_queue, >> - kthread_should_stop(), >> - msecs_to_jiffies(delay)); >> - continue; >> - } >> - >> - fail_stats = 0; >> - >> - /* sleep until entropy bits under write_wakeup_threshold */ >> - add_hwgenerator_randomness((void *)rng_buf, bytes_read, >> - ATH9K_RNG_ENTROPY(bytes_read)); >> - } >> + u32 fail_stats = 0; >> >> - kfree(rng_buf); >> -out: >> - sc->rng_task = NULL; >> +retry: >> + bytes_read = ath9k_rng_data_read(sc, buf, max); > > ... but AFAICT here you're calling it with a buffer size from hw_random > that's in bytes? V2 on its way. Rui - you may need to re-test... Jason
Am Wed, Feb 16, 2022 at 12:52:15AM +0100 schrieb Jason A. Donenfeld: > On 2/16/22, Toke Høiland-Jørgensen <toke@toke.dk> wrote: > >> @@ -22,9 +22,6 @@ > >> #include "hw.h" > >> #include "ar9003_phy.h" > >> > >> -#define ATH9K_RNG_BUF_SIZE 320 > >> -#define ATH9K_RNG_ENTROPY(x) (((x) * 8 * 10) >> 5) /* quality: 10/32 */ > > > > So this comment says "quality: 10/32" but below you're setting "quality" > > as 320. No idea what the units are supposed to be, but is this right? > > I think the unit is supposed to be how many entropic bits there are > out of 1024 bits? These types of estimates are always BS, so keeping > it on the lower end as before seemed right. Herbert can jump in here > if he has a better idea; that's his jig. 10/32 = 320/1024, so that change is correct. Dominik
diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h index ef6f5ea06c1f..142f472903dc 100644 --- a/drivers/net/wireless/ath/ath9k/ath9k.h +++ b/drivers/net/wireless/ath/ath9k/ath9k.h @@ -1072,7 +1072,7 @@ struct ath_softc { #ifdef CONFIG_ATH9K_HWRNG u32 rng_last; - struct task_struct *rng_task; + struct hwrng rng_ops; #endif }; diff --git a/drivers/net/wireless/ath/ath9k/rng.c b/drivers/net/wireless/ath/ath9k/rng.c index aae2bd3cac69..369b222908ba 100644 --- a/drivers/net/wireless/ath/ath9k/rng.c +++ b/drivers/net/wireless/ath/ath9k/rng.c @@ -22,9 +22,6 @@ #include "hw.h" #include "ar9003_phy.h" -#define ATH9K_RNG_BUF_SIZE 320 -#define ATH9K_RNG_ENTROPY(x) (((x) * 8 * 10) >> 5) /* quality: 10/32 */ - static DECLARE_WAIT_QUEUE_HEAD(rng_queue); static int ath9k_rng_data_read(struct ath_softc *sc, u32 *buf, u32 buf_size) @@ -72,61 +69,46 @@ static u32 ath9k_rng_delay_get(u32 fail_stats) return delay; } -static int ath9k_rng_kthread(void *data) +static int ath9k_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) { + struct ath_softc *sc = container_of(rng, struct ath_softc, rng_ops); int bytes_read; - struct ath_softc *sc = data; - u32 *rng_buf; - u32 delay, fail_stats = 0; - - rng_buf = kmalloc_array(ATH9K_RNG_BUF_SIZE, sizeof(u32), GFP_KERNEL); - if (!rng_buf) - goto out; - - while (!kthread_should_stop()) { - bytes_read = ath9k_rng_data_read(sc, rng_buf, - ATH9K_RNG_BUF_SIZE); - if (unlikely(!bytes_read)) { - delay = ath9k_rng_delay_get(++fail_stats); - wait_event_interruptible_timeout(rng_queue, - kthread_should_stop(), - msecs_to_jiffies(delay)); - continue; - } - - fail_stats = 0; - - /* sleep until entropy bits under write_wakeup_threshold */ - add_hwgenerator_randomness((void *)rng_buf, bytes_read, - ATH9K_RNG_ENTROPY(bytes_read)); - } + u32 fail_stats = 0; - kfree(rng_buf); -out: - sc->rng_task = NULL; +retry: + bytes_read = ath9k_rng_data_read(sc, buf, max); + if (unlikely(!bytes_read) && wait) { + msleep(ath9k_rng_delay_get(++fail_stats)); + goto retry; + } - return 0; + return bytes_read; } void ath9k_rng_start(struct ath_softc *sc) { struct ath_hw *ah = sc->sc_ah; + int ret; - if (sc->rng_task) + if (sc->rng_ops.read) return; if (!AR_SREV_9300_20_OR_LATER(ah)) return; - sc->rng_task = kthread_run(ath9k_rng_kthread, sc, "ath9k-hwrng"); - if (IS_ERR(sc->rng_task)) - sc->rng_task = NULL; + sc->rng_ops.name = "ath9k"; + sc->rng_ops.read = ath9k_rng_read; + sc->rng_ops.quality = 320; + + ret = devm_hwrng_register(sc->dev, &sc->rng_ops); + if (ret) + sc->rng_ops.read = NULL; } void ath9k_rng_stop(struct ath_softc *sc) { - if (sc->rng_task) { - kthread_stop(sc->rng_task); - sc->rng_task = NULL; + if (sc->rng_ops.read) { + devm_hwrng_unregister(sc->dev, &sc->rng_ops); + sc->rng_ops.read = NULL; } }
Hardware random number generators are supposed to use the hw_random framework. This commit turns ath9k's kthread-based design into a proper hw_random driver. This compiles, but I have no hardware or other ability to determine whether it works. I'll leave further development up to the ath9k and hw_random maintainers. Cc: Toke Høiland-Jørgensen <toke@redhat.com> Cc: Kalle Valo <kvalo@kernel.org> Cc: Dominik Brodowski <linux@dominikbrodowski.net> Cc: Herbert Xu <herbert@gondor.apana.org.au> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- drivers/net/wireless/ath/ath9k/ath9k.h | 2 +- drivers/net/wireless/ath/ath9k/rng.c | 62 +++++++++----------------- 2 files changed, 23 insertions(+), 41 deletions(-)