Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <60587922.2040903@gmail.com>
Date:   Mon, 22 Mar 2021 13:01:54 +0200
From:   Eli Billauer <eli.billauer@gmail.com>
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.12) Gecko/20100907 Fedora/3.0.7-1.fc12 Thunderbird/3.0.7
MIME-Version: 1.0
To:     Greg KH <gregkh@linuxfoundation.org>
CC:     arnd@arndb.de, devel@driverdev.osuosl.org,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        dan.carpenter@oracle.com
Subject: Re: [PATCH v4 2/2] staging: Add driver for XillyUSB (Xillybus variant
 for USB)
References: <20210311095033.20956-1-eli.billauer@gmail.com> <20210311095033.20956-3-eli.billauer@gmail.com> <YFc6z9REEo27FMA+@kroah.com>
In-Reply-To: <YFc6z9REEo27FMA+@kroah.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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