From: Arnd Bergmann <arnd@kernel.org>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: "Linus Walleij" <linus.walleij@linaro.org>,
"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>,
"Jean-Philippe Brucker" <jean-philippe@linaro.org>,
"Bill Mills" <bill.mills@linaro.org>,
"Alex Bennée" <alex.bennee@linaro.org>,
"Cornelia Huck" <cohuck@redhat.com>,
"Geert Uytterhoeven" <geert@linux-m68k.org>,
"Stratos Mailing List" <stratos-dev@op-lists.linaro.org>,
"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
"Thomas Gleixner" <tglx@linutronix.de>,
"Marc Zyngier" <maz@kernel.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
"open list:DRM DRIVER FOR QEMU'S CIRRUS DEVICE"
<virtualization@lists.linux-foundation.org>
Subject: Re: [PATCH V4 2/2] gpio: virtio: Add IRQ support
Date: Tue, 3 Aug 2021 17:01:59 +0200 [thread overview]
Message-ID: <CAK8P3a29NfFWwtGHhqos1P8f_SmzPJTXvEY5BZJAEMbV2SKe-Q@mail.gmail.com> (raw)
In-Reply-To: <75c8e6e5e8dfa1889938f3a6b2d991763c7a3717.1627989586.git.viresh.kumar@linaro.org>
On Tue, Aug 3, 2021 at 1:36 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 those operations over
> virtio may sleep. Also the notifications for the eventq are processed
> using a work item to allow sleep-able operations.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> drivers/gpio/Kconfig | 1 +
> drivers/gpio/gpio-virtio.c | 281 ++++++++++++++++++++++++++++++-
> include/uapi/linux/virtio_gpio.h | 25 +++
> 3 files changed, 303 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index e5993d6864fb..222f4ae98a35 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1672,6 +1672,7 @@ config GPIO_MOCKUP
> config GPIO_VIRTIO
> tristate "VirtIO GPIO support"
> depends on VIRTIO
> + select GPIOLIB_IRQCHIP
> help
> Say Y here to enable guest support for virtio-based GPIO controllers.
>
> +struct vgpio_irq_line {
> + u8 type;
> + bool masked;
> + bool update_pending;
> + bool queued;
> +
> + struct virtio_gpio_irq_request ireq;
> + struct virtio_gpio_irq_response ires;
> +};
I think the last two members should be marked as __cacheline_aligned,
since they are transferred separately over DMA.
> +static void virtio_gpio_irq_eoi(struct irq_data *d)
> +{
> + /*
> + * Queue buffers, by calling virtio_gpio_irq_prepare(), from
> + * virtio_gpio_event_vq() itself, after taking into consideration the
> + * masking status of the interrupt.
> + */
> +}
Shouldn't this just requeue the interrupt? There is no reason to
defer this, and I think we want the normal operation to not have
to involve any scheduling.
> +static void virtio_gpio_irq_unmask(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct virtio_gpio *vgpio = gpiochip_get_data(gc);
> + struct vgpio_irq_line *irq_line = &vgpio->irq_lines[d->hwirq];
> +
> + irq_line->masked = false;
> + irq_line->update_pending = true;
> +}
Same here. unmask is probably less important, but it's the
same operation: if you want interrupts to become active
again when they are not, just queue the request
> +static void virtio_gpio_irq_mask(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct virtio_gpio *vgpio = gpiochip_get_data(gc);
> + struct vgpio_irq_line *irq_line = &vgpio->irq_lines[d->hwirq];
> +
> + irq_line->masked = true;
> + irq_line->update_pending = true;
> +}
This is of course the tricky bit, I was hoping you had found a solution
based on what I wrote above for eio() and unmask().
> +static void vgpio_work_handler(struct work_struct *work)
> +{
> + struct virtio_gpio *vgpio = container_of(work, struct virtio_gpio,
> + work);
> + struct device *dev = &vgpio->vdev->dev;
> + struct vgpio_irq_line *irq_line;
> + int irq, gpio, ret;
> + unsigned int len;
> +
> + mutex_lock(&vgpio->irq_lock);
> +
> + while (true) {
> + irq_line = virtqueue_get_buf(vgpio->event_vq, &len);
> + if (!irq_line)
> + break;
Related to above, I think all the eventq handling should be moved into the
virtio_gpio_event_vq() function directly.
> + /* The interrupt may have been disabled by now */
> + if (irq_line->update_pending && irq_line->masked)
> + update_irq_type(vgpio, gpio, VIRTIO_GPIO_IRQ_TYPE_NONE);
This is a part I'm not sure about, and I suppose it's the same part that
Marc was also confused by.
As far as I can tell, the update_irq_type() message would lead to the
interrupt getting delivered when it was armed and is now getting disabled,
but I don't see why we would call update_irq_type() as a result of the
eventq notification.
Arnd
next prev parent reply other threads:[~2021-08-03 15:02 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-03 11:36 [PATCH V4 0/2] gpio: Add virtio based driver Viresh Kumar
2021-08-03 11:36 ` [PATCH V4 1/2] gpio: Add virtio-gpio driver Viresh Kumar
2021-08-03 11:36 ` [PATCH V4 2/2] gpio: virtio: Add IRQ support Viresh Kumar
2021-08-03 15:01 ` Arnd Bergmann [this message]
2021-08-04 7:05 ` Viresh Kumar
2021-08-04 8:27 ` Arnd Bergmann
2021-08-05 7:05 ` Viresh Kumar
2021-08-05 11:26 ` Viresh Kumar
[not found] ` <0100017b1610f711-c53c79f2-9e28-4c45-bb42-8db09688b18e-000000@email.amazonses.com>
2021-08-05 12:03 ` [Stratos-dev] " Arnd Bergmann
2021-08-05 12:49 ` Viresh Kumar
2021-08-05 13:10 ` Arnd Bergmann
2021-08-06 7:44 ` Viresh Kumar
[not found] ` <0100017b1a6c0a05-e41dc16c-b326-4017-a63d-a24a6c1fde70-000000@email.amazonses.com>
2021-08-06 8:00 ` Arnd Bergmann
2021-08-09 7:30 ` Viresh Kumar
2021-08-09 7:55 ` Arnd Bergmann
2021-08-09 10:46 ` Viresh Kumar
[not found] ` <0100017b2a85eaf8-08b905fc-89f7-43a4-857e-070ca9691ce1-000000@email.amazonses.com>
2021-08-09 11:19 ` Arnd Bergmann
2021-08-10 7:35 ` 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=CAK8P3a29NfFWwtGHhqos1P8f_SmzPJTXvEY5BZJAEMbV2SKe-Q@mail.gmail.com \
--to=arnd@kernel.org \
--cc=alex.bennee@linaro.org \
--cc=bgolaszewski@baylibre.com \
--cc=bill.mills@linaro.org \
--cc=cohuck@redhat.com \
--cc=geert@linux-m68k.org \
--cc=info@metux.net \
--cc=jasowang@redhat.com \
--cc=jean-philippe@linaro.org \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maz@kernel.org \
--cc=mst@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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).