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);
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 {
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
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
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
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
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
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
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
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 >
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 > >
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 > > >
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);
any comments please?
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
op-tee@lists.trustedfirmware.org
-
Jorge Ramirez-Ortiz -
Jorge Ramirez-Ortiz, Foundries
-
Sumit Garg