Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221012220312.308522-1-mst@redhat.com>
In-Reply-To: <20221012220312.308522-1-mst@redhat.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Wed, 12 Oct 2022 15:27:58 -0700
Message-ID: <CAHk-=wjennbcJPkVy5fk5Hwv9uZvuzJzi9CN0gEwNs5j-psKyA@mail.gmail.com>
Subject: Re: [PATCH] virtio_pci: use irq to detect interrupt support
To:     "Michael S. Tsirkin" <mst@redhat.com>
Cc:     linux-kernel@vger.kernel.org,
        Michael Ellerman <mpe@ellerman.id.au>,
        Angus Chen <angus.chen@jaguarmicro.com>,
        Jason Wang <jasowang@redhat.com>,
        virtualization@lists.linux-foundation.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Oct 12, 2022 at 3:04 PM Michael S. Tsirkin <mst@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