Re: [PATCH v5 3/5] counter: Add character device interface

David Lechner
2020-10-14T22:42:25Z

On 9/26/20 9:18 PM, William Breathitt Gray wrote:
> +static ssize_t counter_chrdev_read(struct file *filp, char __user *buf,
> + size_t len, loff_t *f_ps)
> +{
> + struct counter_device *const counter = filp->private_data;
> + int err;
> + unsigned long flags;
> + unsigned int copied;
> +
> + if (len < sizeof(struct counter_event))
> + return -EINVAL;
> +
> + do {
> + if (kfifo_is_empty(&counter->events)) {
> + if (filp->f_flags & O_NONBLOCK)
> + return -EAGAIN;
> +
> + err = wait_event_interruptible(counter->events_wait,
> + !kfifo_is_empty(&counter->events));
> + if (err)
> + return err;
> + }
> +
> + raw_spin_lock_irqsave(&counter->events_lock, flags);
> + err = kfifo_to_user(&counter->events, buf, len, &copied);
> + raw_spin_unlock_irqrestore(&counter->events_lock, flags);
> + if (err)
> + return err;
> + } while (!copied);
> +
> + return copied;
> +}

All other uses of kfifo_to_user() I saw use a mutex instead of spin
lock. I don't see a reason for disabling interrupts here.

Example:

static ssize_t iio_event_chrdev_read(struct file *filep,
char __user *buf,
size_t count,
loff_t *f_ps)
{
struct iio_dev *indio_dev = filep->private_data;
struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
unsigned int copied;
int ret;

if (!indio_dev->info)
return -ENODEV;

if (count < sizeof(struct iio_event_data))
return -EINVAL;

do {
if (kfifo_is_empty(&ev_int->det_events)) {
if (filep->f_flags & O_NONBLOCK)
return -EAGAIN;

ret = wait_event_interruptible(ev_int->wait,
!kfifo_is_empty(&ev_int->det_events) ||
indio_dev->info == NULL);
if (ret)
return ret;
if (indio_dev->info == NULL)
return -ENODEV;
}

if (mutex_lock_interruptible(&ev_int->read_lock))
return -ERESTARTSYS;
ret = kfifo_to_user(&ev_int->det_events, buf, count, &copied);
mutex_unlock(&ev_int->read_lock);

if (ret)
return ret;

/*
* If we couldn't read anything from the fifo (a different
* thread might have been faster) we either return -EAGAIN if
* the file descriptor is non-blocking, otherwise we go back to
* sleep and wait for more data to arrive.
*/
if (copied == 0 && (filep->f_flags & O_NONBLOCK))
return -EAGAIN;

} while (copied == 0);

return copied;
}