[RFC PATCH v1] Xilinx AXI-Stream FIFO v4.1 IP core driver

[Jacob Feder]

On Mon, Jul 09, 2018 at 05:25:21PM +0300, Dan Carpenter wrote:
> On Sat, Jul 07, 2018 at 10:19:40PM -0400, Jacob Feder wrote:
> > +static ssize_t sysfs_write(struct device *dev, const char *buf,
> > +			   size_t count, unsigned int addr_offset)
> > +{
> > +	struct axis_fifo_local *device_wrapper = dev_get_drvdata(dev);
> 
> What does the "local" mean in axis_fifo_local?  "device_wrapper" is a
> weird name.  It's sort of long but also meaningless and or confusing.
> What kind of device?  What is it wrapping?  How is it not a fifo?  And
> because it's a bit long then we run into the 80 character limit.  Maybe
> just say "struct axis_fifo_local *fifo;"?

fixed

> 
> > +
> > +	if (!mutex_trylock(&device_wrapper->write_mutex)) {
> > +		dev_err(device_wrapper->os_device,
> > +			"couldn't acquire write lock\n");
> > +		return -EBUSY;
> > +	}
> 
> Greg has said to remove this.
> 

fixed

> > +
> > +	if (!mutex_trylock(&device_wrapper->read_mutex)) {
> > +		dev_err(device_wrapper->os_device,
> > +			"couldn't acquire read lock\n");
> > +		mutex_unlock(&device_wrapper->write_mutex);
> > +		dev_dbg(device_wrapper->os_device, "released write lock\n");
> > +		return -EBUSY;
> > +	}
> > +
> > +	dev_dbg(device_wrapper->os_device, "acquired locks\n");
> 
> Remove this, because it's related to locks.  Also you can get this info
> from ftrace.  The extra debug code just obscures the interest parts of
> the code because there is too much debugging.
> 
> ->os_device is a bad name, because there isn't an operating system
> device.  What does that even mean?
> 

fixed

> > +
> > +	if (count != 4) {
> 
> Remove the magic number.  Just say sizeof(u32).
> 

fixed

> > +		dev_err(device_wrapper->os_device,
> > +			"error, sysfs write to address 0x%x expected 4 bytes\n",
> > +			addr_offset);
> > +		mutex_unlock(&device_wrapper->write_mutex);
> > +		mutex_unlock(&device_wrapper->read_mutex);
> 
> Don't let user space spam /var/log messages.  Just return -EINVAL;
> 

fixed

> > +		return -EINVAL;
> > +	}
> > +
> > +	dev_dbg(device_wrapper->os_device,
> > +		"writing 0x%x to sysfs address 0x%x\n",
> > +		*(unsigned int *)buf, addr_offset);
> > +	iowrite32(*(unsigned int __force *)buf,
> 
> __force is for when you're casting to and from __user types.  It's not
> required here.
> 

fixed

> > +		  device_wrapper->base_addr + addr_offset);
> > +	mutex_unlock(&device_wrapper->write_mutex);
> > +	mutex_unlock(&device_wrapper->read_mutex);
> > +	dev_dbg(device_wrapper->os_device, "released locks\n");
> 
> ftrace.
> 
> > +
> > +	return 4;
> 
> This is a magic number.  Use "return count;" instead.
> 

fixed

> > +}
> 
> So now it's:
> 
> static ssize_t sysfs_write(struct device *dev, const char *buf,
> 			   size_t count, unsigned int addr_offset)
> {
> 	struct axis_fifo_local *fifo = dev_get_drvdata(dev);
> 
> 	if (count != sizeof(u32))
> 		return -EINVAL;
> 
> 	iowrite32(*(u32 *)buf, fifo->base_addr + addr_offset);
> 
> 	return count;
> }
> 
> Sysfs files are supposed to be human readable text files so I don't
> feel like casting *(u32 *)buf is the right thing.  In other words you do
> echo "1234" > /sys/whatever.  What are we trying to write here?
> Normally we would use kstrotul() and write the value that
> way.
> 

fixed

> > +// reads a single packet from the fifo as dictated by the tlast signal
> 
> Please use /* */ style comments.
> 

fixed

> > +static ssize_t axis_fifo_read(struct file *device_file, char __user *buf,
> > +			      size_t len, loff_t *off)
> > +{
> > +	struct axis_fifo_local *device_wrapper =
> > +			(struct axis_fifo_local *)device_file->private_data;
> > +	unsigned int bytes_available;
> > +	unsigned int words_available;
> > +	unsigned int word;
> > +	unsigned int buff_word;
> 
> One f in buf.  Otherwise it means a person who works out a lot.  This
> is just an iterator.  "int i;"
> 

fixed

> > +	int wait_ret;
> 
> Just say "int ret;"
> 

fixed

> > +	u32 read_buff[READ_BUFF_SIZE];
> > +
> > +	if (device_wrapper->read_flags & O_NONBLOCK) {
> > +		// opened in non-blocking mode
> > +		// return if there are no packets available
> > +		if (!ioread32(device_wrapper->base_addr + XLLF_RDFO_OFFSET))
> > +			return -EAGAIN;
> > +	} else {
> > +		// opened in blocking mode
> > +
> > +		// wait for a packet available interrupt (or timeout)
> > +		// if nothing is currently available
> > +		spin_lock_irq(&device_wrapper->read_queue_lock);
> > +		if (read_timeout < 0) {
> 
> There are too many O_NONBLOCK and read_timeout modes...
> 

Not sure what to do about this - I think they are all necessary?

> > +			wait_ret = wait_event_interruptible_lock_irq_timeout(
> > +				device_wrapper->read_queue,
> > +				ioread32(device_wrapper->base_addr +
> > +					XLLF_RDFO_OFFSET),
> > +				device_wrapper->read_queue_lock,
> > +				MAX_SCHEDULE_TIMEOUT);
> > +		} else {
> > +			wait_ret = wait_event_interruptible_lock_irq_timeout(
> > +				device_wrapper->read_queue,
> > +				ioread32(device_wrapper->base_addr +
> > +					XLLF_RDFO_OFFSET),
> > +				device_wrapper->read_queue_lock,
> > +				msecs_to_jiffies(read_timeout));
> > +		}
> > +		spin_unlock_irq(&device_wrapper->read_queue_lock);
> > +
> > +		if (wait_ret == 0) {
> > +			// timeout occurred
> > +			dev_dbg(device_wrapper->os_device, "read timeout");
> > +			return 0;
> 
> This should be a failure return, no?
> 

fixed

> > +		} else if (wait_ret == -ERESTARTSYS) {
> > +			// signal received
> > +			return -ERESTARTSYS;
> > +		} else if (wait_ret < 0) {
> > +			dev_err(device_wrapper->os_device,
> > +				"wait_event_interruptible_timeout() error in read (wait_ret=%i)\n",
> > +				wait_ret);
> > +			return wait_ret;
> > +		}
> > +	}
> > +
> > +	bytes_available = ioread32(device_wrapper->base_addr + XLLF_RLR_OFFSET);
> > +	if (!bytes_available) {
> > +		dev_err(device_wrapper->os_device,
> > +			"received a packet of length 0 - fifo core will be reset\n");
> > +		reset_ip_core(device_wrapper);
> > +		return -EIO;
> > +	}
> > +
> > +	if (bytes_available > len) {
> > +		dev_err(device_wrapper->os_device,
> > +			"user read buffer too small (available bytes=%u user buffer bytes=%u) - fifo core will be reset\n",
> > +			bytes_available, len);
> > +		reset_ip_core(device_wrapper);
> > +		return -EINVAL;
> 
> This is a weird thing.  Shouldn't we just return len bytes and not
> treat this as an error?  Whey are we resetting after this situation?
> 

The IP has the slightly annoying property that reading certain read-only
registers actually changes the state of the IP. The RLR register has been
read but the actual data hasn't been taken out of the FIFO, so it's state
is now corrupted.

> > +	}
> > +
> > +	if (bytes_available % 4) {
> > +		// this probably can't happen unless IP
> > +		// registers were previously mishandled
> > +		dev_err(device_wrapper->os_device,
> > +			"received a packet that isn't word-aligned - fifo core will be reset\n");
> > +		reset_ip_core(device_wrapper);
> > +		return -EIO;
> > +	}
> > +
> > +	words_available = bytes_available / 4;
> > +
> > +	// read data into an intermediate buffer, copying the contents
> > +	// to userspace when the buffer is full
> > +	for (word = 0; word < words_available; word++) {
> > +		buff_word = word % READ_BUFF_SIZE;
> > +		read_buff[buff_word] = ioread32(device_wrapper->base_addr +
> > +						XLLF_RDFD_OFFSET);
> > +		if ((buff_word == READ_BUFF_SIZE - 1) ||
> > +		    (word == words_available - 1)) {
> > +			if (copy_to_user(buf + (word - buff_word) * 4,
> > +					 read_buff,
> > +					 (buff_word + 1) * 4)) {
> > +				// this occurs if the user passes
> > +				// an invalid pointer
> > +				dev_err(device_wrapper->os_device,
> > +					"couldn't copy data to userspace buffer - fifo core will be reset\n");
> > +				reset_ip_core(device_wrapper);
> 
> No need to comment because kernel devs are expected to know about
> copy_to_user().  No need to print an error.  Why are we resetting?
> 

fixed - see previous about resetting.

> > +				return -EFAULT;
> > +			}
> > +		}
> > +	}
> 
> 	copied = 0;
> 	while (words_available > 0 {
> 		copy = min(words_available, READ_BUFF_SIZE);
> 
> 		for (i = 0; i < copy; i++) {
> 			tmp_buf[i] = ioread32(fifo->base_addr +
> 					      XLLF_RDFD_OFFSET);
> 		}
> 
> 		if (copy_to_user(buf + copied * 4, tmp_buf, copy))
> 			return -EFAULT;
> 
> 		copied += copy;
> 		words_available -= copy;
> 	}
> 
> 
> regards,
> dan carpenter
> 
> 

Thanks for the feedback!

I can submit all of these changes in a v2 patch unless there's anything else.

Cheers,
Jacob