| Message ID | 20220216113323.53332-1-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [v3] ath9k: use hw_random API instead of directly dumping into random.c | expand |
Hi again, Jason, On Wed, 16 Feb 2022 at 11:33, Jason A. Donenfeld <Jason@zx2c4.com> wrote: [snipped] > Changes v2->v3: > - Use msleep_interruptable like other hwrng drivers. > - Give up after 110 tries. > - Return -EIO after giving up like other hwrng drivers. > - Use for loop for style nits. > - Append serial number for driver in case of multiple cards. [snipped] Everything working as expected here, so this patch (v3) is also 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. > > Cc: Toke Høiland-Jørgensen <toke@redhat.com> > Cc: Kalle Valo <kvalo@kernel.org> > Cc: Rui Salvaterra <rsalvaterra@gmail.com> > Cc: Dominik Brodowski <linux@dominikbrodowski.net> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> Alright, LGTM. Thank you for the patch! Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
"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. > > Cc: Toke Høiland-Jørgensen <toke@redhat.com> > Cc: Kalle Valo <kvalo@kernel.org> > Cc: Rui Salvaterra <rsalvaterra@gmail.com> > Cc: Dominik Brodowski <linux@dominikbrodowski.net> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > Tested-by: Rui Salvaterra <rsalvaterra@gmail.com> > Acked-by: Toke Høiland-Jørgensen <toke@toke.dk> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> Patch applied to ath-next branch of ath.git, thanks. fcd09c90c3c5 ath9k: use hw_random API instead of directly dumping into random.c
On Mon, 21 Feb 2022 at 11:57, Kalle Valo <kvalo@kernel.org> wrote: > > "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. > > > > Cc: Toke Høiland-Jørgensen <toke@redhat.com> > > Cc: Kalle Valo <kvalo@kernel.org> > > Cc: Rui Salvaterra <rsalvaterra@gmail.com> > > Cc: Dominik Brodowski <linux@dominikbrodowski.net> > > Cc: Herbert Xu <herbert@gondor.apana.org.au> > > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > > Tested-by: Rui Salvaterra <rsalvaterra@gmail.com> > > Acked-by: Toke Høiland-Jørgensen <toke@toke.dk> > > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> > > Patch applied to ath-next branch of ath.git, thanks. > > fcd09c90c3c5 ath9k: use hw_random API instead of directly dumping into random.c > With this patch, it seems we end up registering the hw_rng every time the link goes up, and unregister it again when the link goes down, right? Wouldn't it be better to split off this driver from the 802.11 link state handling?
On 2/22/22, Ard Biesheuvel <ardb@kernel.org> wrote: > On Mon, 21 Feb 2022 at 11:57, Kalle Valo <kvalo@kernel.org> wrote: >> >> "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. >> > >> > Cc: Toke Høiland-Jørgensen <toke@redhat.com> >> > Cc: Kalle Valo <kvalo@kernel.org> >> > Cc: Rui Salvaterra <rsalvaterra@gmail.com> >> > Cc: Dominik Brodowski <linux@dominikbrodowski.net> >> > Cc: Herbert Xu <herbert@gondor.apana.org.au> >> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> >> > Tested-by: Rui Salvaterra <rsalvaterra@gmail.com> >> > Acked-by: Toke Høiland-Jørgensen <toke@toke.dk> >> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> >> >> Patch applied to ath-next branch of ath.git, thanks. >> >> fcd09c90c3c5 ath9k: use hw_random API instead of directly dumping into >> random.c >> > > With this patch, it seems we end up registering the hw_rng every time > the link goes up, and unregister it again when the link goes down, > right? > > Wouldn't it be better to split off this driver from the 802.11 link > state handling? > I really have no idea how this thing works, and I tried hard to change as little as possible in converting it to the API. You may want to send some follow-up patches if you have hardware to experiment with. One consideration does leap out, which is that in my experience wifi cards use a lot less power when they're set "down", as though a decent amount of hardware is being switched off. I think this ath9k rng call might be using the ADC to gather samples of ether from somewhere. I imagine this gets shutdown too as part of that dame circuitry.
diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h index ef6f5ea06c1f..3ccf8cfc6b63 100644 --- a/drivers/net/wireless/ath/ath9k/ath9k.h +++ b/drivers/net/wireless/ath/ath9k/ath9k.h @@ -1071,8 +1071,9 @@ struct ath_softc { #endif #ifdef CONFIG_ATH9K_HWRNG + struct hwrng rng_ops; u32 rng_last; - struct task_struct *rng_task; + char rng_name[sizeof("ath9k_65535")]; #endif }; diff --git a/drivers/net/wireless/ath/ath9k/rng.c b/drivers/net/wireless/ath/ath9k/rng.c index f9d3d6eedd3c..cb5414265a9b 100644 --- a/drivers/net/wireless/ath/ath9k/rng.c +++ b/drivers/net/wireless/ath/ath9k/rng.c @@ -21,11 +21,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; @@ -71,61 +66,56 @@ 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; + struct ath_softc *sc = container_of(rng, struct ath_softc, rng_ops); + u32 fail_stats = 0, word; + int bytes_read = 0; + + for (;;) { + 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 & 3UL); + bytes_read += max & 3UL; + memzero_explicit(&word, sizeof(word)); } + if (!wait || !max || likely(bytes_read) || fail_stats > 110) + break; - fail_stats = 0; - - /* sleep until entropy bits under write_wakeup_threshold */ - add_hwgenerator_randomness((void *)rng_buf, bytes_read, - ATH9K_RNG_ENTROPY(bytes_read)); + msleep_interruptible(ath9k_rng_delay_get(++fail_stats)); } - kfree(rng_buf); -out: - sc->rng_task = NULL; - - return 0; + if (wait && !bytes_read && max) + bytes_read = -EIO; + return bytes_read; } void ath9k_rng_start(struct ath_softc *sc) { + static atomic_t serial = ATOMIC_INIT(0); struct ath_hw *ah = sc->sc_ah; - 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; + snprintf(sc->rng_name, sizeof(sc->rng_name), "ath9k_%u", + (atomic_inc_return(&serial) - 1) & U16_MAX); + sc->rng_ops.name = sc->rng_name; + sc->rng_ops.read = ath9k_rng_read; + sc->rng_ops.quality = 320; + + if (devm_hwrng_register(sc->dev, &sc->rng_ops)) + 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. Cc: Toke Høiland-Jørgensen <toke@redhat.com> Cc: Kalle Valo <kvalo@kernel.org> Cc: Rui Salvaterra <rsalvaterra@gmail.com> Cc: Dominik Brodowski <linux@dominikbrodowski.net> Cc: Herbert Xu <herbert@gondor.apana.org.au> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- Changes v2->v3: - Use msleep_interruptable like other hwrng drivers. - Give up after 110 tries. - Return -EIO after giving up like other hwrng drivers. - Use for loop for style nits. - Append serial number for driver in case of multiple cards. Changes v1->v2: - Count in words rather than bytes. drivers/net/wireless/ath/ath9k/ath9k.h | 3 +- drivers/net/wireless/ath/ath9k/rng.c | 72 +++++++++++--------------- 2 files changed, 33 insertions(+), 42 deletions(-)