lkml.org 
[lkml]   [2020]   [Sep]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v9 09/20] gpiolib: cdev: support edge detection for uAPI v2
On Wed, Sep 23, 2020 at 06:47:28PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > Add support for edge detection to lines requested using
> > GPIO_V2_GET_LINE_IOCTL.
> >

[snip]
>
>
> > + if (!overflow)
> > + wake_up_poll(&lr->wait, EPOLLIN);
> > + else
> > + pr_debug_ratelimited("event FIFO is full - event dropped\n");
>
> Under positive conditionals I meant something like this
>
> if (overflow)
> pr_debug_ratelimited("event FIFO is full - event dropped\n");
> else
> wake_up_poll(&lr->wait, EPOLLIN);
>

Ahh, ok. I tend to stick with the more normal path being first, and the
overflow is definitely the abnormal path.

Also, this code is drawn from lineevent_irq_thread(), which is ordered
this way.

> > +}
> > +
> > +static irqreturn_t edge_irq_thread(int irq, void *p)
> > +{
> > + struct line *line = p;
> > + struct linereq *lr = line->req;
> > + struct gpio_v2_line_event le;
> > +
> > + /* Do not leak kernel stack to userspace */
> > + memset(&le, 0, sizeof(le));

> > + /*
> > + * We may be running from a nested threaded interrupt in which case
> > + * we didn't get the timestamp from edge_irq_handler().
> > + */
> > + if (!line->timestamp_ns) {
> > + le.timestamp_ns = ktime_get_ns();
> > + if (lr->num_lines != 1)
> > + line->req_seqno = atomic_inc_return(&lr->seqno);
> > + } else {
> > + le.timestamp_ns = line->timestamp_ns;
> > > + }
>
> Ditto.

Firstly, drawn from lineevent_irq_thread() which is structured this way.

In this case the comment relates to the condition being true, so
re-ordering the if/else would be confusing - unless the comment were
moved into the corresponding body??


[snip]
> > +static int edge_detector_setup(struct line *line,
> > + u64 eflags)
> > +{
> > + unsigned long irqflags = 0;
> > + int irq, ret;
> > +
> > + if (eflags && !kfifo_initialized(&line->req->events)) {
> > + ret = kfifo_alloc(&line->req->events,
> > + line->req->event_buffer_size, GFP_KERNEL);
> > + if (ret)
> > + return ret;
> > + }
> > + line->eflags = eflags;
> > +
> > + if (!eflags)
> > + return 0;
> > +
> > + irq = gpiod_to_irq(line->desc);
> > + if (irq <= 0)
> > + return -ENODEV;
>
> So, you mean this is part of ABI. Can we return more appropriate code,
> because getting no IRQ doesn't mean we don't have a device.
> Also does 0 case have the same meaning?

Firstly, this code is drawn from lineevent_create(), so any changes
here should be considered for there as well - though this may
constitute an ABI change??

I agree ENODEV doesn't seem right here. Are you ok with ENXIO?

From gpiod_to_irq():

/* Zero means NO_IRQ */
if (!retirq)
return -ENXIO;

so it can't even return a 0 :-| - we're just being cautious.

Cheers,
Kent.

\
 
 \ /
  Last update: 2020-09-24 05:09    [W:0.106 / U:0.312 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site