Re: [PATCH v9 09/20] gpiolib: cdev: support edge detection for uAPI v2

[Kent Gibson]

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@xxxxxxxxx> 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.