Re: [PATCHv2 2/2] hwrng: optee: fix wait use case

On Thu, 6 Aug 2020 at 13:44, Jorge Ramirez-Ortiz, Foundries
[off-list ref] wrote:

On 06/08/20, Sumit Garg wrote:

quoted

On Thu, 6 Aug 2020 at 12:00, Jorge Ramirez-Ortiz, Foundries
[off-list ref] wrote:

quoted

On 06/08/20, Sumit Garg wrote:

quoted

On Thu, 6 Aug 2020 at 02:08, Jorge Ramirez-Ortiz, Foundries
[off-list ref] wrote:

quoted

On 05/08/20, Sumit Garg wrote:

quoted

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.

quoted

On Fri, 24 Jul 2020 at 19:53, Jorge Ramirez-Ortiz, Foundries
[off-list ref] wrote:

quoted

On 24/07/20, Sumit Garg wrote:

quoted

On Thu, 23 Jul 2020 at 14:16, Jorge Ramirez-Ortiz [off-list ref] wrote:

quoted

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.

quoted

quoted

quoted

with this patch, this request is avoided.

quoted

quoted

Signed-off-by: Jorge Ramirez-Ortiz <redacted>
---
 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.

quoted

-Sumit

quoted

quoted

-Sumit

quoted

quoted

-Sumit

quoted

quoted

-Sumit

quoted

                                return read;
                        msleep((1000 * (max - read)) / pvt_data->data_rate);
                } else {
--
2.17.1