| Message ID | 20220216000230.22625-1-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v2] ath9k: use hw_random API instead of directly dumping into random.c | expand |
On 2/15/2022 4:02 PM, Jason A. Donenfeld 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> > --- [snip] > 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"; You will have to give this instance an unique name because there can be multiple ath9k adapters registered in a given system (like Wi-Fi routers), and one of the first thing hwrng_register() does is ensure that there is not an existing rng with the same name. Maybe using a combination of ath9k + dev_name() ought to be unique enough?
"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> [...] > +retry: > + if (max & ~3UL) > + bytes_read = ath9k_rng_data_read(sc, buf, max >> 2); > + if ((max & 3UL) && ath9k_rng_data_read(sc, &word, 1)) { > + memcpy(buf + bytes_read, &word, max & 3); > + bytes_read += max & 3; > + memzero_explicit(&word, sizeof(word)); > + } > + if (max && unlikely(!bytes_read) && wait) { > + msleep(ath9k_rng_delay_get(++fail_stats)); > + goto retry; > } Wouldn't a while loop be cleaner? With a some kind limit for the number of loops, to avoid a neverending loop.
Am Wed, Feb 16, 2022 at 01:02:30AM +0100 schrieb Jason A. Donenfeld: > 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> > --- > v2 operates on whole words when possible. > > drivers/net/wireless/ath/ath9k/ath9k.h | 2 +- > drivers/net/wireless/ath/ath9k/rng.c | 72 +++++++++++--------------- > 2 files changed, 30 insertions(+), 44 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..a0a58f8e08d3 100644 > --- a/drivers/net/wireless/ath/ath9k/rng.c > +++ b/drivers/net/wireless/ath/ath9k/rng.c > @@ -22,11 +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) > { > int i, j; > @@ -72,61 +67,52 @@ 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) > { > - 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)); > + struct ath_softc *sc = container_of(rng, struct ath_softc, rng_ops); > + int bytes_read = 0; > + u32 fail_stats = 0, word; > + > +retry: > + if (max & ~3UL) > + bytes_read = ath9k_rng_data_read(sc, buf, max >> 2); > + if ((max & 3UL) && ath9k_rng_data_read(sc, &word, 1)) { > + memcpy(buf + bytes_read, &word, max & 3); > + bytes_read += max & 3; > + memzero_explicit(&word, sizeof(word)); > + } > + if (max && unlikely(!bytes_read) && wait) { > + msleep(ath9k_rng_delay_get(++fail_stats)); > + goto retry; > } Potentially, this waits forever, if wait is set and no data is returned. Instead, it should return to the main kthread loop every once in a while. Thanks, Dominik
Hi Florian, On Wed, Feb 16, 2022 at 4:13 AM Florian Fainelli <f.fainelli@gmail.com> wrote: > You will have to give this instance an unique name because there can be > multiple ath9k adapters registered in a given system (like Wi-Fi > routers), and one of the first thing hwrng_register() does is ensure > that there is not an existing rng with the same name. > > Maybe using a combination of ath9k + dev_name() ought to be unique enough? Good point. Will do. dev_name() probably won't cut it because of namespaces, but I can always just attach a counter. Will do that for v3. Jason
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..a0a58f8e08d3 100644 --- a/drivers/net/wireless/ath/ath9k/rng.c +++ b/drivers/net/wireless/ath/ath9k/rng.c @@ -22,11 +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) { int i, j; @@ -72,61 +67,52 @@ 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) { - 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)); + struct ath_softc *sc = container_of(rng, struct ath_softc, rng_ops); + int bytes_read = 0; + u32 fail_stats = 0, word; + +retry: + if (max & ~3UL) + bytes_read = ath9k_rng_data_read(sc, buf, max >> 2); + if ((max & 3UL) && ath9k_rng_data_read(sc, &word, 1)) { + memcpy(buf + bytes_read, &word, max & 3); + bytes_read += max & 3; + memzero_explicit(&word, sizeof(word)); + } + if (max && unlikely(!bytes_read) && wait) { + msleep(ath9k_rng_delay_get(++fail_stats)); + goto retry; } - kfree(rng_buf); -out: - sc->rng_task = NULL; - - 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> --- v2 operates on whole words when possible. drivers/net/wireless/ath/ath9k/ath9k.h | 2 +- drivers/net/wireless/ath/ath9k/rng.c | 72 +++++++++++--------------- 2 files changed, 30 insertions(+), 44 deletions(-)