thr3ads.net - Virtualization - [PATCH] virtio_pci: use irq to detect interrupt support [Oct 2022]

If this information is useful, please help other people find it:
Share via:

Michael S. Tsirkin

2022-Oct-12 22:04 UTC

[PATCH] virtio_pci: use irq to detect interrupt support

commit 71491c54eafa ("virtio_pci: don't try to use intxif pin is
zero")
breaks virtio_pci on powerpc, when running as a qemu guest.

vp_find_vqs() bails out because pci_dev->pin == 0.

But pci_dev->irq is populated correctly, so vp_find_vqs_intx() would
succeed if we called it - which is what the code used to do.

This seems to happen because pci_dev->pin is not populated in
pci_assign_irq().

Which is absolutely a bug in the relevant PCI code, but it
may also affect other platforms that use of_irq_parse_and_map_pci().

However Linus said:
	The correct way to check for "no irq" doesn't use NO_IRQ at all,
it just does
		if (dev->irq) ...
so let's just check irq and be done with it.

Suggested-by: Linus Torvalds <torvalds at linux-foundation.org>
Reported-by: Michael Ellerman <mpe at ellerman.id.au>
Fixes: 71491c54eafa ("virtio_pci: don't try to use intxif pin is
zero")
Cc: "Angus Chen" <angus.chen at jaguarmicro.com>
Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---

Build tested only - very late here. Angus any chance you could
help test this? Thanks!

 drivers/virtio/virtio_pci_common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c
b/drivers/virtio/virtio_pci_common.c
index 4df77eeb4d16..a6c86f916dbd 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -409,8 +409,8 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned int
nvqs,
 	err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, false, ctx, desc);
 	if (!err)
 		return 0;
-	/* Is there an interrupt pin? If not give up. */
-	if (!(to_vp_device(vdev)->pci_dev->pin))
+	/* Is there an interrupt? If not give up. */
+	if (!(to_vp_device(vdev)->pci_dev->irq))
 		return err;
 	/* Finally fall back to regular interrupts. */
 	return vp_find_vqs_intx(vdev, nvqs, vqs, callbacks, names, ctx);
-- 
MST

Linus Torvalds

2022-Oct-12 22:27 UTC

head link

[PATCH] virtio_pci: use irq to detect interrupt support

On Wed, Oct 12, 2022 at 3:04 PM Michael S. Tsirkin <mst at redhat.com>
wrote:>
> This seems to happen because pci_dev->pin is not populated in
> pci_assign_irq().
Yeah, I have to say, I wonder why it was looking at pci_dev->pin in
the first place.

I was curious, as this is fairly odd.

Of course, I only did a fairly strange 'grep' for this thing, so I
might have missed some other use:

    git grep -e '->pin\>' $(git grep -l 'struct pci_dev')

and the above will possibly find other uses of 'pin' as a member than
the 'struct pci_dev', so I'm not going to claim the above is
exhaustive, but looking at the results, I do see how ACPI has somewhat
similar logic, and acpi_pci_irq_enable() does this:

        ...
        pin = dev->pin;
        if (!pin) {
                dev_dbg(&dev->dev, "No interrupt pin
configured\n");
                return 0;
        }
        ...

but in the end that seems to be because it's then later using the pin
to do the actual PCI IRQ routing table lookup, and then it uses that
value to actually look up the IRQ number:

        dev->irq = rc;
        dev->irq_managed = 1;

and in fact all this code also has a "have I already looked this up"
and then it doesn't do anything (but somewhat illogically, it does
that *after* having tested for that "!pin" condition - I think it
would make more sense to go "oh, I already have this interrupt mapped,
I shouldn't care about the pin", but I suspect it doesn't matter in
practice).

And I really think that that is basically the only time you should use
that 'pci_dev->pin' thing: it basically exists not for "does
this
device have an IRQ", but for "what is the routing of this irq on this
device".

There's also some testing of dev->pin in drives/pci/pci.c and
drivers/pci/probe.c, but it seems to be very similar: it's actually
doing the irq lookup, and the pin swizzling that goes along with it.

IOW, I think that yes, this patch makes sense, and virtio_pci was
doing something odd. That virtio_pci driver at no point actually cares
about the PCI pin, will not do any PCI irq routing lookup with it, it
will just do

        err = request_irq(vp_dev->pci_dev->irq, [...]

using the pci_dev->irq that has already been looked up.

So the patch makes sense to me. If there is some problem with
pci_dev->irq, then it's up to request_irq() to complain about it (ie
we have things like IRQ_NOTCONNECTED etc, which is a more modern-day
version of the old NO_IRQ thing).  Again, not something that
virtio_pci should check for itself, I think.

But I don't really know the virtio code. I can only say that "check
the pin" pattern seems entirely wrong and should only be done by code
that does actual irq routing, and "just check the irq" is what
everybody else does.

                    Linus

Michael Ellerman

2022-Oct-13 01:34 UTC

head link

[PATCH] virtio_pci: use irq to detect interrupt support

"Michael S. Tsirkin" <mst at redhat.com>
writes:> commit 71491c54eafa ("virtio_pci: don't try to use intxif pin is
zero")
> breaks virtio_pci on powerpc, when running as a qemu guest.
>
> vp_find_vqs() bails out because pci_dev->pin == 0.
>
> But pci_dev->irq is populated correctly, so vp_find_vqs_intx() would
> succeed if we called it - which is what the code used to do.
>
> This seems to happen because pci_dev->pin is not populated in
> pci_assign_irq().
>
> Which is absolutely a bug in the relevant PCI code, but it
> may also affect other platforms that use of_irq_parse_and_map_pci().
>
> However Linus said:
> 	The correct way to check for "no irq" doesn't use NO_IRQ at
all, it just does
> 		if (dev->irq) ...
> so let's just check irq and be done with it.
>
> Suggested-by: Linus Torvalds <torvalds at linux-foundation.org>
> Reported-by: Michael Ellerman <mpe at ellerman.id.au>
> Fixes: 71491c54eafa ("virtio_pci: don't try to use intxif pin is
zero")
> Cc: "Angus Chen" <angus.chen at jaguarmicro.com>
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>
> Build tested only - very late here. Angus any chance you could
> help test this? Thanks!
This works for me on powerpc.

Tested-by: Michael Ellerman <mpe at ellerman.id.au>

cheers
> diff --git a/drivers/virtio/virtio_pci_common.c
b/drivers/virtio/virtio_pci_common.c
> index 4df77eeb4d16..a6c86f916dbd 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -409,8 +409,8 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned
int nvqs,
>  	err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, false, ctx,
desc);
>  	if (!err)
>  		return 0;
> -	/* Is there an interrupt pin? If not give up. */
> -	if (!(to_vp_device(vdev)->pci_dev->pin))
> +	/* Is there an interrupt? If not give up. */
> +	if (!(to_vp_device(vdev)->pci_dev->irq))
>  		return err;
>  	/* Finally fall back to regular interrupts. */
>  	return vp_find_vqs_intx(vdev, nvqs, vqs, callbacks, names, ctx);
> -- 
> MST

Jason Wang

2022-Oct-13 02:29 UTC

head link

[PATCH] virtio_pci: use irq to detect interrupt support

On Thu, Oct 13, 2022 at 6:04 AM Michael S. Tsirkin <mst at redhat.com>
wrote:>
> commit 71491c54eafa ("virtio_pci: don't try to use intxif pin is
zero")
> breaks virtio_pci on powerpc, when running as a qemu guest.
>
> vp_find_vqs() bails out because pci_dev->pin == 0.
>
> But pci_dev->irq is populated correctly, so vp_find_vqs_intx() would
> succeed if we called it - which is what the code used to do.
>
> This seems to happen because pci_dev->pin is not populated in
> pci_assign_irq().
>
> Which is absolutely a bug in the relevant PCI code, but it
> may also affect other platforms that use of_irq_parse_and_map_pci().
>
> However Linus said:
>         The correct way to check for "no irq" doesn't use
NO_IRQ at all, it just does
>                 if (dev->irq) ...
> so let's just check irq and be done with it.
>
> Suggested-by: Linus Torvalds <torvalds at linux-foundation.org>
> Reported-by: Michael Ellerman <mpe at ellerman.id.au>
> Fixes: 71491c54eafa ("virtio_pci: don't try to use intxif pin is
zero")
> Cc: "Angus Chen" <angus.chen at jaguarmicro.com>
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
Acked-by: Jason Wang <jasowang at redhat.com>
> ---
>
> Build tested only - very late here. Angus any chance you could
> help test this? Thanks!
>
>  drivers/virtio/virtio_pci_common.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c
b/drivers/virtio/virtio_pci_common.c
> index 4df77eeb4d16..a6c86f916dbd 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -409,8 +409,8 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned
int nvqs,
>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, false,
ctx, desc);
>         if (!err)
>                 return 0;
> -       /* Is there an interrupt pin? If not give up. */
> -       if (!(to_vp_device(vdev)->pci_dev->pin))
> +       /* Is there an interrupt? If not give up. */
> +       if (!(to_vp_device(vdev)->pci_dev->irq))
>                 return err;
>         /* Finally fall back to regular interrupts. */
>         return vp_find_vqs_intx(vdev, nvqs, vqs, callbacks, names, ctx);
> --
> MST
>

Virtualization - Oct 2022 - [PATCH] virtio_pci: use irq to detect interrupt support

[PATCH] virtio_pci: use irq to detect interrupt support

[PATCH] virtio_pci: use irq to detect interrupt support

[PATCH] virtio_pci: use irq to detect interrupt support

[PATCH] virtio_pci: use irq to detect interrupt support