ath9k should perhaps use hw_random api?

ath9k should perhaps use hw_random api?

[Jason A. Donenfeld]

Hi Miaoqing,

I'm emailing you because I've noticed that ath9k's rng.c is the *only*
driver in the whole of the tree that calls
add_hwgenerator_randomness() directly, rather than going through
Herbert's hw_random API, as every single other hardware RNG does.

I'm wondering if you'd consider converting your driver into something
suitable for the hw_random API (in drivers/char/hw_random/), rather
than adhoc rolling your own ath9k rng kthread. Is this something
you're actively maintaining and would be interested in doing?

Regards,
Jason

ath9k should perhaps use hw_random api?

[Jason A. Donenfeld]

Hi Ath9k Maintainers,

I'm emailing you because I've noticed that ath9k's rng.c is the *only*
driver in the whole of the tree that calls
add_hwgenerator_randomness() directly, rather than going through
Herbert's hw_random API, as every single other hardware RNG does.

I'm wondering if you'd consider converting your driver into something
suitable for the hw_random API (in drivers/char/hw_random/), rather
than adhoc rolling your own ath9k rng kthread. Is this something
you're actively maintaining and would be interested in doing?

Regards,
Jason

[PATCH] ath9k: use hw_random API instead of directly dumping into random.c

[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>
---
 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 */
-
 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;
 	}
 }
-- 
2.35.0

Re: [PATCH] ath9k: use hw_random API instead of directly dumping into random.c

[Rui Salvaterra]

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

Re: [PATCH] ath9k: use hw_random API instead of directly dumping into random.c

[Toke Høiland-Jørgensen]

"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

Re: [PATCH] ath9k: use hw_random API instead of directly dumping into random.c

[Jason A. Donenfeld]

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

[PATCH v2] ath9k: use hw_random API instead of directly dumping into random.c

[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;
 	}
 
-	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;
 	}
 }
-- 
2.35.0

Re: [PATCH v2] ath9k: use hw_random API instead of directly dumping into random.c

[Florian Fainelli]

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

Re: [PATCH v2] ath9k: use hw_random API instead of directly dumping into random.c

[Kalle Valo]

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

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] ath9k: use hw_random API instead of directly dumping into random.c

[Dominik Brodowski]

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

Re: [PATCH v2] ath9k: use hw_random API instead of directly dumping into random.c

[Dominik Brodowski]

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

Re: [PATCH v2] ath9k: use hw_random API instead of directly dumping into random.c

[Jason A. Donenfeld]

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

[PATCH v3] ath9k: use hw_random API instead of directly dumping into random.c

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

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

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;
 	}
 }
-- 
2.35.0

Re: [PATCH v3] ath9k: use hw_random API instead of directly dumping into random.c

[Rui Salvaterra]

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

Re: [PATCH v3] ath9k: use hw_random API instead of directly dumping into random.c

[Toke Høiland-Jørgensen]

"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>

Re: [PATCH v3] ath9k: use hw_random API instead of directly dumping into random.c

[Kalle Valo]

"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

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20220216113323.53332-1-Jason@zx2c4.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v3] ath9k: use hw_random API instead of directly dumping into random.c

[Ard Biesheuvel]

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?

Re: [PATCH v3] ath9k: use hw_random API instead of directly dumping into random.c

[Jason A. Donenfeld]

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.