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

[Martin Wilck <mwilck@suse.com>]

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.

Thanks,
Martin