Re: [PATCH v4 2/2] staging: Add driver for XillyUSB (Xillybus variant for USB)

[Greg KH]

On Mon, Mar 22, 2021 at 01:01:54PM +0200, Eli Billauer wrote:
> Hello, Greg.
> 
> Thanks for your comments. I'd like to address a couple of them.
> 
> First, there's the lockless FIFO that is implemented in the driver:
> 
> On 21/03/21 14:23, Greg KH wrote:
> > 
> > > +
> > > +static unsigned int fifo_read(struct xillyfifo *fifo,
> > > +			      void *data, unsigned int len,
> > > +			      int (*copier)(void *, const void *, int))
> > > +{
> > > +	unsigned int done = 0;
> > > +	unsigned int todo = len;
> > > +	unsigned int fill;
> > > +	unsigned int readpos = fifo->readpos;
> > > +	unsigned int readbuf = fifo->readbuf;
> > > +
> > > +	fill = atomic_read(&fifo->fill);
> > And the number changed right after reading it :(
> > 
> > Again, no atomics, use a lock please.
> > 
> > This is a USB device, you are NOT doing high-speed data transfers at
> > all.
> > 
> The current XillyUSB hardware is USB 3.0 based, running at ~400 MB/s, and
> this is just the beginning. For comparison, when the PCIe-based Xillybus
> started at 200 MB/s, I didn't believe it would reach 6.6 GB/s.
> 
> So that's why I made the effort to implement a lockless FIFO, with all the
> extra synchronization fuss. And yes, it works perfectly, and has been
> heavily fuzz tested on an x86_64 machine. The memory barriers are carefully
> placed to make this work on less favorable platforms as well, but even if I
> got it wrong -- odds are that the fix will be a line or two.
> 
> Replacing atomics with spinlocks is a piece of cake, of course. But given
> this extra information, do you still think it's a good idea?

Trying to review this code is hard, if not impossible because of the
structure.  Again, USB interfaces are slow, a "custom lockless FIFO" is
something for the core kernel to implement, not for a random individual
driver, to ensure it is working properly.

And it seems like an overkill, are you sure those locks are a slowdown?

thanks,

greg k-h