| Message ID | 20210824164801.28896-3-lakshmi.sowjanya.d@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Review Request: Add support for Intel PMC | expand |
On Tue, Aug 24, 2021 at 6:48 PM <lakshmi.sowjanya.d@intel.com> wrote: > > From: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com> > > Some Intel Timed I/O devices do not implement IRQ functionality. Augment > read() interface to allow polling. > > Add two GPIO device methods: setup_poll() and poll(): > - setup_poll() configures the GPIO interface e.g. capture rising edges > - poll() checks for events on the interface > > To implement polling, the driver must implement the two functions above > and should either leave to_irq() method NULL or return irq 0. > > setup_poll() should configure the hardware to 'listen' for input events. > > poll() driver implementation must return the realtime timestamp > corresponding to the event and -EAGAIN if no data is available. > > Co-developed-by: Christopher Hall <christopher.s.hall@intel.com> > Signed-off-by: Christopher Hall <christopher.s.hall@intel.com> > Signed-off-by: Tamal Saha <tamal.saha@intel.com> > Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com> > Reviewed-by: Mark Gross <mgross@linux.intel.com> > --- Interesting. So the idea is to allow user-space to read line events as if they were generated by interrupts handled in the kernel. While this whole series has a long way to go and this patch looks wrong to me in several places at first glance, I find the idea interesting. Cc'ing Kent who's the author of most of this code - Kent: what do you think? Bart > drivers/gpio/gpiolib-cdev.c | 28 ++++++++++++++++++++++++++-- > include/linux/gpio/driver.h | 19 +++++++++++++++++++ > 2 files changed, 45 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > index c7b5446d01fd..4741bf34750b 100644 > --- a/drivers/gpio/gpiolib-cdev.c > +++ b/drivers/gpio/gpiolib-cdev.c > @@ -1227,13 +1227,34 @@ static ssize_t linereq_read(struct file *file, > loff_t *f_ps) Why would you do this in linereq_read()? Userspace ends up in linereq_poll() when it calls poll(). > { > struct linereq *lr = file->private_data; > + struct gpioevent_poll_data poll_data; > struct gpio_v2_line_event le; > ssize_t bytes_read = 0; > - int ret; > + int ret, offset; > > if (count < sizeof(le)) > return -EINVAL; > > + /* Without an IRQ, we can only poll */ > + offset = gpio_chip_hwgpio(lr->gdev->descs); > + if (lr->lines[offset].irq == 0) { > + struct gpio_v2_line_event *event; > + > + if (!(file->f_flags & O_NONBLOCK)) > + return -ENODEV; > + > + ret = lr->gdev->chip->do_poll(lr->gdev->chip, offset, > + lr->lines[offset].eflags, &poll_data); What if the driver doesn't implement do_poll()? > + if (ret) > + return ret; > + event = kzalloc(sizeof(*event), GFP_KERNEL); > + event->timestamp_ns = poll_data.timestamp; > + event->id = poll_data.id; > + if (copy_to_user(buf, (void *)&event, sizeof(event))) > + return -EFAULT; > + return sizeof(event); > + } > + > do { > spin_lock(&lr->wait.lock); > if (kfifo_is_empty(&lr->events)) { > @@ -1314,6 +1335,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip) > { > struct gpio_v2_line_request ulr; > struct gpio_v2_line_config *lc; > + unsigned int file_flags; > struct linereq *lr; > struct file *file; > u64 flags; > @@ -1411,6 +1433,8 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip) > goto out_free_linereq; > } > > + file_flags = O_RDONLY | O_CLOEXEC; > + > blocking_notifier_call_chain(&desc->gdev->notifier, > GPIO_V2_LINE_CHANGED_REQUESTED, desc); > > @@ -1425,7 +1449,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip) > } > > file = anon_inode_getfile("gpio-line", &line_fileops, lr, > - O_RDONLY | O_CLOEXEC); > + file_flags); > if (IS_ERR(file)) { > ret = PTR_ERR(file); > goto out_put_unused_fd; > diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h > index 3a268781fcec..f5b971ad40bc 100644 > --- a/include/linux/gpio/driver.h > +++ b/include/linux/gpio/driver.h > @@ -17,6 +17,7 @@ struct device_node; > struct seq_file; > struct gpio_device; > struct module; > +struct gpioevent_poll_data; > enum gpiod_flags; > enum gpio_lookup_flags; > > @@ -304,6 +305,11 @@ struct gpio_irq_chip { > * @add_pin_ranges: optional routine to initialize pin ranges, to be used when > * requires special mapping of the pins that provides GPIO functionality. > * It is called after adding GPIO chip and before adding IRQ chip. > + * @setup_poll: optional routine for devices that don't support interrupts. > + * Takes flags argument as in/out parameter, where caller requests > + * event flags and driver returns accepted flags. > + * @do_poll: optional routine for devices that don't support interrupts. > + * Returns event specification in data parameter. > * @base: identifies the first GPIO number handled by this chip; > * or, if negative during registration, requests dynamic ID allocation. > * DEPRECATION: providing anything non-negative and nailing the base > @@ -396,6 +402,14 @@ struct gpio_chip { > > int (*add_pin_ranges)(struct gpio_chip *gc); > > + int (*setup_poll)(struct gpio_chip *chip, > + unsigned int offset, > + u32 *eflags); Does anyone even call this? > + > + int (*do_poll)(struct gpio_chip *chip, > + unsigned int offset, u32 eflags, > + struct gpioevent_poll_data *data); > + > int base; > u16 ngpio; > const char *const *names; > @@ -471,6 +485,11 @@ struct gpio_chip { > #endif /* CONFIG_OF_GPIO */ > }; > > +struct gpioevent_poll_data { > + __u64 timestamp; > + __u32 id; > +}; > + > extern const char *gpiochip_is_requested(struct gpio_chip *gc, > unsigned int offset); > > -- > 2.17.1 > This patch doesn't look good - or even tested - but as I said - the idea itself sounds reasonable in general. Bart
On Wed, Sep 22, 2021 at 12:03:53PM +0200, Bartosz Golaszewski wrote: > On Tue, Aug 24, 2021 at 6:48 PM <lakshmi.sowjanya.d@intel.com> wrote: > > > > From: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com> > > > > Some Intel Timed I/O devices do not implement IRQ functionality. Augment > > read() interface to allow polling. > > > > Add two GPIO device methods: setup_poll() and poll(): > > - setup_poll() configures the GPIO interface e.g. capture rising edges > > - poll() checks for events on the interface > > > > To implement polling, the driver must implement the two functions above > > and should either leave to_irq() method NULL or return irq 0. > > > > setup_poll() should configure the hardware to 'listen' for input events. > > > > poll() driver implementation must return the realtime timestamp > > corresponding to the event and -EAGAIN if no data is available. > > > > Co-developed-by: Christopher Hall <christopher.s.hall@intel.com> > > Signed-off-by: Christopher Hall <christopher.s.hall@intel.com> > > Signed-off-by: Tamal Saha <tamal.saha@intel.com> > > Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com> > > Reviewed-by: Mark Gross <mgross@linux.intel.com> > > --- > > Interesting. So the idea is to allow user-space to read line events as > if they were generated by interrupts handled in the kernel. While this > whole series has a long way to go and this patch looks wrong to me in > several places at first glance, I find the idea interesting. Cc'ing > Kent who's the author of most of this code - Kent: what do you think? > It is interesting that we're seeing more hardware that provides more detailed edge info than we get from irq. The hte patch series can also provide hardware timestamps, but the Timed I/O could even provide the sequence numbers. It might be worth abstracting the edge detection so edge events could be more easily driven by subsystems other than irq, without festooning cdev with special cases. I'm not a fan of the polling here though, particularly from userspace. If polling can't be avoided (why did they not provide an irq??) then I would do the polling in kernel and place any resulting events in the cdev kfifo for userspace to read as per the existing events. Of course that is without knowing a whole lot about the hardware or use cases. The Intel datasheet doesn't provide much in the way of data :|. Cheers, Kent.
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index c7b5446d01fd..4741bf34750b 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -1227,13 +1227,34 @@ static ssize_t linereq_read(struct file *file, loff_t *f_ps) { struct linereq *lr = file->private_data; + struct gpioevent_poll_data poll_data; struct gpio_v2_line_event le; ssize_t bytes_read = 0; - int ret; + int ret, offset; if (count < sizeof(le)) return -EINVAL; + /* Without an IRQ, we can only poll */ + offset = gpio_chip_hwgpio(lr->gdev->descs); + if (lr->lines[offset].irq == 0) { + struct gpio_v2_line_event *event; + + if (!(file->f_flags & O_NONBLOCK)) + return -ENODEV; + + ret = lr->gdev->chip->do_poll(lr->gdev->chip, offset, + lr->lines[offset].eflags, &poll_data); + if (ret) + return ret; + event = kzalloc(sizeof(*event), GFP_KERNEL); + event->timestamp_ns = poll_data.timestamp; + event->id = poll_data.id; + if (copy_to_user(buf, (void *)&event, sizeof(event))) + return -EFAULT; + return sizeof(event); + } + do { spin_lock(&lr->wait.lock); if (kfifo_is_empty(&lr->events)) { @@ -1314,6 +1335,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip) { struct gpio_v2_line_request ulr; struct gpio_v2_line_config *lc; + unsigned int file_flags; struct linereq *lr; struct file *file; u64 flags; @@ -1411,6 +1433,8 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip) goto out_free_linereq; } + file_flags = O_RDONLY | O_CLOEXEC; + blocking_notifier_call_chain(&desc->gdev->notifier, GPIO_V2_LINE_CHANGED_REQUESTED, desc); @@ -1425,7 +1449,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip) } file = anon_inode_getfile("gpio-line", &line_fileops, lr, - O_RDONLY | O_CLOEXEC); + file_flags); if (IS_ERR(file)) { ret = PTR_ERR(file); goto out_put_unused_fd; diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index 3a268781fcec..f5b971ad40bc 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -17,6 +17,7 @@ struct device_node; struct seq_file; struct gpio_device; struct module; +struct gpioevent_poll_data; enum gpiod_flags; enum gpio_lookup_flags; @@ -304,6 +305,11 @@ struct gpio_irq_chip { * @add_pin_ranges: optional routine to initialize pin ranges, to be used when * requires special mapping of the pins that provides GPIO functionality. * It is called after adding GPIO chip and before adding IRQ chip. + * @setup_poll: optional routine for devices that don't support interrupts. + * Takes flags argument as in/out parameter, where caller requests + * event flags and driver returns accepted flags. + * @do_poll: optional routine for devices that don't support interrupts. + * Returns event specification in data parameter. * @base: identifies the first GPIO number handled by this chip; * or, if negative during registration, requests dynamic ID allocation. * DEPRECATION: providing anything non-negative and nailing the base @@ -396,6 +402,14 @@ struct gpio_chip { int (*add_pin_ranges)(struct gpio_chip *gc); + int (*setup_poll)(struct gpio_chip *chip, + unsigned int offset, + u32 *eflags); + + int (*do_poll)(struct gpio_chip *chip, + unsigned int offset, u32 eflags, + struct gpioevent_poll_data *data); + int base; u16 ngpio; const char *const *names; @@ -471,6 +485,11 @@ struct gpio_chip { #endif /* CONFIG_OF_GPIO */ }; +struct gpioevent_poll_data { + __u64 timestamp; + __u32 id; +}; + extern const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset);