linux-kernel - Re: [PATCH] PCI/MSI: Clarify the irq sysfs ABI for PCI devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 20 Aug 2021 16:43:20 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Barry Song <21cnbao@...il.com>
Cc:     bhelgaas@...gle.com, corbet@....net, linux-kernel@...r.kernel.org,
        linux-pci@...r.kernel.org, mchehab+huawei@...nel.org,
        gregkh@...uxfoundation.org, Jonathan.Cameron@...wei.com,
        leon@...nel.org, schnelle@...ux.ibm.com, bilbao@...edu,
        luzmaximilian@...il.com, linuxarm@...wei.com,
        Barry Song <song.bao.hua@...ilicon.com>
Subject: Re: [PATCH] PCI/MSI: Clarify the irq sysfs ABI for PCI devices

On Sat, Aug 14, 2021 at 12:26:50AM +1200, Barry Song wrote:
> From: Barry Song <song.bao.hua@...ilicon.com>
> 
> /sys/bus/pci/devices/.../irq has been there for many years but it has never
> been documented. This patch is trying to document it. Plus, irq ABI is very
> confusing at this moment especially for MSI and MSI-x cases. MSI sets irq
> to the first number in the vector, but MSI-X does nothing for this though
> it saves default_irq in msix_setup_entries(). Weird the saved default_irq
> for MSI-X is never used in pci_msix_shutdown(), which is quite different
> with pci_msi_shutdown(). Thus, this patch also moves to show the first IRQ
> number which is from the first msi_entry for MSI-X. Hopefully, this can
> make irq ABI more clear and more consistent.

s/MSI-x/MSI-X/
s/irq/IRQ/

This looks like it should be two patches:

  - Update documentation

  - Some sort of bug fix in msix_capability_init() and pci_msix_shutdown()

Perhaps do the bug fix first if you want the doc to match the code.

> Signed-off-by: Barry Song <song.bao.hua@...ilicon.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-pci | 8 ++++++++
>  drivers/pci/msi.c                       | 6 ++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index 793cbb7..8d42385 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -96,6 +96,14 @@ Description:
>  		This attribute indicates the mode that the irq vector named by
>  		the file is in (msi vs. msix)
>  
> +What:		/sys/bus/pci/devices/.../irq
> +Date:		August 2021
> +Contact:	Barry Song <song.bao.hua@...ilicon.com>
> +Description:
> +		Historically this attribute represent the IRQ line which runs
> +		from the PCI device to the Interrupt controller. With MSI and
> +		MSI-X, this attribute is the first IRQ number of IRQ vectors.
> +
>  What:		/sys/bus/pci/devices/.../remove
>  Date:		January 2009
>  Contact:	Linux PCI developers <linux-pci@...r.kernel.org>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 9232255..6bbf81b 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -771,6 +771,7 @@ static int msix_capability_init(struct pci_dev *dev, struct msix_entry *entries,
>  	int ret;
>  	u16 control;
>  	void __iomem *base;
> +	struct msi_desc *desc;
>  
>  	/* Ensure MSI-X is disabled while it is set up */
>  	pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
> @@ -814,6 +815,10 @@ static int msix_capability_init(struct pci_dev *dev, struct msix_entry *entries,
>  	pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0);
>  
>  	pcibios_free_irq(dev);
> +
> +	desc = first_pci_msi_entry(dev);
> +	dev->irq = desc->irq;
> +
>  	return 0;
>  
>  out_avail:
> @@ -1024,6 +1029,7 @@ static void pci_msix_shutdown(struct pci_dev *dev)
>  	pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
>  	pci_intx_for_msi(dev, 1);
>  	dev->msix_enabled = 0;
> +	dev->irq = entry->msi_attrib.default_irq;
>  	pcibios_alloc_irq(dev);
>  }
>  
> -- 
> 1.8.3.1
>