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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?

As for the specific remark on fifo->fill changing after reading it -- that's OK, since it would just mean that the reader of the FIFO doesn't see the extra data that has just been written to it. This doesn't break anything.

Moving on to the second topic:

+static loff_t xillyusb_llseek(struct file *filp, loff_t offset, int whence)
USB devices do not have a "seek", why is this needed?

Xillybus' device nodes are optionally seekable, which gives the user application access to a RAM array on the FPGA (or logic emulating it, such as registers). This holds true for the existing Xillybus API as well as the one for XillyUSB.

Thanks and regards,
   Eli



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux