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

[Laurent Vivier <lvivier@redhat.com>]

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