Re: [PATCH v2] virtio-rng: return available data with O_NONBLOCK

[Laurent Vivier]

On 11/08/2020 14:22, Martin Wilck wrote:
> On Tue, 2020-08-11 at 14:02 +0200, Laurent Vivier wrote:
>>
>>>>  drivers/char/hw_random/virtio-rng.c | 14 ++++++++++++++
>>>>  1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/drivers/char/hw_random/virtio-rng.c
>>>> b/drivers/char/hw_random/virtio-rng.c
>>>> index 79a6e47b5fbc..984713b35892 100644
>>>> --- a/drivers/char/hw_random/virtio-rng.c
>>>> +++ b/drivers/char/hw_random/virtio-rng.c
>>>> @@ -59,6 +59,20 @@ static int virtio_read(struct hwrng *rng, void
>>>> *buf, size_t size, bool wait)
>>>>    if (vi->hwrng_removed)
>>>>            return -ENODEV;
>>>>  
>>>> +  /*
>>>> +   * If the previous call was non-blocking, we may have got some
>>>> +   * randomness already.
>>>> +   */
>>>> +  if (vi->busy && completion_done(&vi->have_data)) {
>>>> +          unsigned int len;
>>>> +
>>>> +          vi->busy = false;
>>>> +          len = vi->data_avail > size ? size : vi->data_avail;
>>>> +          vi->data_avail -= len;
>>
>> You don't need to modify data_avail. As busy is set to false, the
>> buffer
>> will be reused. and it is always overwritten by virtqueue_get_buf().
>> And moreover, if it was reused it would be always the beginning.
> 
> Ok.
> 
>>
>>>> +          if (len)
>>>> +                  return len;
>>>> +  }
>>>> +
>>>>    if (!vi->busy) {
>>>>            vi->busy = true;
>>>>            reinit_completion(&vi->have_data);
>>>>
>>
>> Why don't you modify only the wait case?
>>
>> Something like:
>>
>>      if (!wait && !completion_done(&vi->have_data)) {
>>              return 0;
>>         }
>>
>> then at the end you can do "return min(size, vi->data_avail);".
> 
> Sorry, I don't understand what you mean. Where would you insert the
> above "if" clause? Are you saying I should call
> wait_for_completion_killable() also in the (!wait) case?

Yes, but only if a the completion is done, so it will not wait.

> 
> I must call check completion_done() before calling reinit_completion().

Normally, the busy flag is here for that. If busy is true, a buffer is
already registered. reinit_completion() must not be called if busy is
true. busy becomes false when the before is ready to be reused.

> OTOH, if completion_done() returns false, I can't simply return 0, I
> must at least start fetching new random data, so that a subsequent
> virtio_read() call has a chance to return something.

if you modify "if (!wait)" to becomes "if (!wait &&
!completion_done(&vi->have_data))", either we have already a registered
buffer from a previous call or the one we have registered if busy is
false. So you can return 0 as nothing is ready but we have a registered
buffer for the next time.

Thanks,
Laurent