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

[Martin Wilck]

On Tue, 2020-08-11 at 14:53 +0200, 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.
> 
> > > 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.
> 
> My thinking was that once the completion is done, "busy" doesn't
> reflect the actual state any more. But your idea is leaner and keeps
> the semantics of "busy". I'll give it a try.

Confirmed, your code solves the issue as well. I'm going to submit a
v3, although nn light of the dicussion with Michael, I assume that
you're going to go at it yourself to solve the other issues.

Regards and thanks,
Martin