Re: [PATCH v1] hw_random: Fix timeriomem_rng for sub-jiffie update periods

[Herbert Xu]

On Tue, Apr 04, 2017 at 02:43:07PM -0700, Rick Altherr wrote:
> Some hardware RNGs provide a single register for obtaining random data.
> Instead of signaling when new data is available, the reader must wait a
> fixed amount of time between reads for new data to be generated.
> timeriomem_rng implements this scheme with the period specified in
> platform data or device tree.  While the period is specified in
> microseconds, the implementation used a standard timer which has a
> minimum delay of 1 jiffie and caused a significant bottleneck for
> devices that can update at 1us.  By switching to an hrtimer, 1us periods
> now only delay at most 2us per read.
> 
> Migrated to new hw_random API while I in this driver.
> 
> Signed-off-by: Rick Altherr 

Thanks Rick.  Could you split this into two patches? One doing
the API conversion and the other one doing the substantive change
to the entropy gathering?

That way if there is a problem it'll be easier to track down.

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v1] hw_random: Fix timeriomem_rng for sub-jiffie update periods

[Herbert Xu]

On Tue, Apr 04, 2017 at 02:43:07PM -0700, Rick Altherr wrote:
> Some hardware RNGs provide a single register for obtaining random data.
> Instead of signaling when new data is available, the reader must wait a
> fixed amount of time between reads for new data to be generated.
> timeriomem_rng implements this scheme with the period specified in
> platform data or device tree.  While the period is specified in
> microseconds, the implementation used a standard timer which has a
> minimum delay of 1 jiffie and caused a significant bottleneck for
> devices that can update at 1us.  By switching to an hrtimer, 1us periods
> now only delay at most 2us per read.
> 
> Migrated to new hw_random API while I in this driver.
> 
> Signed-off-by: Rick Altherr 

Thanks Rick.  Could you split this into two patches? One doing
the API conversion and the other one doing the substantive change
to the entropy gathering?

That way if there is a problem it'll be easier to track down.

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH v1] hw_random: Fix timeriomem_rng for sub-jiffie update periods

[Rick Altherr]

Some hardware RNGs provide a single register for obtaining random data.
Instead of signaling when new data is available, the reader must wait a
fixed amount of time between reads for new data to be generated.
timeriomem_rng implements this scheme with the period specified in
platform data or device tree.  While the period is specified in
microseconds, the implementation used a standard timer which has a
minimum delay of 1 jiffie and caused a significant bottleneck for
devices that can update at 1us.  By switching to an hrtimer, 1us periods
now only delay at most 2us per read.

Migrated to new hw_random API while I in this driver.

Signed-off-by: Rick Altherr 
---

 drivers/char/hw_random/timeriomem-rng.c | 153 
 1 file changed, 75 insertions(+), 78 deletions(-)

diff --git a/drivers/char/hw_random/timeriomem-rng.c 
b/drivers/char/hw_random/timeriomem-rng.c
index cf37db263ecd..7482de2ca71c 100644
--- a/drivers/char/hw_random/timeriomem-rng.c
+++ b/drivers/char/hw_random/timeriomem-rng.c
@@ -20,90 +20,99 @@
  * TODO: add support for reading sizes other than 32bits and masking
  */
 
-#include 
-#include 
-#include 
-#include 
+#include 
+#include 
+#include 
 #include 
 #include 
+#include 
+#include 
+#include 
+#include 
 #include 
+#include 
 #include 
-#include 
-#include 
-#include 
-#include 
 
-struct timeriomem_rng_private_data {
+struct timeriomem_rng_private {
void __iomem*io_base;
-   unsigned intexpires;
-   unsigned intperiod;
+   ktime_t period;
unsigned intpresent:1;
 
-   struct timer_list   timer;
+   struct hrtimer  timer;
struct completion   completion;
 
-   struct hwrngtimeriomem_rng_ops;
+   struct hwrngrng_ops;
 };
 
-#define to_rng_priv(rng) \
-   ((struct timeriomem_rng_private_data *)rng->priv)
-
-/*
- * have data return 1, however return 0 if we have nothing
- */
-static int timeriomem_rng_data_present(struct hwrng *rng, int wait)
+static int timeriomem_rng_read(struct hwrng *hwrng, void *data,
+   size_t max, bool wait)
 {
-   struct timeriomem_rng_private_data *priv = to_rng_priv(rng);
-
-   if (!wait || priv->present)
-   return priv->present;
+   struct timeriomem_rng_private *priv =
+   container_of(hwrng, struct timeriomem_rng_private, rng_ops);
+   int retval = 0;
+   int period_us = ktime_to_us(priv->period);
+
+   /*
+* There may not have been enough time for new data to be generated
+* since the last request.  If the caller doesn't want to wait, let them
+* bail out.  Otherwise, wait for the completion.  If the new data has
+* already been generated, the completion should already be available.
+*/
+   if (!wait && !priv->present)
+   return 0;
 
wait_for_completion(>completion);
 
-   return 1;
-}
-
-static int timeriomem_rng_data_read(struct hwrng *rng, u32 *data)
-{
-   struct timeriomem_rng_private_data *priv = to_rng_priv(rng);
-   unsigned long cur;
-   s32 delay;
-
-   *data = readl(priv->io_base);
-
-   cur = jiffies;
-
-   delay = cur - priv->expires;
-   delay = priv->period - (delay % priv->period);
-
-   priv->expires = cur + delay;
+   do {
+   /*
+* After the first read, all additional reads will need to wait
+* for the RNG to generate new data.  Since the period can have
+* a wide range of values (1us to 1s have been observed), allow
+* for 1% tolerance in the sleep time rather than a fixed value.
+*/
+   if (retval > 0)
+   usleep_range(period_us,
+   period_us + min(1, period_us / 100));
+
+   *(u32 *)data = readl(priv->io_base);
+   retval += sizeof(u32);
+   data += sizeof(u32);
+   max -= sizeof(u32);
+   } while (wait && max > sizeof(u32));
+
+   /*
+* Block any new callers until the RNG has had time to generate new
+* data.
+*/
priv->present = 0;
-
reinit_completion(>completion);
-   mod_timer(>timer, priv->expires);
+   hrtimer_forward_now(>timer, priv->period);
+   hrtimer_restart(>timer);
 
-   return 4;
+   return retval;
 }
 
-static void timeriomem_rng_trigger(unsigned long data)
+static enum hrtimer_restart timeriomem_rng_trigger(struct hrtimer *timer)
 {
-   struct timeriomem_rng_private_data *priv
-   = (struct timeriomem_rng_private_data *)data;
+   struct timeriomem_rng_private *priv
+   = container_of(timer, struct timeriomem_rng_private, timer);
 
priv->present = 1;
complete(>completion);
+
+   return 

[PATCH v1] hw_random: Fix timeriomem_rng for sub-jiffie update periods

[Rick Altherr]

Some hardware RNGs provide a single register for obtaining random data.
Instead of signaling when new data is available, the reader must wait a
fixed amount of time between reads for new data to be generated.
timeriomem_rng implements this scheme with the period specified in
platform data or device tree.  While the period is specified in
microseconds, the implementation used a standard timer which has a
minimum delay of 1 jiffie and caused a significant bottleneck for
devices that can update at 1us.  By switching to an hrtimer, 1us periods
now only delay at most 2us per read.

Migrated to new hw_random API while I in this driver.

Signed-off-by: Rick Altherr 
---

 drivers/char/hw_random/timeriomem-rng.c | 153 
 1 file changed, 75 insertions(+), 78 deletions(-)

diff --git a/drivers/char/hw_random/timeriomem-rng.c 
b/drivers/char/hw_random/timeriomem-rng.c
index cf37db263ecd..7482de2ca71c 100644
--- a/drivers/char/hw_random/timeriomem-rng.c
+++ b/drivers/char/hw_random/timeriomem-rng.c
@@ -20,90 +20,99 @@
  * TODO: add support for reading sizes other than 32bits and masking
  */
 
-#include 
-#include 
-#include 
-#include 
+#include 
+#include 
+#include 
 #include 
 #include 
+#include 
+#include 
+#include 
+#include 
 #include 
+#include 
 #include 
-#include 
-#include 
-#include 
-#include 
 
-struct timeriomem_rng_private_data {
+struct timeriomem_rng_private {
void __iomem*io_base;
-   unsigned intexpires;
-   unsigned intperiod;
+   ktime_t period;
unsigned intpresent:1;
 
-   struct timer_list   timer;
+   struct hrtimer  timer;
struct completion   completion;
 
-   struct hwrngtimeriomem_rng_ops;
+   struct hwrngrng_ops;
 };
 
-#define to_rng_priv(rng) \
-   ((struct timeriomem_rng_private_data *)rng->priv)
-
-/*
- * have data return 1, however return 0 if we have nothing
- */
-static int timeriomem_rng_data_present(struct hwrng *rng, int wait)
+static int timeriomem_rng_read(struct hwrng *hwrng, void *data,
+   size_t max, bool wait)
 {
-   struct timeriomem_rng_private_data *priv = to_rng_priv(rng);
-
-   if (!wait || priv->present)
-   return priv->present;
+   struct timeriomem_rng_private *priv =
+   container_of(hwrng, struct timeriomem_rng_private, rng_ops);
+   int retval = 0;
+   int period_us = ktime_to_us(priv->period);
+
+   /*
+* There may not have been enough time for new data to be generated
+* since the last request.  If the caller doesn't want to wait, let them
+* bail out.  Otherwise, wait for the completion.  If the new data has
+* already been generated, the completion should already be available.
+*/
+   if (!wait && !priv->present)
+   return 0;
 
wait_for_completion(>completion);
 
-   return 1;
-}
-
-static int timeriomem_rng_data_read(struct hwrng *rng, u32 *data)
-{
-   struct timeriomem_rng_private_data *priv = to_rng_priv(rng);
-   unsigned long cur;
-   s32 delay;
-
-   *data = readl(priv->io_base);
-
-   cur = jiffies;
-
-   delay = cur - priv->expires;
-   delay = priv->period - (delay % priv->period);
-
-   priv->expires = cur + delay;
+   do {
+   /*
+* After the first read, all additional reads will need to wait
+* for the RNG to generate new data.  Since the period can have
+* a wide range of values (1us to 1s have been observed), allow
+* for 1% tolerance in the sleep time rather than a fixed value.
+*/
+   if (retval > 0)
+   usleep_range(period_us,
+   period_us + min(1, period_us / 100));
+
+   *(u32 *)data = readl(priv->io_base);
+   retval += sizeof(u32);
+   data += sizeof(u32);
+   max -= sizeof(u32);
+   } while (wait && max > sizeof(u32));
+
+   /*
+* Block any new callers until the RNG has had time to generate new
+* data.
+*/
priv->present = 0;
-
reinit_completion(>completion);
-   mod_timer(>timer, priv->expires);
+   hrtimer_forward_now(>timer, priv->period);
+   hrtimer_restart(>timer);
 
-   return 4;
+   return retval;
 }
 
-static void timeriomem_rng_trigger(unsigned long data)
+static enum hrtimer_restart timeriomem_rng_trigger(struct hrtimer *timer)
 {
-   struct timeriomem_rng_private_data *priv
-   = (struct timeriomem_rng_private_data *)data;
+   struct timeriomem_rng_private *priv
+   = container_of(timer, struct timeriomem_rng_private, timer);
 
priv->present = 1;
complete(>completion);
+
+   return HRTIMER_NORESTART;
 }
 
 static