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

[Laurent Vivier]

On 11/08/2020 14:53, Martin Wilck wrote:
> On Tue, 2020-08-11 at 14:39 +0200, Laurent Vivier wrote:
>> 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.
>>
> 
> Slowly getting there, thanks for your patience. Yes, I guess this would
> work, too. I'll test and get back to you.

No problem. This code is tricky and it took me several months to really
start to understand it ...

Thanks,
Laurent