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

[Martin Wilck]

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?

I must call check completion_done() before calling reinit_completion().
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.

Thanks,
Martin