From: Marc Zyngier <maz@kernel.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Barry Song <21cnbao@gmail.com>,
Bjorn Helgaas <bhelgaas@google.com>,
Jonathan Corbet <corbet@lwn.net>,
Jonathan.Cameron@huawei.com, bilbao@vt.edu,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
leon@kernel.org, LKML <linux-kernel@vger.kernel.org>,
linux-pci@vger.kernel.org, Linuxarm <linuxarm@huawei.com>,
luzmaximilian@gmail.com, mchehab+huawei@kernel.org,
schnelle@linux.ibm.com, Barry Song <song.bao.hua@hisilicon.com>,
Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH v2 1/2] PCI/MSI: Fix the confusing IRQ sysfs ABI for MSI-X
Date: Wed, 25 Aug 2021 10:45:28 +0100 [thread overview]
Message-ID: <87lf4pq2mf.wl-maz@kernel.org> (raw)
In-Reply-To: <20210824193438.GA3486820@bjorn-Precision-5520>
On Tue, 24 Aug 2021 20:34:38 +0100,
Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Tue, Aug 24, 2021 at 10:46:59AM +1200, Barry Song wrote:
> > On Mon, Aug 23, 2021 at 11:28 PM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Mon, 23 Aug 2021 12:03:08 +0100,
> > > Barry Song <21cnbao@gmail.com> wrote:
>
> > +static ssize_t irq_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct pci_dev *pdev = to_pci_dev(dev);
> > +#ifdef CONFIG_PCI_MSI
> > + struct msi_desc *desc = first_pci_msi_entry(pdev);
> > +
> > + /* for MSI, return the 1st IRQ in IRQ vector */
> > + if (desc && !desc->msi_attrib.is_msix)
> > + return sysfs_emit(buf, "%u\n", desc->irq);
> > +#endif
> > +
> > + return sysfs_emit(buf, "%u\n", pdev->irq);
> > +}
> > +static DEVICE_ATTR_RO(irq);
>
> Makes sense to me. And with Marc's patch maybe we could get rid of
> default_irq, which also seems nice.
>
> > > > if we don't want to change the behaviour of any existing ABI, it
> > > > seems the only thing we can do here to document it well in ABI
> > > > doc. i actually doubt anyone has really understood what the irq
> > > > entry is really showing.
> > >
> > > Given that we can't prove that it is actually the case, I believe this
> > > is the only option.
> >
> > we have to document the ABI like below though it seems quite annoying.
> >
> > 1. for devices which don't support MSI and MSI-X, show legacy INTx
> > 2. for devices which support MSI
> > a. if CONFIG_PCI_MSI is not enabled, show legacy INTx
> > b. if CONFIG_PCI_MSI is enabled and devices are using MSI at this
> > moment, show 1st IRQ in the vector
> > c. if CONFIG_PCI_MSI is enabled, but we shutdown its MSI before
> > the users call sysfs entry,
> > so at this moment, devices are not using MSI, show legacy INTx
> > 3. for devices which support MSI-X, no matter if it is using MSI-X,
> > show legacy INTx
> > 4. In Addition, INTx might be broken due to incomplete firmware or
> > hardware design for MSI and MSI-X cases
> >
> > To be honest, it sounds like a disaster :-) but if this is what we
> > have to do, I'd like to try it in v3.
>
> It doesn't seem necessary to me to get into the gory details of
> CONFIG_PCI_MSI -- if that's not enabled, drivers can't use MSI anyway.
>
> I don't understand 3. If a device supports both MSI and MSI-X and a
> driver enables MSI, msi_capability_init() writes dev->irq, so it looks
> like "irq" should contain the first MSI vector.
>
> I don't understand 4, either. Is the possibility of broken hardware
> or firmware something we need to document?
>
> What about something like this?
>
> If a driver has enabled MSI (not MSI-X), "irq" contains the IRQ of
> the first MSI vector. Otherwise "irq" contains the IRQ of the
> legacy INTx interrupt.
>
I think that pretty much nails it. CONFIG_MSI is not something that
userspace can (nor should) discover anyway.
For (4), you may want to add that
"irq" being set to 0 indicates that the device isn't capable of
generating legacy INTx interrupts.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
next prev parent reply other threads:[~2021-08-25 9:45 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-20 22:37 [PATCH v2 0/2] PCI/MSI: Clarify the IRQ sysfs ABI for PCI devices Barry Song
2021-08-20 22:37 ` [PATCH v2 1/2] PCI/MSI: Fix the confusing IRQ sysfs ABI for MSI-X Barry Song
2021-08-20 23:33 ` Bjorn Helgaas
2021-08-21 10:42 ` Marc Zyngier
2021-08-21 22:14 ` Barry Song
2021-08-21 22:41 ` Barry Song
2021-08-23 10:33 ` Marc Zyngier
2021-08-24 19:25 ` Bjorn Helgaas
2021-08-23 10:30 ` Marc Zyngier
2021-08-23 11:03 ` Barry Song
2021-08-23 11:28 ` Marc Zyngier
2021-08-23 22:46 ` Barry Song
2021-08-24 19:34 ` Bjorn Helgaas
2021-08-25 9:45 ` Marc Zyngier [this message]
2021-08-24 20:51 ` Barry Song
2021-08-24 21:29 ` Barry Song
2021-08-25 10:24 ` Marc Zyngier
2021-08-24 22:51 ` Barry Song
2021-08-20 22:37 ` [PATCH v2 2/2] Documentation: ABI: sysfs-bus-pci: Add description for IRQ entry Barry Song
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=87lf4pq2mf.wl-maz@kernel.org \
--to=maz@kernel.org \
--cc=21cnbao@gmail.com \
--cc=Jonathan.Cameron@huawei.com \
--cc=bhelgaas@google.com \
--cc=bilbao@vt.edu \
--cc=corbet@lwn.net \
--cc=gregkh@linuxfoundation.org \
--cc=helgaas@kernel.org \
--cc=leon@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linuxarm@huawei.com \
--cc=luzmaximilian@gmail.com \
--cc=mchehab+huawei@kernel.org \
--cc=schnelle@linux.ibm.com \
--cc=song.bao.hua@hisilicon.com \
--cc=tglx@linutronix.de \
/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).