lkml.org 
[lkml]   [2020]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v5 3/5] counter: Add character device interface
    On Sun, Oct 25, 2020 at 11:34:43AM -0500, David Lechner wrote:
    > On 10/25/20 8:18 AM, William Breathitt Gray wrote:
    > > On Tue, Oct 20, 2020 at 11:06:42AM -0500, David Lechner wrote:
    > >> On 10/18/20 11:58 AM, William Breathitt Gray wrote:
    > >>> On Wed, Oct 14, 2020 at 05:40:44PM -0500, David Lechner wrote:
    > >>>> 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.
    > >>>
    > >>> The Counter character device interface is special in this case because
    > >>> counter->events could be accessed from an interrupt context. This is
    > >>> possible if counter_push_event() is called for an interrupt (as is the
    > >>> case for the 104_quad_8 driver). In this case, we can't use mutex
    > >>> because we can't sleep in an interrupt context, so our only option is to
    > >>> use spin lock.
    > >>>
    > >>
    > >>
    > >> The way I understand it, locking is only needed for concurrent readers
    > >> and locking between reader and writer is not needed.
    > >
    > > You're right, it does say in the kfifo.h comments that with only one
    > > concurrent reader and one current write, we don't need extra locking to
    > > use these macros. Because we only have one kfifo_to_user() operating on
    > > counter->events, does that mean we don't need locking at all here for
    > > the counter_chrdev_read() function?
    > >
    > > William Breathitt Gray
    > >
    >
    > Even if we have the policy that only one file handle to the chrdev
    > can be open at a time, it is still possible that the it could be
    > read from multiple threads. So it I think it makes sense to keep
    > it just to be safe.

    All right, I'll keep the locks in the code for now to keep it safe in
    case we have multiple threads reading.

    William Breathitt Gray
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2020-10-25 18:55    [W:4.511 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site