From: Linus Walleij <linus.walleij@linaro.org> To: Viresh Kumar <viresh.kumar@linaro.org>, Marc Zyngier <maz@kernel.org>, Thomas Gleixner <tglx@linutronix.de> Cc: "Bartosz Golaszewski" <bgolaszewski@baylibre.com>, "Enrico Weigelt, metux IT consult" <info@metux.net>, "Viresh Kumar" <vireshk@kernel.org>, "Michael S. Tsirkin" <mst@redhat.com>, "Jason Wang" <jasowang@redhat.com>, "Vincent Guittot" <vincent.guittot@linaro.org>, "Bill Mills" <bill.mills@linaro.org>, "Alex Bennée" <alex.bennee@linaro.org>, stratos-dev@op-lists.linaro.org, "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>, "Stefan Hajnoczi" <stefanha@redhat.com>, "Stefano Garzarella --cc virtualization @ lists . linux-foundation . org" <sgarzare@redhat.com>, virtualization@lists.linux-foundation.org Subject: Re: [PATCH V3 2/3] gpio: virtio: Add IRQ support Date: Thu, 10 Jun 2021 23:30:07 +0200 [thread overview] Message-ID: <CACRpkdYHMtG_X3FgiArbQW49kTwJwOGn90peDvAV5Bs5oDiC7A@mail.gmail.com> (raw) In-Reply-To: <911941d4bf19f18abdc9700abca9f26b3c04c343.1623326176.git.viresh.kumar@linaro.org> Hi Viresh! thanks for this interesting patch! On Thu, Jun 10, 2021 at 2:16 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > This patch adds IRQ support for the virtio GPIO driver. Note that this > uses the irq_bus_lock/unlock() callbacks since the operations over > virtio can sleep. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > drivers/gpio/gpio-virtio.c | 256 ++++++++++++++++++++++++++++++- > include/uapi/linux/virtio_gpio.h | 15 ++ You also need to select GPIOLIB_IRQCHIP in the Kconfig entry for the gpio-virtio driver, because you make use of it. > +static void virtio_gpio_irq_mask(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_to_gpio_chip(d); > + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); > + struct vgpio_line *line = &vgpio->lines[d->hwirq]; > + > + line->masked = true; > + line->masked_pending = true; > +} > + > +static void virtio_gpio_irq_unmask(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_to_gpio_chip(d); > + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); > + struct vgpio_line *line = &vgpio->lines[d->hwirq]; > + > + line->masked = false; > + line->masked_pending = true; > +} This looks dangerous in combination with this: > +static void virtio_gpio_interrupt(struct virtqueue *vq) > +{ (...) > + local_irq_disable(); > + ret = generic_handle_irq(irq); > + local_irq_enable(); Nominally slow IRQs like those being marshalled over virtio should be nested, handle_nested_irq(irq); but are they? Or are they just quite slow not super slow? If a threaded IRQF_ONESHOT was requested the IRQ core will kick the thread and *MASK* this IRQ, which means it will call back to your .irq_mask() function and expect it to be masked from this point. But the IRQ will not actually be masked until you issue your callbacks in the .irq_bus_sync_unlock() callback right? So from this point until .irq_bus_sync_unlock() get called and actually mask the IRQ, it could be fired again? I suppose the IRQ handler is reentrant? This would violate the API. I would say that from this point and until you sync you need a spinlock or other locking primitive to stop this IRQ from fireing again, and a spinlock will imply local_irq_disable() so this gets really complex. I would say only using nesting IRQs or guarantee this some other way, one way would be to specify that whatever is at the other side of virtio cannot send another GPIO IRQ message before the last one is handled, so you would need to send a specific (new) VIRTIO_GPIO_REQ_IRQ_ACK after all other messages have been sent in .irq_bus_sync_unlock() so that the next GPIO IRQ can be dispatched after that. (Is this how messaged signalled interrupts work? No idea. When in doubt ask the IRQ maintainers.) Thanks, Linus Walleij
WARNING: multiple messages have this Message-ID (diff)
From: Linus Walleij <linus.walleij@linaro.org> To: Viresh Kumar <viresh.kumar@linaro.org>, Marc Zyngier <maz@kernel.org>, Thomas Gleixner <tglx@linutronix.de> Cc: Vincent Guittot <vincent.guittot@linaro.org>, Stefan Hajnoczi <stefanha@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Viresh Kumar <vireshk@kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>, virtualization@lists.linux-foundation.org, Bartosz Golaszewski <bgolaszewski@baylibre.com>, "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>, "Enrico Weigelt, metux IT consult" <info@metux.net>, Bill Mills <bill.mills@linaro.org>, stratos-dev@op-lists.linaro.org Subject: Re: [PATCH V3 2/3] gpio: virtio: Add IRQ support Date: Thu, 10 Jun 2021 23:30:07 +0200 [thread overview] Message-ID: <CACRpkdYHMtG_X3FgiArbQW49kTwJwOGn90peDvAV5Bs5oDiC7A@mail.gmail.com> (raw) In-Reply-To: <911941d4bf19f18abdc9700abca9f26b3c04c343.1623326176.git.viresh.kumar@linaro.org> Hi Viresh! thanks for this interesting patch! On Thu, Jun 10, 2021 at 2:16 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > This patch adds IRQ support for the virtio GPIO driver. Note that this > uses the irq_bus_lock/unlock() callbacks since the operations over > virtio can sleep. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > drivers/gpio/gpio-virtio.c | 256 ++++++++++++++++++++++++++++++- > include/uapi/linux/virtio_gpio.h | 15 ++ You also need to select GPIOLIB_IRQCHIP in the Kconfig entry for the gpio-virtio driver, because you make use of it. > +static void virtio_gpio_irq_mask(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_to_gpio_chip(d); > + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); > + struct vgpio_line *line = &vgpio->lines[d->hwirq]; > + > + line->masked = true; > + line->masked_pending = true; > +} > + > +static void virtio_gpio_irq_unmask(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_to_gpio_chip(d); > + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); > + struct vgpio_line *line = &vgpio->lines[d->hwirq]; > + > + line->masked = false; > + line->masked_pending = true; > +} This looks dangerous in combination with this: > +static void virtio_gpio_interrupt(struct virtqueue *vq) > +{ (...) > + local_irq_disable(); > + ret = generic_handle_irq(irq); > + local_irq_enable(); Nominally slow IRQs like those being marshalled over virtio should be nested, handle_nested_irq(irq); but are they? Or are they just quite slow not super slow? If a threaded IRQF_ONESHOT was requested the IRQ core will kick the thread and *MASK* this IRQ, which means it will call back to your .irq_mask() function and expect it to be masked from this point. But the IRQ will not actually be masked until you issue your callbacks in the .irq_bus_sync_unlock() callback right? So from this point until .irq_bus_sync_unlock() get called and actually mask the IRQ, it could be fired again? I suppose the IRQ handler is reentrant? This would violate the API. I would say that from this point and until you sync you need a spinlock or other locking primitive to stop this IRQ from fireing again, and a spinlock will imply local_irq_disable() so this gets really complex. I would say only using nesting IRQs or guarantee this some other way, one way would be to specify that whatever is at the other side of virtio cannot send another GPIO IRQ message before the last one is handled, so you would need to send a specific (new) VIRTIO_GPIO_REQ_IRQ_ACK after all other messages have been sent in .irq_bus_sync_unlock() so that the next GPIO IRQ can be dispatched after that. (Is this how messaged signalled interrupts work? No idea. When in doubt ask the IRQ maintainers.) Thanks, Linus Walleij _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2021-06-10 21:30 UTC|newest] Thread overview: 73+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-10 12:09 [PATCH V3 0/3] gpio: Add virtio based driver Viresh Kumar 2021-06-10 12:09 ` Viresh Kumar 2021-06-10 12:16 ` [PATCH V3 1/3] gpio: Add virtio-gpio driver Viresh Kumar 2021-06-10 12:16 ` Viresh Kumar 2021-06-10 13:22 ` Arnd Bergmann 2021-06-10 16:00 ` Enrico Weigelt, metux IT consult [not found] ` <01000179f6a7715c-cd106846-7770-4088-bb7c-a696bfcbf83e-000000@email.amazonses.com> 2021-06-10 17:03 ` [Stratos-dev] " Jean-Philippe Brucker 2021-06-10 17:03 ` Jean-Philippe Brucker 2021-06-10 19:41 ` Arnd Bergmann 2021-06-14 10:21 ` Viresh Kumar 2021-06-14 10:21 ` Viresh Kumar 2021-06-14 12:31 ` Arnd Bergmann 2021-06-14 12:49 ` Vincent Guittot [not found] ` <0100017a0a9264cc-57668c56-fdbf-412a-9f82-9bf95f5c653e-000000@email.amazonses.com> 2021-06-14 12:58 ` [Stratos-dev] " Arnd Bergmann 2021-06-14 13:24 ` Vincent Guittot 2021-06-14 20:54 ` Arnd Bergmann 2021-06-15 7:30 ` Vincent Guittot 2021-06-10 15:54 ` Enrico Weigelt, metux IT consult 2021-06-10 16:57 ` Viresh Kumar 2021-06-10 16:57 ` Viresh Kumar 2021-06-10 20:46 ` Linus Walleij 2021-06-10 20:46 ` Linus Walleij 2021-06-11 3:56 ` Viresh Kumar 2021-06-11 3:56 ` Viresh Kumar 2021-06-11 7:42 ` Geert Uytterhoeven 2021-06-11 7:42 ` Geert Uytterhoeven 2021-06-11 8:01 ` Viresh Kumar 2021-06-11 8:01 ` Viresh Kumar 2021-06-11 8:22 ` Geert Uytterhoeven 2021-06-11 8:22 ` Geert Uytterhoeven 2021-06-15 11:15 ` Viresh Kumar 2021-06-15 11:15 ` Viresh Kumar 2021-06-15 11:37 ` Geert Uytterhoeven 2021-06-15 11:37 ` Geert Uytterhoeven 2021-06-15 20:03 ` Linus Walleij 2021-06-15 20:03 ` Linus Walleij 2021-06-16 1:45 ` Viresh Kumar 2021-06-16 1:45 ` Viresh Kumar 2021-06-14 8:07 ` Enrico Weigelt, metux IT consult 2021-06-14 8:12 ` Andy Shevchenko 2021-06-14 8:12 ` Andy Shevchenko 2021-06-14 9:14 ` Viresh Kumar 2021-06-14 9:14 ` Viresh Kumar 2021-06-14 9:17 ` Enrico Weigelt, metux IT consult 2021-06-14 9:52 ` Viresh Kumar 2021-06-14 9:52 ` Viresh Kumar 2021-06-14 9:12 ` Viresh Kumar 2021-06-14 9:12 ` Viresh Kumar 2021-06-14 9:29 ` Enrico Weigelt, metux IT consult 2021-06-14 8:03 ` Enrico Weigelt, metux IT consult 2021-06-14 9:24 ` Viresh Kumar 2021-06-14 9:24 ` Viresh Kumar 2021-06-16 3:30 ` Bjorn Andersson 2021-06-16 3:30 ` Bjorn Andersson 2021-06-16 15:52 ` Enrico Weigelt, metux IT consult 2021-06-18 9:13 ` Linus Walleij 2021-06-18 9:13 ` Linus Walleij 2021-06-21 17:25 ` Bjorn Andersson 2021-06-21 17:25 ` Bjorn Andersson 2021-06-10 12:16 ` [PATCH V3 2/3] gpio: virtio: Add IRQ support Viresh Kumar 2021-06-10 12:16 ` Viresh Kumar 2021-06-10 21:30 ` Linus Walleij [this message] 2021-06-10 21:30 ` Linus Walleij 2021-06-14 7:08 ` Viresh Kumar 2021-06-14 7:08 ` Viresh Kumar 2021-06-10 12:16 ` [PATCH V3 3/3] MAINTAINERS: Add entry for Virtio-gpio Viresh Kumar [not found] ` <01000179f5da7763-2ea817c6-e176-423a-952e-de02443f71e2-000000@email.amazonses.com> 2021-06-10 17:40 ` [PATCH V3 1/3] gpio: Add virtio-gpio driver Jean-Philippe Brucker 2021-06-10 17:40 ` Jean-Philippe Brucker 2021-06-11 3:39 ` Viresh Kumar 2021-06-11 3:39 ` Viresh Kumar [not found] ` <01000179f9276678-ae2bb25f-4c0c-4176-b906-650c585b9753-000000@email.amazonses.com> 2021-06-11 8:34 ` [Stratos-dev] " Arnd Bergmann 2021-06-14 5:26 ` Viresh Kumar 2021-06-14 5:26 ` Viresh Kumar
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CACRpkdYHMtG_X3FgiArbQW49kTwJwOGn90peDvAV5Bs5oDiC7A@mail.gmail.com \ --to=linus.walleij@linaro.org \ --cc=alex.bennee@linaro.org \ --cc=bgolaszewski@baylibre.com \ --cc=bill.mills@linaro.org \ --cc=info@metux.net \ --cc=jasowang@redhat.com \ --cc=linux-gpio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=maz@kernel.org \ --cc=mst@redhat.com \ --cc=sgarzare@redhat.com \ --cc=stefanha@redhat.com \ --cc=stratos-dev@op-lists.linaro.org \ --cc=tglx@linutronix.de \ --cc=vincent.guittot@linaro.org \ --cc=viresh.kumar@linaro.org \ --cc=vireshk@kernel.org \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.