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