linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Viresh Kumar <viresh.kumar@linaro.org>
To: Arnd Bergmann <arnd@kernel.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: Thu, 5 Aug 2021 12:35:34 +0530	[thread overview]
Message-ID: <20210805070534.lia6bc5c2l73d7cf@vireshk-i7> (raw)
In-Reply-To: <CAK8P3a0vBhP6ZJuqCxAUVZG_eAPu1Lk_Nw8FEarnHb286pPPyQ@mail.gmail.com>

On 04-08-21, 10:27, Arnd Bergmann wrote:
> On Wed, Aug 4, 2021 at 9:05 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 03-08-21, 17:01, Arnd Bergmann wrote:
> > > >
> > > > +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
> >
> > We can't because its a slow bus ? And unmask can be called from
> > irq-context. That's exactly why we had the irq_bus_lock/unlock
> > discussion earlier.
> 
> I thought only 'mask' is slow, since that has to wait for the completion,
> but 'unmask' just involves sending the eventq request without having
> to wait for it.

Ahh, so you are suggesting of just sending the request without waiting
for the response. We can do it, but I am not sure if it is going to be
worth it considering the additional complexity in the driver.

Maybe lets come back to this later, if this is going to make anything
better, once the basic support for irqs look good here.

> I don't think it is correct that you cannot issue virtio requests from
> atomic context, only that you cannot wait for the reply.

Yes, you are right. I was having a mess because of all the races and
different ways things were getting called.

> For 'unmask', there is no waiting, since the reply is the actual IRQ
> event. For the others, the sequence makes sense.
 
> > - Interrupt: i.e. buffer sent back by the host over virtio.
> >
> >   virtio_gpio_event_vq() schedules a work item, which processes the
> >   items from the eventq virtqueue and eventually calls
> >   generic_handle_irq(). The irq-core can issue calls to
> >   ->irq_mask/unmask() here without a prior call to
> >   irq_bus_lock/unlock(), normally they will balance out by the end,
> >   but I am not sure if it is guaranteed. Moreover, interrupt should be
> >   re-enabled only after unmask() is called (for ONESHOT) and not at
> >   EOI, right ?
> >
> >   I chose not to queue the buffers back from eoi() as it is possible
> >   that we won't want to queue them at all, as the interrupt needs to
> >   be disabled by the time generic_handle_irq() returns. And so I did
> >   everything from the end of vgpio_work_handler()'s loop, i.e. either
> >   disable the interrupts with VIRTIO_GPIO_IRQ_TYPE_NONE or enable the
> >   interrupt again by re-queuing the buffer.
> >
> > Regarding irq handling using work-item, I had to move to that to take
> > care of locking for re-queuing the buffers for a GPIO line from
> > irq-handler and bus-unlock. Nothing else seemed to work, though I am
> > continuing to look into that to see if there is an alternative here.
> 
> I don't think it makes sense to optimize for the rare case that the
> irq handler disables the irq, when that makes the common case
> (irq remains unmasked and enabled) much slower.

I am not worried about that case only, i.e. where the
interrupt-handler disables the interrupt. It should work fine though,
even if it isn't optimized.

A) If the client has registered with irq with IRQF_ONESHOT, irq core
   will call ->irq_mask(), irq-handler(), ->irq_eoi(), and finally
   ->irq_unmask(). Looking at this basic sequence itself, irq_eoi()
   shouldn't enable the interrupt again by re-queuing the buffer,
   rather it should happen from unmask in this case.

B) If the client has registered a threaded-irq, it gets even more
   complex. The core does this in that case:

   call ->irq_mask(), ->irq_eoi(), and return control back to
   virtio_gpio_event_vq().

   The thread gets scheduled then and calls the irq-handler and then
   the irq-core calls irq-bus-lock(), ->irq_unmask(),
   irq-bus-unlock().

Looking at these, eoi shouldn't be queuing the buffer. Also, I wonder
if we should use handle_fasteoi_irq() here or something else.

-- 
viresh

  reply	other threads:[~2021-08-05  7:05 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
2021-08-04  7:05     ` Viresh Kumar
2021-08-04  8:27       ` Arnd Bergmann
2021-08-05  7:05         ` Viresh Kumar [this message]
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=20210805070534.lia6bc5c2l73d7cf@vireshk-i7 \
    --to=viresh.kumar@linaro.org \
    --cc=alex.bennee@linaro.org \
    --cc=arnd@kernel.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=vireshk@kernel.org \
    --cc=virtualization@lists.linux-foundation.org \
    --subject='Re: [PATCH V4 2/2] gpio: virtio: Add IRQ support' \
    /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

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).