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
