Messages in this thread | | | Subject | Re: [PATCH v5 3/5] counter: Add character device interface | From | David Lechner <> | Date | Wed, 14 Oct 2020 17:40:44 -0500 |
| |
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; }
| |