From: Viresh Kumar <viresh.kumar@linaro.org> To: Arnd Bergmann <arnd@kernel.org> Cc: "Michael S. Tsirkin" <mst@redhat.com>, Viresh Kumar <vireshk@kernel.org>, Linus Walleij <linus.walleij@linaro.org>, Cornelia Huck <cohuck@redhat.com>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, "open list:DRM DRIVER FOR QEMU'S CIRRUS DEVICE" <virtualization@lists.linux-foundation.org>, Bartosz Golaszewski <bgolaszewski@baylibre.com>, Geert Uytterhoeven <geert@linux-m68k.org>, "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>, Marc Zyngier <maz@kernel.org>, Thomas Gleixner <tglx@linutronix.de>, Stratos Mailing List <stratos-dev@op-lists.linaro.org>, "Enrico Weigelt, metux IT consult" <info@metux.net>, Jason Wang <jasowang@redhat.com> Subject: Re: [Stratos-dev] [PATCH V4 2/2] gpio: virtio: Add IRQ support Date: Thu, 5 Aug 2021 18:19:22 +0530 [thread overview] Message-ID: <20210805124922.j7lts7tfmm4t2kpf@vireshk-i7> (raw) In-Reply-To: <CAK8P3a0DWkfQcZpmyfKcdNt1MHf8ha6a9L2LmLt1Tv-j0HDr3w@mail.gmail.com> On 05-08-21, 14:03, Arnd Bergmann wrote: > On Thu, Aug 5, 2021 at 1:26 PM Viresh Kumar via Stratos-dev > > Based on discussion we had today (offline), I changed the design a bit > > and used handle_level_irq() instead, as it provides consistent calls > > to mask/unmask(), which simplified the whole thing a bit. > > The new flow looks much nicer to me, without the workqueue, and > doing the requeue directly in the unmask() operation. > > I don't quite understand the purpose of the type_pending and > mask_pending flags yet, can you explain what they actually > do? They are required to make sure we don't send unnecessary VIRTIO_GPIO_MSG_IRQ_TYPE events to the device, every time bus_unlock() is called. mask_pending tracks if the masked state has changed since the time last bus_unlock() was called. So on an interrupt, both mask() and unmask() will get called by the irq-core now and mask_pending will change to true (in mask()} and then false (in unmask()). And eventually in bus_unlock() we won't send an extra VIRTIO_GPIO_MSG_IRQ_TYPE message. > Also, I have no idea about whether using the handle_level_irq() > function is actually correct here. I suppose if necessary, the driver > could provide its own irq.handler callback in place of that. After looking at internals of these, I felt handle_level_irq() suits much better in our case as we need to queue the buffer only at unmask(). With handle_fasteoi_irq(), we would be required to do the same from multiple places, unmask(), eoi(). > > Also I have broken the rule from specs, maybe we should update spec > > with that, where the specs said that the buffer must not be queued > > before enabling the interrupt. I just queue the buffer unconditionally > > now from unmask(). > > > > I am not sure but there may be some race around the "queued" flag and > > I wonder if we can land in a scenario where the buffer is left > > un-queued somehow, while an interrupt is enabled. > > Can that be integrated with the "masked" state now? It looks like > the two flags are always opposites now. Yeah, but then there can be races and keeping them separate is a better thing IMO. I was thinking of something on these lines, disable_irq() followed by enable_irq(): CPU0 CPU1 disable_irq() -> irq_bus_lock() -> irq_mask() -> irq_bus_sync_unlock() -> sends blocking VIRTIO_GPIO_MSG_IRQ_TYPE to disable interrupt Backend (at host) disables irq and returns the unused buffer. enable_irq() -> irq_bus_lock() -> irq_unmask() -> Tries to queues buffer again Finds it already queued and returns. - virtio_gpio_event_vq() runs at guest - Finds VIRTIO_GPIO_IRQ_STATUS_INVALID in status - returns without doing anything -> irq_bus_sync_unlock() -> sends blocking VIRTIO_GPIO_MSG_IRQ_TYPE to enable interrupt So the irq is still enabled and the buffer isn't queued. Yes, need some locking here for sure, confirmed :) -- viresh
WARNING: multiple messages have this Message-ID (diff)
From: Viresh Kumar <viresh.kumar@linaro.org> To: Arnd Bergmann <arnd@kernel.org> Cc: "Michael S. Tsirkin" <mst@redhat.com>, Viresh Kumar <vireshk@kernel.org>, Linus Walleij <linus.walleij@linaro.org>, Cornelia Huck <cohuck@redhat.com>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, "open list:DRM DRIVER FOR QEMU'S CIRRUS DEVICE" <virtualization@lists.linux-foundation.org>, Bartosz Golaszewski <bgolaszewski@baylibre.com>, Geert Uytterhoeven <geert@linux-m68k.org>, "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>, Marc Zyngier <maz@kernel.org>, Thomas Gleixner <tglx@linutronix.de>, "Enrico Weigelt, metux IT consult" <info@metux.net>, Stratos Mailing List <stratos-dev@op-lists.linaro.org> Subject: Re: [Stratos-dev] [PATCH V4 2/2] gpio: virtio: Add IRQ support Date: Thu, 5 Aug 2021 18:19:22 +0530 [thread overview] Message-ID: <20210805124922.j7lts7tfmm4t2kpf@vireshk-i7> (raw) In-Reply-To: <CAK8P3a0DWkfQcZpmyfKcdNt1MHf8ha6a9L2LmLt1Tv-j0HDr3w@mail.gmail.com> On 05-08-21, 14:03, Arnd Bergmann wrote: > On Thu, Aug 5, 2021 at 1:26 PM Viresh Kumar via Stratos-dev > > Based on discussion we had today (offline), I changed the design a bit > > and used handle_level_irq() instead, as it provides consistent calls > > to mask/unmask(), which simplified the whole thing a bit. > > The new flow looks much nicer to me, without the workqueue, and > doing the requeue directly in the unmask() operation. > > I don't quite understand the purpose of the type_pending and > mask_pending flags yet, can you explain what they actually > do? They are required to make sure we don't send unnecessary VIRTIO_GPIO_MSG_IRQ_TYPE events to the device, every time bus_unlock() is called. mask_pending tracks if the masked state has changed since the time last bus_unlock() was called. So on an interrupt, both mask() and unmask() will get called by the irq-core now and mask_pending will change to true (in mask()} and then false (in unmask()). And eventually in bus_unlock() we won't send an extra VIRTIO_GPIO_MSG_IRQ_TYPE message. > Also, I have no idea about whether using the handle_level_irq() > function is actually correct here. I suppose if necessary, the driver > could provide its own irq.handler callback in place of that. After looking at internals of these, I felt handle_level_irq() suits much better in our case as we need to queue the buffer only at unmask(). With handle_fasteoi_irq(), we would be required to do the same from multiple places, unmask(), eoi(). > > Also I have broken the rule from specs, maybe we should update spec > > with that, where the specs said that the buffer must not be queued > > before enabling the interrupt. I just queue the buffer unconditionally > > now from unmask(). > > > > I am not sure but there may be some race around the "queued" flag and > > I wonder if we can land in a scenario where the buffer is left > > un-queued somehow, while an interrupt is enabled. > > Can that be integrated with the "masked" state now? It looks like > the two flags are always opposites now. Yeah, but then there can be races and keeping them separate is a better thing IMO. I was thinking of something on these lines, disable_irq() followed by enable_irq(): CPU0 CPU1 disable_irq() -> irq_bus_lock() -> irq_mask() -> irq_bus_sync_unlock() -> sends blocking VIRTIO_GPIO_MSG_IRQ_TYPE to disable interrupt Backend (at host) disables irq and returns the unused buffer. enable_irq() -> irq_bus_lock() -> irq_unmask() -> Tries to queues buffer again Finds it already queued and returns. - virtio_gpio_event_vq() runs at guest - Finds VIRTIO_GPIO_IRQ_STATUS_INVALID in status - returns without doing anything -> irq_bus_sync_unlock() -> sends blocking VIRTIO_GPIO_MSG_IRQ_TYPE to enable interrupt So the irq is still enabled and the buffer isn't queued. Yes, need some locking here for sure, confirmed :) -- viresh _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2021-08-05 12:49 UTC|newest] Thread overview: 32+ 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 ` Viresh Kumar 2021-08-03 11:36 ` [PATCH V4 1/2] gpio: Add virtio-gpio driver Viresh Kumar 2021-08-03 11:36 ` Viresh Kumar 2021-08-04 0:14 ` kernel test robot 2021-08-04 4:07 ` Viresh Kumar 2021-08-03 11:36 ` [PATCH V4 2/2] gpio: virtio: Add IRQ support Viresh Kumar 2021-08-03 11:36 ` Viresh Kumar 2021-08-03 15:01 ` Arnd Bergmann 2021-08-04 7:05 ` Viresh Kumar 2021-08-04 7:05 ` Viresh Kumar 2021-08-04 8:27 ` Arnd Bergmann 2021-08-05 7:05 ` Viresh Kumar 2021-08-05 7:05 ` Viresh Kumar 2021-08-05 11:26 ` 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 [this message] 2021-08-05 12:49 ` Viresh Kumar 2021-08-05 13:10 ` Arnd Bergmann 2021-08-06 7:44 ` Viresh Kumar 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:30 ` Viresh Kumar 2021-08-09 7:55 ` Arnd Bergmann 2021-08-09 10:46 ` Viresh Kumar 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 2021-08-10 7:35 ` Viresh Kumar 2021-08-04 6:29 ` kernel test robot
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=20210805124922.j7lts7tfmm4t2kpf@vireshk-i7 \ --to=viresh.kumar@linaro.org \ --cc=arnd@kernel.org \ --cc=bgolaszewski@baylibre.com \ --cc=cohuck@redhat.com \ --cc=geert@linux-m68k.org \ --cc=info@metux.net \ --cc=jasowang@redhat.com \ --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=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.