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
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
