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

[Michael S. Tsirkin]

On Tue, Aug 11, 2020 at 02:07:23PM +0200, Martin Wilck wrote:
> On Tue, 2020-08-11 at 07:26 -0400, Michael S. Tsirkin wrote:
> > On Wed, Jul 15, 2020 at 03:32:55PM +0200, mwilck@suse.com 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;
> > 
> > I wonder what purpose does this line serve: busy is false
> > which basically means data_avail is invalid, right?
> > A following non blocking call will not enter here.
> 
> Well, I thought this is just how reading data normally works. But
> you're right, the remainder will not be used. I can remove the line, or
> reset data_avail to 0 at this point. What do you prefer?
> 
> Regards,
> Martin

Removing seems cleaner.

But looking at it, it is using the API in a very strange way:
a buffer is placed in the ring by one call, and *assumed*
to still be there in the next call.

which it might not be if one call is from userspace and the
next one is from fill kthread.

I guess this is why it's returning 0: yes it knows there's
data, but it does not know where it is.

-- 
MST