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;"?
> +
> + 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.
> +
> + 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?
> +
> + if (count != 4) {
Remove the magic number. Just say sizeof(u32).
> + 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;
> + 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.
> + 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.
> +}
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.
> +// reads a single packet from the fifo as dictated by the tlast signal
Please use /* */ style comments.
> +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;"
> + int wait_ret;
Just say "int ret;"
> + 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...
> + 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?
> + } 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?
> + }
> +
> + 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?
> + 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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
