* [PATCHv2 1/2] hwrng: optee: handle unlimited data rates @ 2020-07-23 8:46 Jorge Ramirez-Ortiz 2020-07-23 8:46 ` [PATCHv2 2/2] hwrng: optee: fix wait use case Jorge Ramirez-Ortiz 2020-07-24 13:24 ` [PATCHv2 1/2] hwrng: optee: handle unlimited data rates Sumit Garg 0 siblings, 2 replies; 14+ messages in thread From: Jorge Ramirez-Ortiz @ 2020-07-23 8:46 UTC (permalink / raw) To: jorge, sumit.garg, mpm, herbert Cc: jens.wiklander, arnd, ricardo, mike, gregkh, op-tee, linux-crypto, linux-kernel Data rates of MAX_UINT32 will schedule an unnecessary one jiffy timeout on the call to msleep. Avoid this scenario by using 0 as the unlimited data rate. Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> --- drivers/char/hw_random/optee-rng.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c index 49b2e02537dd..5bc4700c4dae 100644 --- a/drivers/char/hw_random/optee-rng.c +++ b/drivers/char/hw_random/optee-rng.c @@ -128,7 +128,7 @@ static int optee_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) data += rng_size; read += rng_size; - if (wait) { + if (wait && pvt_data->data_rate) { if (timeout-- == 0) return read; msleep((1000 * (max - read)) / pvt_data->data_rate); -- 2.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCHv2 2/2] hwrng: optee: fix wait use case 2020-07-23 8:46 [PATCHv2 1/2] hwrng: optee: handle unlimited data rates Jorge Ramirez-Ortiz @ 2020-07-23 8:46 ` Jorge Ramirez-Ortiz 2020-07-24 13:22 ` Sumit Garg 2020-08-05 13:34 ` Jorge Ramirez-Ortiz, Foundries 2020-07-24 13:24 ` [PATCHv2 1/2] hwrng: optee: handle unlimited data rates Sumit Garg 1 sibling, 2 replies; 14+ messages in thread From: Jorge Ramirez-Ortiz @ 2020-07-23 8:46 UTC (permalink / raw) To: jorge, sumit.garg, mpm, herbert Cc: jens.wiklander, arnd, ricardo, mike, gregkh, op-tee, linux-crypto, linux-kernel The current code waits for data to be available before attempting a second read. However the second read would not be executed as the while loop exits. This fix does not wait if all data has been read and reads a second time if only partial data was retrieved on the first read. This fix also does not attempt to read if not data is requested. Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> --- v2: tidy up the while loop to avoid reading when no data is requested drivers/char/hw_random/optee-rng.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c index 5bc4700c4dae..a99d82949981 100644 --- a/drivers/char/hw_random/optee-rng.c +++ b/drivers/char/hw_random/optee-rng.c @@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) if (max > MAX_ENTROPY_REQ_SZ) max = MAX_ENTROPY_REQ_SZ; - while (read == 0) { + while (read < max) { rng_size = get_optee_rng_data(pvt_data, data, (max - read)); data += rng_size; read += rng_size; if (wait && pvt_data->data_rate) { - if (timeout-- == 0) + if ((timeout-- == 0) || (read == max)) return read; msleep((1000 * (max - read)) / pvt_data->data_rate); } else { -- 2.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCHv2 2/2] hwrng: optee: fix wait use case 2020-07-23 8:46 ` [PATCHv2 2/2] hwrng: optee: fix wait use case Jorge Ramirez-Ortiz @ 2020-07-24 13:22 ` Sumit Garg 2020-07-24 14:23 ` Jorge Ramirez-Ortiz, Foundries 2020-08-05 13:34 ` Jorge Ramirez-Ortiz, Foundries 1 sibling, 1 reply; 14+ messages in thread From: Sumit Garg @ 2020-07-24 13:22 UTC (permalink / raw) To: Jorge Ramirez-Ortiz Cc: Matt Mackall, Herbert Xu, Jens Wiklander, Arnd Bergmann, ricardo, Michael Scott, Greg Kroah-Hartman, op-tee, open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Linux Kernel Mailing List On Thu, 23 Jul 2020 at 14:16, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote: > > The current code waits for data to be available before attempting a > second read. However the second read would not be executed as the > while loop exits. > > This fix does not wait if all data has been read and reads a second > time if only partial data was retrieved on the first read. > > This fix also does not attempt to read if not data is requested. I am not sure how this is possible, can you elaborate? > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> > --- > v2: tidy up the while loop to avoid reading when no data is requested > > drivers/char/hw_random/optee-rng.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c > index 5bc4700c4dae..a99d82949981 100644 > --- a/drivers/char/hw_random/optee-rng.c > +++ b/drivers/char/hw_random/optee-rng.c > @@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) > if (max > MAX_ENTROPY_REQ_SZ) > max = MAX_ENTROPY_REQ_SZ; > > - while (read == 0) { > + while (read < max) { > rng_size = get_optee_rng_data(pvt_data, data, (max - read)); > > data += rng_size; > read += rng_size; > > if (wait && pvt_data->data_rate) { > - if (timeout-- == 0) > + if ((timeout-- == 0) || (read == max)) If read == max, would there be any sleep? -Sumit > return read; > msleep((1000 * (max - read)) / pvt_data->data_rate); > } else { > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv2 2/2] hwrng: optee: fix wait use case 2020-07-24 13:22 ` Sumit Garg @ 2020-07-24 14:23 ` Jorge Ramirez-Ortiz, Foundries 2020-07-28 10:05 ` Jorge Ramirez-Ortiz, Foundries 2020-08-05 13:49 ` Sumit Garg 0 siblings, 2 replies; 14+ messages in thread From: Jorge Ramirez-Ortiz, Foundries @ 2020-07-24 14:23 UTC (permalink / raw) To: Sumit Garg Cc: Jorge Ramirez-Ortiz, Matt Mackall, Herbert Xu, Jens Wiklander, Arnd Bergmann, ricardo, Michael Scott, Greg Kroah-Hartman, op-tee, open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Linux Kernel Mailing List On 24/07/20, Sumit Garg wrote: > On Thu, 23 Jul 2020 at 14:16, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote: > > > > The current code waits for data to be available before attempting a > > second read. However the second read would not be executed as the > > while loop exits. > > > > This fix does not wait if all data has been read and reads a second > > time if only partial data was retrieved on the first read. > > > > This fix also does not attempt to read if not data is requested. > > I am not sure how this is possible, can you elaborate? currently, if the user sets max 0, get_optee_rng_data will regardless issuese a call to the secure world requesting 0 bytes from the RNG with this patch, this request is avoided. > > > > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> > > --- > > v2: tidy up the while loop to avoid reading when no data is requested > > > > drivers/char/hw_random/optee-rng.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c > > index 5bc4700c4dae..a99d82949981 100644 > > --- a/drivers/char/hw_random/optee-rng.c > > +++ b/drivers/char/hw_random/optee-rng.c > > @@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) > > if (max > MAX_ENTROPY_REQ_SZ) > > max = MAX_ENTROPY_REQ_SZ; > > > > - while (read == 0) { > > + while (read < max) { > > rng_size = get_optee_rng_data(pvt_data, data, (max - read)); > > > > data += rng_size; > > read += rng_size; > > > > if (wait && pvt_data->data_rate) { > > - if (timeout-- == 0) > > + if ((timeout-- == 0) || (read == max)) > > If read == max, would there be any sleep? no but I see no reason why there should be a wait since we already have all the data that we need; the msleep is only required when we need to wait for the RNG to generate entropy for the number of bytes we are requesting. if we are requesting 0 bytes, the entropy is already available. at leat this is what makes sense to me. > > -Sumit > > > return read; > > msleep((1000 * (max - read)) / pvt_data->data_rate); > > } else { > > -- > > 2.17.1 > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv2 2/2] hwrng: optee: fix wait use case 2020-07-24 14:23 ` Jorge Ramirez-Ortiz, Foundries @ 2020-07-28 10:05 ` Jorge Ramirez-Ortiz, Foundries 2020-08-05 13:49 ` Sumit Garg 1 sibling, 0 replies; 14+ messages in thread From: Jorge Ramirez-Ortiz, Foundries @ 2020-07-28 10:05 UTC (permalink / raw) To: Jorge Ramirez-Ortiz, Foundries Cc: Sumit Garg, Matt Mackall, Herbert Xu, Jens Wiklander, Arnd Bergmann, ricardo, Michael Scott, Greg Kroah-Hartman, op-tee, open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Linux Kernel Mailing List On 24/07/20, Jorge Ramirez-Ortiz, Foundries wrote: > On 24/07/20, Sumit Garg wrote: > > On Thu, 23 Jul 2020 at 14:16, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote: > > > > > > The current code waits for data to be available before attempting a > > > second read. However the second read would not be executed as the > > > while loop exits. > > > > > > This fix does not wait if all data has been read and reads a second > > > time if only partial data was retrieved on the first read. > > > > > > This fix also does not attempt to read if not data is requested. > > > > I am not sure how this is possible, can you elaborate? > > currently, if the user sets max 0, get_optee_rng_data will regardless > issuese a call to the secure world requesting 0 bytes from the RNG > > with this patch, this request is avoided. > > > > > > > > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> > > > --- > > > v2: tidy up the while loop to avoid reading when no data is requested > > > > > > drivers/char/hw_random/optee-rng.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c > > > index 5bc4700c4dae..a99d82949981 100644 > > > --- a/drivers/char/hw_random/optee-rng.c > > > +++ b/drivers/char/hw_random/optee-rng.c > > > @@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) > > > if (max > MAX_ENTROPY_REQ_SZ) > > > max = MAX_ENTROPY_REQ_SZ; > > > > > > - while (read == 0) { > > > + while (read < max) { > > > rng_size = get_optee_rng_data(pvt_data, data, (max - read)); > > > > > > data += rng_size; > > > read += rng_size; > > > > > > if (wait && pvt_data->data_rate) { > > > - if (timeout-- == 0) > > > + if ((timeout-- == 0) || (read == max)) > > > > If read == max, would there be any sleep? > > no but I see no reason why there should be a wait since we already have > all the data that we need; the msleep is only required when we need to > wait for the RNG to generate entropy for the number of bytes we are > requesting. if we are requesting 0 bytes, the entropy is already > available. at leat this is what makes sense to me. > > any further comments? > > > > -Sumit > > > > > return read; > > > msleep((1000 * (max - read)) / pvt_data->data_rate); > > > } else { > > > -- > > > 2.17.1 > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv2 2/2] hwrng: optee: fix wait use case 2020-07-24 14:23 ` Jorge Ramirez-Ortiz, Foundries 2020-07-28 10:05 ` Jorge Ramirez-Ortiz, Foundries @ 2020-08-05 13:49 ` Sumit Garg 2020-08-05 20:38 ` Jorge Ramirez-Ortiz, Foundries 1 sibling, 1 reply; 14+ messages in thread From: Sumit Garg @ 2020-08-05 13:49 UTC (permalink / raw) To: Jorge Ramirez-Ortiz, Foundries Cc: Matt Mackall, Herbert Xu, Jens Wiklander, Arnd Bergmann, ricardo, Michael Scott, Greg Kroah-Hartman, op-tee, open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Linux Kernel Mailing List Apologies for my delayed response as I was busy with some other tasks along with holidays. On Fri, 24 Jul 2020 at 19:53, Jorge Ramirez-Ortiz, Foundries <jorge@foundries.io> wrote: > > On 24/07/20, Sumit Garg wrote: > > On Thu, 23 Jul 2020 at 14:16, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote: > > > > > > The current code waits for data to be available before attempting a > > > second read. However the second read would not be executed as the > > > while loop exits. > > > > > > This fix does not wait if all data has been read and reads a second > > > time if only partial data was retrieved on the first read. > > > > > > This fix also does not attempt to read if not data is requested. > > > > I am not sure how this is possible, can you elaborate? > > currently, if the user sets max 0, get_optee_rng_data will regardless > issuese a call to the secure world requesting 0 bytes from the RNG > This case is already handled by core API: rng_dev_read(). > with this patch, this request is avoided. > > > > > > > > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> > > > --- > > > v2: tidy up the while loop to avoid reading when no data is requested > > > > > > drivers/char/hw_random/optee-rng.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c > > > index 5bc4700c4dae..a99d82949981 100644 > > > --- a/drivers/char/hw_random/optee-rng.c > > > +++ b/drivers/char/hw_random/optee-rng.c > > > @@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) > > > if (max > MAX_ENTROPY_REQ_SZ) > > > max = MAX_ENTROPY_REQ_SZ; > > > > > > - while (read == 0) { > > > + while (read < max) { > > > rng_size = get_optee_rng_data(pvt_data, data, (max - read)); > > > > > > data += rng_size; > > > read += rng_size; > > > > > > if (wait && pvt_data->data_rate) { > > > - if (timeout-- == 0) > > > + if ((timeout-- == 0) || (read == max)) > > > > If read == max, would there be any sleep? > > no but I see no reason why there should be a wait since we already have > all the data that we need; the msleep is only required when we need to > wait for the RNG to generate entropy for the number of bytes we are > requesting. if we are requesting 0 bytes, the entropy is already > available. at leat this is what makes sense to me. > Wouldn't it lead to a call as msleep(0); that means no wait as well? -Sumit > > > > > -Sumit > > > > > return read; > > > msleep((1000 * (max - read)) / pvt_data->data_rate); > > > } else { > > > -- > > > 2.17.1 > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv2 2/2] hwrng: optee: fix wait use case 2020-08-05 13:49 ` Sumit Garg @ 2020-08-05 20:38 ` Jorge Ramirez-Ortiz, Foundries 2020-08-06 6:11 ` Sumit Garg 0 siblings, 1 reply; 14+ messages in thread From: Jorge Ramirez-Ortiz, Foundries @ 2020-08-05 20:38 UTC (permalink / raw) To: Sumit Garg Cc: Jorge Ramirez-Ortiz, Foundries, Matt Mackall, Herbert Xu, Jens Wiklander, Arnd Bergmann, ricardo, Michael Scott, Greg Kroah-Hartman, op-tee, open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Linux Kernel Mailing List On 05/08/20, Sumit Garg wrote: > Apologies for my delayed response as I was busy with some other tasks > along with holidays. no pb! was just making sure this wasnt falling through some cracks. > > On Fri, 24 Jul 2020 at 19:53, Jorge Ramirez-Ortiz, Foundries > <jorge@foundries.io> wrote: > > > > On 24/07/20, Sumit Garg wrote: > > > On Thu, 23 Jul 2020 at 14:16, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote: > > > > > > > > The current code waits for data to be available before attempting a > > > > second read. However the second read would not be executed as the > > > > while loop exits. > > > > > > > > This fix does not wait if all data has been read and reads a second > > > > time if only partial data was retrieved on the first read. > > > > > > > > This fix also does not attempt to read if not data is requested. > > > > > > I am not sure how this is possible, can you elaborate? > > > > currently, if the user sets max 0, get_optee_rng_data will regardless > > issuese a call to the secure world requesting 0 bytes from the RNG > > > > This case is already handled by core API: rng_dev_read(). ah ok good point, you are right but yeah, there is no consequence to the actual patch. > > > with this patch, this request is avoided. > > > > > > > > > > > > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> > > > > --- > > > > v2: tidy up the while loop to avoid reading when no data is requested > > > > > > > > drivers/char/hw_random/optee-rng.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c > > > > index 5bc4700c4dae..a99d82949981 100644 > > > > --- a/drivers/char/hw_random/optee-rng.c > > > > +++ b/drivers/char/hw_random/optee-rng.c > > > > @@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) > > > > if (max > MAX_ENTROPY_REQ_SZ) > > > > max = MAX_ENTROPY_REQ_SZ; > > > > > > > > - while (read == 0) { > > > > + while (read < max) { > > > > rng_size = get_optee_rng_data(pvt_data, data, (max - read)); > > > > > > > > data += rng_size; > > > > read += rng_size; > > > > > > > > if (wait && pvt_data->data_rate) { > > > > - if (timeout-- == 0) > > > > + if ((timeout-- == 0) || (read == max)) > > > > > > If read == max, would there be any sleep? > > > > no but I see no reason why there should be a wait since we already have > > all the data that we need; the msleep is only required when we need to > > wait for the RNG to generate entropy for the number of bytes we are > > requesting. if we are requesting 0 bytes, the entropy is already > > available. at leat this is what makes sense to me. > > > > Wouldn't it lead to a call as msleep(0); that means no wait as well? I dont understand: there is no reason to wait if read == max and this patch will not wait: if read == max it calls 'return read' am I misunderstanding your point? > > -Sumit > > > > > > > > > -Sumit > > > > > > > return read; > > > > msleep((1000 * (max - read)) / pvt_data->data_rate); > > > > } else { > > > > -- > > > > 2.17.1 > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv2 2/2] hwrng: optee: fix wait use case 2020-08-05 20:38 ` Jorge Ramirez-Ortiz, Foundries @ 2020-08-06 6:11 ` Sumit Garg 2020-08-06 6:30 ` Jorge Ramirez-Ortiz, Foundries 0 siblings, 1 reply; 14+ messages in thread From: Sumit Garg @ 2020-08-06 6:11 UTC (permalink / raw) To: Jorge Ramirez-Ortiz, Foundries Cc: Matt Mackall, Herbert Xu, Jens Wiklander, Arnd Bergmann, ricardo, Michael Scott, Greg Kroah-Hartman, op-tee, open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Linux Kernel Mailing List On Thu, 6 Aug 2020 at 02:08, Jorge Ramirez-Ortiz, Foundries <jorge@foundries.io> wrote: > > On 05/08/20, Sumit Garg wrote: > > Apologies for my delayed response as I was busy with some other tasks > > along with holidays. > > no pb! was just making sure this wasnt falling through some cracks. > > > > > On Fri, 24 Jul 2020 at 19:53, Jorge Ramirez-Ortiz, Foundries > > <jorge@foundries.io> wrote: > > > > > > On 24/07/20, Sumit Garg wrote: > > > > On Thu, 23 Jul 2020 at 14:16, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote: > > > > > > > > > > The current code waits for data to be available before attempting a > > > > > second read. However the second read would not be executed as the > > > > > while loop exits. > > > > > > > > > > This fix does not wait if all data has been read and reads a second > > > > > time if only partial data was retrieved on the first read. > > > > > > > > > > This fix also does not attempt to read if not data is requested. > > > > > > > > I am not sure how this is possible, can you elaborate? > > > > > > currently, if the user sets max 0, get_optee_rng_data will regardless > > > issuese a call to the secure world requesting 0 bytes from the RNG > > > > > > > This case is already handled by core API: rng_dev_read(). > > ah ok good point, you are right > but yeah, there is no consequence to the actual patch. > So, at least you could get rid of the corresponding text from commit message. > > > > > with this patch, this request is avoided. > > > > > > > > > > > > > > > > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> > > > > > --- > > > > > v2: tidy up the while loop to avoid reading when no data is requested > > > > > > > > > > drivers/char/hw_random/optee-rng.c | 4 ++-- > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c > > > > > index 5bc4700c4dae..a99d82949981 100644 > > > > > --- a/drivers/char/hw_random/optee-rng.c > > > > > +++ b/drivers/char/hw_random/optee-rng.c > > > > > @@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) > > > > > if (max > MAX_ENTROPY_REQ_SZ) > > > > > max = MAX_ENTROPY_REQ_SZ; > > > > > > > > > > - while (read == 0) { > > > > > + while (read < max) { > > > > > rng_size = get_optee_rng_data(pvt_data, data, (max - read)); > > > > > > > > > > data += rng_size; > > > > > read += rng_size; > > > > > > > > > > if (wait && pvt_data->data_rate) { > > > > > - if (timeout-- == 0) > > > > > + if ((timeout-- == 0) || (read == max)) > > > > > > > > If read == max, would there be any sleep? > > > > > > no but I see no reason why there should be a wait since we already have > > > all the data that we need; the msleep is only required when we need to > > > wait for the RNG to generate entropy for the number of bytes we are > > > requesting. if we are requesting 0 bytes, the entropy is already > > > available. at leat this is what makes sense to me. > > > > > > > Wouldn't it lead to a call as msleep(0); that means no wait as well? > > I dont understand: there is no reason to wait if read == max and this > patch will not wait: if read == max it calls 'return read' > > am I misunderstanding your point? What I mean is that we shouldn't require this extra check here as there wasn't any wait if read == max with existing implementation too. -Sumit > > > > > -Sumit > > > > > > > > > > > > > -Sumit > > > > > > > > > return read; > > > > > msleep((1000 * (max - read)) / pvt_data->data_rate); > > > > > } else { > > > > > -- > > > > > 2.17.1 > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv2 2/2] hwrng: optee: fix wait use case 2020-08-06 6:11 ` Sumit Garg @ 2020-08-06 6:30 ` Jorge Ramirez-Ortiz, Foundries 2020-08-06 6:57 ` Sumit Garg 0 siblings, 1 reply; 14+ messages in thread From: Jorge Ramirez-Ortiz, Foundries @ 2020-08-06 6:30 UTC (permalink / raw) To: Sumit Garg Cc: Jorge Ramirez-Ortiz, Foundries, Matt Mackall, Herbert Xu, Jens Wiklander, Arnd Bergmann, ricardo, Michael Scott, Greg Kroah-Hartman, op-tee, open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Linux Kernel Mailing List On 06/08/20, Sumit Garg wrote: > On Thu, 6 Aug 2020 at 02:08, Jorge Ramirez-Ortiz, Foundries > <jorge@foundries.io> wrote: > > > > On 05/08/20, Sumit Garg wrote: > > > Apologies for my delayed response as I was busy with some other tasks > > > along with holidays. > > > > no pb! was just making sure this wasnt falling through some cracks. > > > > > > > > On Fri, 24 Jul 2020 at 19:53, Jorge Ramirez-Ortiz, Foundries > > > <jorge@foundries.io> wrote: > > > > > > > > On 24/07/20, Sumit Garg wrote: > > > > > On Thu, 23 Jul 2020 at 14:16, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote: > > > > > > > > > > > > The current code waits for data to be available before attempting a > > > > > > second read. However the second read would not be executed as the > > > > > > while loop exits. > > > > > > > > > > > > This fix does not wait if all data has been read and reads a second > > > > > > time if only partial data was retrieved on the first read. > > > > > > > > > > > > This fix also does not attempt to read if not data is requested. > > > > > > > > > > I am not sure how this is possible, can you elaborate? > > > > > > > > currently, if the user sets max 0, get_optee_rng_data will regardless > > > > issuese a call to the secure world requesting 0 bytes from the RNG > > > > > > > > > > This case is already handled by core API: rng_dev_read(). > > > > ah ok good point, you are right > > but yeah, there is no consequence to the actual patch. > > > > So, at least you could get rid of the corresponding text from commit message. > > > > > > > > with this patch, this request is avoided. > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> > > > > > > --- > > > > > > v2: tidy up the while loop to avoid reading when no data is requested > > > > > > > > > > > > drivers/char/hw_random/optee-rng.c | 4 ++-- > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c > > > > > > index 5bc4700c4dae..a99d82949981 100644 > > > > > > --- a/drivers/char/hw_random/optee-rng.c > > > > > > +++ b/drivers/char/hw_random/optee-rng.c > > > > > > @@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) > > > > > > if (max > MAX_ENTROPY_REQ_SZ) > > > > > > max = MAX_ENTROPY_REQ_SZ; > > > > > > > > > > > > - while (read == 0) { > > > > > > + while (read < max) { > > > > > > rng_size = get_optee_rng_data(pvt_data, data, (max - read)); > > > > > > > > > > > > data += rng_size; > > > > > > read += rng_size; > > > > > > > > > > > > if (wait && pvt_data->data_rate) { > > > > > > - if (timeout-- == 0) > > > > > > + if ((timeout-- == 0) || (read == max)) > > > > > > > > > > If read == max, would there be any sleep? > > > > > > > > no but I see no reason why there should be a wait since we already have > > > > all the data that we need; the msleep is only required when we need to > > > > wait for the RNG to generate entropy for the number of bytes we are > > > > requesting. if we are requesting 0 bytes, the entropy is already > > > > available. at leat this is what makes sense to me. > > > > > > > > > > Wouldn't it lead to a call as msleep(0); that means no wait as well? > > > > I dont understand: there is no reason to wait if read == max and this > > patch will not wait: if read == max it calls 'return read' > > > > am I misunderstanding your point? > > What I mean is that we shouldn't require this extra check here as > there wasn't any wait if read == max with existing implementation too. um, I am getting confused Sumit with the exisiting implementation (the one we aim to replace), if get_optee_rng_data reads all the values requested on the first call (ie, read = 0) with wait set to true, the call will wait with msleep(0). Which is unnecessary and waits for a jiffy (ie, the call to msleep 0 will schedule a one jiffy timeout interrruptible) with this alternative implementation, msleep(0) does not get called. are we in synch? > > -Sumit > > > > > > > > > -Sumit > > > > > > > > > > > > > > > > > -Sumit > > > > > > > > > > > return read; > > > > > > msleep((1000 * (max - read)) / pvt_data->data_rate); > > > > > > } else { > > > > > > -- > > > > > > 2.17.1 > > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv2 2/2] hwrng: optee: fix wait use case 2020-08-06 6:30 ` Jorge Ramirez-Ortiz, Foundries @ 2020-08-06 6:57 ` Sumit Garg 2020-08-06 8:14 ` Jorge Ramirez-Ortiz, Foundries 0 siblings, 1 reply; 14+ messages in thread From: Sumit Garg @ 2020-08-06 6:57 UTC (permalink / raw) To: Jorge Ramirez-Ortiz, Foundries Cc: Matt Mackall, Herbert Xu, Jens Wiklander, Arnd Bergmann, ricardo, Michael Scott, Greg Kroah-Hartman, op-tee, open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Linux Kernel Mailing List On Thu, 6 Aug 2020 at 12:00, Jorge Ramirez-Ortiz, Foundries <jorge@foundries.io> wrote: > > On 06/08/20, Sumit Garg wrote: > > On Thu, 6 Aug 2020 at 02:08, Jorge Ramirez-Ortiz, Foundries > > <jorge@foundries.io> wrote: > > > > > > On 05/08/20, Sumit Garg wrote: > > > > Apologies for my delayed response as I was busy with some other tasks > > > > along with holidays. > > > > > > no pb! was just making sure this wasnt falling through some cracks. > > > > > > > > > > > On Fri, 24 Jul 2020 at 19:53, Jorge Ramirez-Ortiz, Foundries > > > > <jorge@foundries.io> wrote: > > > > > > > > > > On 24/07/20, Sumit Garg wrote: > > > > > > On Thu, 23 Jul 2020 at 14:16, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote: > > > > > > > > > > > > > > The current code waits for data to be available before attempting a > > > > > > > second read. However the second read would not be executed as the > > > > > > > while loop exits. > > > > > > > > > > > > > > This fix does not wait if all data has been read and reads a second > > > > > > > time if only partial data was retrieved on the first read. > > > > > > > > > > > > > > This fix also does not attempt to read if not data is requested. > > > > > > > > > > > > I am not sure how this is possible, can you elaborate? > > > > > > > > > > currently, if the user sets max 0, get_optee_rng_data will regardless > > > > > issuese a call to the secure world requesting 0 bytes from the RNG > > > > > > > > > > > > > This case is already handled by core API: rng_dev_read(). > > > > > > ah ok good point, you are right > > > but yeah, there is no consequence to the actual patch. > > > > > > > So, at least you could get rid of the corresponding text from commit message. > > > > > > > > > > > with this patch, this request is avoided. > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> > > > > > > > --- > > > > > > > v2: tidy up the while loop to avoid reading when no data is requested > > > > > > > > > > > > > > drivers/char/hw_random/optee-rng.c | 4 ++-- > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c > > > > > > > index 5bc4700c4dae..a99d82949981 100644 > > > > > > > --- a/drivers/char/hw_random/optee-rng.c > > > > > > > +++ b/drivers/char/hw_random/optee-rng.c > > > > > > > @@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) > > > > > > > if (max > MAX_ENTROPY_REQ_SZ) > > > > > > > max = MAX_ENTROPY_REQ_SZ; > > > > > > > > > > > > > > - while (read == 0) { > > > > > > > + while (read < max) { > > > > > > > rng_size = get_optee_rng_data(pvt_data, data, (max - read)); > > > > > > > > > > > > > > data += rng_size; > > > > > > > read += rng_size; > > > > > > > > > > > > > > if (wait && pvt_data->data_rate) { > > > > > > > - if (timeout-- == 0) > > > > > > > + if ((timeout-- == 0) || (read == max)) > > > > > > > > > > > > If read == max, would there be any sleep? > > > > > > > > > > no but I see no reason why there should be a wait since we already have > > > > > all the data that we need; the msleep is only required when we need to > > > > > wait for the RNG to generate entropy for the number of bytes we are > > > > > requesting. if we are requesting 0 bytes, the entropy is already > > > > > available. at leat this is what makes sense to me. > > > > > > > > > > > > > Wouldn't it lead to a call as msleep(0); that means no wait as well? > > > > > > I dont understand: there is no reason to wait if read == max and this > > > patch will not wait: if read == max it calls 'return read' > > > > > > am I misunderstanding your point? > > > > What I mean is that we shouldn't require this extra check here as > > there wasn't any wait if read == max with existing implementation too. > > um, I am getting confused Sumit > > with the exisiting implementation (the one we aim to replace), if get_optee_rng_data reads all the values requested on the first call (ie, read = 0) with wait set to true, the call will wait with msleep(0). Which is unnecessary and waits for a jiffy (ie, the call to msleep 0 will schedule a one jiffy timeout interrruptible) > > with this alternative implementation, msleep(0) does not get called. > > are we in synch? Ah, I see msleep(0) also by default schedules timeout for 1 jiffy. So we are in sync now. Probably you can clarify this in commit message as well to avoid confusion. -Sumit > > > > > -Sumit > > > > > > > > > > > > > -Sumit > > > > > > > > > > > > > > > > > > > > > -Sumit > > > > > > > > > > > > > return read; > > > > > > > msleep((1000 * (max - read)) / pvt_data->data_rate); > > > > > > > } else { > > > > > > > -- > > > > > > > 2.17.1 > > > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv2 2/2] hwrng: optee: fix wait use case 2020-08-06 6:57 ` Sumit Garg @ 2020-08-06 8:14 ` Jorge Ramirez-Ortiz, Foundries 2020-08-06 9:15 ` Sumit Garg 0 siblings, 1 reply; 14+ messages in thread From: Jorge Ramirez-Ortiz, Foundries @ 2020-08-06 8:14 UTC (permalink / raw) To: Sumit Garg Cc: Jorge Ramirez-Ortiz, Foundries, Matt Mackall, Herbert Xu, Jens Wiklander, Arnd Bergmann, ricardo, Michael Scott, Greg Kroah-Hartman, op-tee, open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Linux Kernel Mailing List On 06/08/20, Sumit Garg wrote: > On Thu, 6 Aug 2020 at 12:00, Jorge Ramirez-Ortiz, Foundries > <jorge@foundries.io> wrote: > > > > On 06/08/20, Sumit Garg wrote: > > > On Thu, 6 Aug 2020 at 02:08, Jorge Ramirez-Ortiz, Foundries > > > <jorge@foundries.io> wrote: > > > > > > > > On 05/08/20, Sumit Garg wrote: > > > > > Apologies for my delayed response as I was busy with some other tasks > > > > > along with holidays. > > > > > > > > no pb! was just making sure this wasnt falling through some cracks. > > > > > > > > > > > > > > On Fri, 24 Jul 2020 at 19:53, Jorge Ramirez-Ortiz, Foundries > > > > > <jorge@foundries.io> wrote: > > > > > > > > > > > > On 24/07/20, Sumit Garg wrote: > > > > > > > On Thu, 23 Jul 2020 at 14:16, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote: > > > > > > > > > > > > > > > > The current code waits for data to be available before attempting a > > > > > > > > second read. However the second read would not be executed as the > > > > > > > > while loop exits. > > > > > > > > > > > > > > > > This fix does not wait if all data has been read and reads a second > > > > > > > > time if only partial data was retrieved on the first read. > > > > > > > > > > > > > > > > This fix also does not attempt to read if not data is requested. > > > > > > > > > > > > > > I am not sure how this is possible, can you elaborate? > > > > > > > > > > > > currently, if the user sets max 0, get_optee_rng_data will regardless > > > > > > issuese a call to the secure world requesting 0 bytes from the RNG > > > > > > > > > > > > > > > > This case is already handled by core API: rng_dev_read(). > > > > > > > > ah ok good point, you are right > > > > but yeah, there is no consequence to the actual patch. > > > > > > > > > > So, at least you could get rid of the corresponding text from commit message. > > > > > > > > > > > > > > with this patch, this request is avoided. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> > > > > > > > > --- > > > > > > > > v2: tidy up the while loop to avoid reading when no data is requested > > > > > > > > > > > > > > > > drivers/char/hw_random/optee-rng.c | 4 ++-- > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c > > > > > > > > index 5bc4700c4dae..a99d82949981 100644 > > > > > > > > --- a/drivers/char/hw_random/optee-rng.c > > > > > > > > +++ b/drivers/char/hw_random/optee-rng.c > > > > > > > > @@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) > > > > > > > > if (max > MAX_ENTROPY_REQ_SZ) > > > > > > > > max = MAX_ENTROPY_REQ_SZ; > > > > > > > > > > > > > > > > - while (read == 0) { > > > > > > > > + while (read < max) { > > > > > > > > rng_size = get_optee_rng_data(pvt_data, data, (max - read)); > > > > > > > > > > > > > > > > data += rng_size; > > > > > > > > read += rng_size; > > > > > > > > > > > > > > > > if (wait && pvt_data->data_rate) { > > > > > > > > - if (timeout-- == 0) > > > > > > > > + if ((timeout-- == 0) || (read == max)) > > > > > > > > > > > > > > If read == max, would there be any sleep? > > > > > > > > > > > > no but I see no reason why there should be a wait since we already have > > > > > > all the data that we need; the msleep is only required when we need to > > > > > > wait for the RNG to generate entropy for the number of bytes we are > > > > > > requesting. if we are requesting 0 bytes, the entropy is already > > > > > > available. at leat this is what makes sense to me. > > > > > > > > > > > > > > > > Wouldn't it lead to a call as msleep(0); that means no wait as well? > > > > > > > > I dont understand: there is no reason to wait if read == max and this > > > > patch will not wait: if read == max it calls 'return read' > > > > > > > > am I misunderstanding your point? > > > > > > What I mean is that we shouldn't require this extra check here as > > > there wasn't any wait if read == max with existing implementation too. > > > > um, I am getting confused Sumit > > > > with the exisiting implementation (the one we aim to replace), if get_optee_rng_data reads all the values requested on the first call (ie, read = 0) with wait set to true, the call will wait with msleep(0). Which is unnecessary and waits for a jiffy (ie, the call to msleep 0 will schedule a one jiffy timeout interrruptible) > > > > with this alternative implementation, msleep(0) does not get called. > > > > are we in synch? > > Ah, I see msleep(0) also by default schedules timeout for 1 jiffy. So > we are in sync now. Probably you can clarify this in commit message as > well to avoid confusion. ok will do. shall I add your reviewed-by line or just resend? > > -Sumit > > > > > > > > > -Sumit > > > > > > > > > > > > > > > > > -Sumit > > > > > > > > > > > > > > > > > > > > > > > > > -Sumit > > > > > > > > > > > > > > > return read; > > > > > > > > msleep((1000 * (max - read)) / pvt_data->data_rate); > > > > > > > > } else { > > > > > > > > -- > > > > > > > > 2.17.1 > > > > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv2 2/2] hwrng: optee: fix wait use case 2020-08-06 8:14 ` Jorge Ramirez-Ortiz, Foundries @ 2020-08-06 9:15 ` Sumit Garg 0 siblings, 0 replies; 14+ messages in thread From: Sumit Garg @ 2020-08-06 9:15 UTC (permalink / raw) To: Jorge Ramirez-Ortiz, Foundries Cc: Matt Mackall, Herbert Xu, Jens Wiklander, Arnd Bergmann, ricardo, Michael Scott, Greg Kroah-Hartman, op-tee, open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Linux Kernel Mailing List On Thu, 6 Aug 2020 at 13:44, Jorge Ramirez-Ortiz, Foundries <jorge@foundries.io> wrote: > > On 06/08/20, Sumit Garg wrote: > > On Thu, 6 Aug 2020 at 12:00, Jorge Ramirez-Ortiz, Foundries > > <jorge@foundries.io> wrote: > > > > > > On 06/08/20, Sumit Garg wrote: > > > > On Thu, 6 Aug 2020 at 02:08, Jorge Ramirez-Ortiz, Foundries > > > > <jorge@foundries.io> wrote: > > > > > > > > > > On 05/08/20, Sumit Garg wrote: > > > > > > Apologies for my delayed response as I was busy with some other tasks > > > > > > along with holidays. > > > > > > > > > > no pb! was just making sure this wasnt falling through some cracks. > > > > > > > > > > > > > > > > > On Fri, 24 Jul 2020 at 19:53, Jorge Ramirez-Ortiz, Foundries > > > > > > <jorge@foundries.io> wrote: > > > > > > > > > > > > > > On 24/07/20, Sumit Garg wrote: > > > > > > > > On Thu, 23 Jul 2020 at 14:16, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote: > > > > > > > > > > > > > > > > > > The current code waits for data to be available before attempting a > > > > > > > > > second read. However the second read would not be executed as the > > > > > > > > > while loop exits. > > > > > > > > > > > > > > > > > > This fix does not wait if all data has been read and reads a second > > > > > > > > > time if only partial data was retrieved on the first read. > > > > > > > > > > > > > > > > > > This fix also does not attempt to read if not data is requested. > > > > > > > > > > > > > > > > I am not sure how this is possible, can you elaborate? > > > > > > > > > > > > > > currently, if the user sets max 0, get_optee_rng_data will regardless > > > > > > > issuese a call to the secure world requesting 0 bytes from the RNG > > > > > > > > > > > > > > > > > > > This case is already handled by core API: rng_dev_read(). > > > > > > > > > > ah ok good point, you are right > > > > > but yeah, there is no consequence to the actual patch. > > > > > > > > > > > > > So, at least you could get rid of the corresponding text from commit message. > > > > > > > > > > > > > > > > > with this patch, this request is avoided. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> > > > > > > > > > --- > > > > > > > > > v2: tidy up the while loop to avoid reading when no data is requested > > > > > > > > > > > > > > > > > > drivers/char/hw_random/optee-rng.c | 4 ++-- > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c > > > > > > > > > index 5bc4700c4dae..a99d82949981 100644 > > > > > > > > > --- a/drivers/char/hw_random/optee-rng.c > > > > > > > > > +++ b/drivers/char/hw_random/optee-rng.c > > > > > > > > > @@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) > > > > > > > > > if (max > MAX_ENTROPY_REQ_SZ) > > > > > > > > > max = MAX_ENTROPY_REQ_SZ; > > > > > > > > > > > > > > > > > > - while (read == 0) { > > > > > > > > > + while (read < max) { > > > > > > > > > rng_size = get_optee_rng_data(pvt_data, data, (max - read)); > > > > > > > > > > > > > > > > > > data += rng_size; > > > > > > > > > read += rng_size; > > > > > > > > > > > > > > > > > > if (wait && pvt_data->data_rate) { > > > > > > > > > - if (timeout-- == 0) > > > > > > > > > + if ((timeout-- == 0) || (read == max)) > > > > > > > > > > > > > > > > If read == max, would there be any sleep? > > > > > > > > > > > > > > no but I see no reason why there should be a wait since we already have > > > > > > > all the data that we need; the msleep is only required when we need to > > > > > > > wait for the RNG to generate entropy for the number of bytes we are > > > > > > > requesting. if we are requesting 0 bytes, the entropy is already > > > > > > > available. at leat this is what makes sense to me. > > > > > > > > > > > > > > > > > > > Wouldn't it lead to a call as msleep(0); that means no wait as well? > > > > > > > > > > I dont understand: there is no reason to wait if read == max and this > > > > > patch will not wait: if read == max it calls 'return read' > > > > > > > > > > am I misunderstanding your point? > > > > > > > > What I mean is that we shouldn't require this extra check here as > > > > there wasn't any wait if read == max with existing implementation too. > > > > > > um, I am getting confused Sumit > > > > > > with the exisiting implementation (the one we aim to replace), if get_optee_rng_data reads all the values requested on the first call (ie, read = 0) with wait set to true, the call will wait with msleep(0). Which is unnecessary and waits for a jiffy (ie, the call to msleep 0 will schedule a one jiffy timeout interrruptible) > > > > > > with this alternative implementation, msleep(0) does not get called. > > > > > > are we in synch? > > > > Ah, I see msleep(0) also by default schedules timeout for 1 jiffy. So > > we are in sync now. Probably you can clarify this in commit message as > > well to avoid confusion. > > ok will do. > shall I add your reviewed-by line or just resend? > Yes it's fine with me to add mine reviewed-by. > > > > -Sumit > > > > > > > > > > > > > -Sumit > > > > > > > > > > > > > > > > > > > > > -Sumit > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -Sumit > > > > > > > > > > > > > > > > > return read; > > > > > > > > > msleep((1000 * (max - read)) / pvt_data->data_rate); > > > > > > > > > } else { > > > > > > > > > -- > > > > > > > > > 2.17.1 > > > > > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv2 2/2] hwrng: optee: fix wait use case 2020-07-23 8:46 ` [PATCHv2 2/2] hwrng: optee: fix wait use case Jorge Ramirez-Ortiz 2020-07-24 13:22 ` Sumit Garg @ 2020-08-05 13:34 ` Jorge Ramirez-Ortiz, Foundries 1 sibling, 0 replies; 14+ messages in thread From: Jorge Ramirez-Ortiz, Foundries @ 2020-08-05 13:34 UTC (permalink / raw) To: Jorge Ramirez-Ortiz Cc: sumit.garg, mpm, herbert, jens.wiklander, arnd, ricardo, mike, gregkh, op-tee, linux-crypto, linux-kernel On 23/07/20, Jorge Ramirez-Ortiz wrote: > The current code waits for data to be available before attempting a > second read. However the second read would not be executed as the > while loop exits. > > This fix does not wait if all data has been read and reads a second > time if only partial data was retrieved on the first read. > > This fix also does not attempt to read if not data is requested. > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> > --- > v2: tidy up the while loop to avoid reading when no data is requested > > drivers/char/hw_random/optee-rng.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c > index 5bc4700c4dae..a99d82949981 100644 > --- a/drivers/char/hw_random/optee-rng.c > +++ b/drivers/char/hw_random/optee-rng.c > @@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) > if (max > MAX_ENTROPY_REQ_SZ) > max = MAX_ENTROPY_REQ_SZ; > > - while (read == 0) { > + while (read < max) { > rng_size = get_optee_rng_data(pvt_data, data, (max - read)); > > data += rng_size; > read += rng_size; > > if (wait && pvt_data->data_rate) { > - if (timeout-- == 0) > + if ((timeout-- == 0) || (read == max)) > return read; > msleep((1000 * (max - read)) / pvt_data->data_rate); > } else { any comments please? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv2 1/2] hwrng: optee: handle unlimited data rates 2020-07-23 8:46 [PATCHv2 1/2] hwrng: optee: handle unlimited data rates Jorge Ramirez-Ortiz 2020-07-23 8:46 ` [PATCHv2 2/2] hwrng: optee: fix wait use case Jorge Ramirez-Ortiz @ 2020-07-24 13:24 ` Sumit Garg 1 sibling, 0 replies; 14+ messages in thread From: Sumit Garg @ 2020-07-24 13:24 UTC (permalink / raw) To: Jorge Ramirez-Ortiz Cc: Matt Mackall, Herbert Xu, Jens Wiklander, Arnd Bergmann, ricardo, Michael Scott, Greg Kroah-Hartman, op-tee, open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Linux Kernel Mailing List On Thu, 23 Jul 2020 at 14:16, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote: > > Data rates of MAX_UINT32 will schedule an unnecessary one jiffy > timeout on the call to msleep. Avoid this scenario by using 0 as the > unlimited data rate. > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> > --- > drivers/char/hw_random/optee-rng.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Sounds good to me. FWIW: Reviewed-by: Sumit Garg <sumit.garg@linaro.org> -Sumit > diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c > index 49b2e02537dd..5bc4700c4dae 100644 > --- a/drivers/char/hw_random/optee-rng.c > +++ b/drivers/char/hw_random/optee-rng.c > @@ -128,7 +128,7 @@ static int optee_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) > data += rng_size; > read += rng_size; > > - if (wait) { > + if (wait && pvt_data->data_rate) { > if (timeout-- == 0) > return read; > msleep((1000 * (max - read)) / pvt_data->data_rate); > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-08-06 11:13 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-23 8:46 [PATCHv2 1/2] hwrng: optee: handle unlimited data rates Jorge Ramirez-Ortiz 2020-07-23 8:46 ` [PATCHv2 2/2] hwrng: optee: fix wait use case Jorge Ramirez-Ortiz 2020-07-24 13:22 ` Sumit Garg 2020-07-24 14:23 ` Jorge Ramirez-Ortiz, Foundries 2020-07-28 10:05 ` Jorge Ramirez-Ortiz, Foundries 2020-08-05 13:49 ` Sumit Garg 2020-08-05 20:38 ` Jorge Ramirez-Ortiz, Foundries 2020-08-06 6:11 ` Sumit Garg 2020-08-06 6:30 ` Jorge Ramirez-Ortiz, Foundries 2020-08-06 6:57 ` Sumit Garg 2020-08-06 8:14 ` Jorge Ramirez-Ortiz, Foundries 2020-08-06 9:15 ` Sumit Garg 2020-08-05 13:34 ` Jorge Ramirez-Ortiz, Foundries 2020-07-24 13:24 ` [PATCHv2 1/2] hwrng: optee: handle unlimited data rates Sumit Garg
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.