mbox series

[V3,00/35] genirq/msi, PCI/MSI: Spring cleaning - Part 2

Message ID 20211210221642.869015045@linutronix.de (mailing list archive)
Headers show
Series genirq/msi, PCI/MSI: Spring cleaning - Part 2 | expand

Message

Thomas Gleixner Dec. 10, 2021, 10:18 p.m. UTC
This is the second part of [PCI]MSI refactoring which aims to provide the
ability of expanding MSI-X vectors after enabling MSI-X.

This is based on the first part of this work which can be found here:

    https://lore.kernel.org/r/20211206210147.872865823@linutronix.de

and has been applied to:

     git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/msi


This second part has the following important changes:

   1) Cleanup of the MSI related data in struct device

      struct device contains at the moment various MSI related parts. Some
      of them (the irq domain pointer) cannot be moved out, but the rest
      can be allocated on first use. This is in preparation of adding more
      per device MSI data later on.

   2) Consolidation of sysfs handling

      As a first step this moves the sysfs pointer from struct msi_desc
      into the new per device MSI data structure where it belongs.

      Later changes will cleanup this code further, but that's not possible
      at this point.

   3) Use PCI device properties instead of looking up MSI descriptors and
      analysing their data.

   4) Provide a function to retrieve the Linux interrupt number for a given
      MSI index similar to pci_irq_vector() and cleanup all open coded
      variants.

It's also available from git:

     git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git msi-v3-part-2

Part 3 of this effort is available on top

     git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git msi-v3-part-3

     Part 3 is not going to be reposted as there is no change vs. V2.

V2 of part 2 can be found here:

    https://lore.kernel.org/r/20211206210307.625116253@linutronix.de

Changes versus V2:

  - Use PCI device properties instead of creating a new set - Jason

  - Picked up Reviewed/Tested/Acked-by tags as appropriate

Thanks,

	tglx
---
 arch/powerpc/platforms/cell/axon_msi.c              |    5 
 arch/powerpc/platforms/pseries/msi.c                |   38 +---
 arch/x86/kernel/apic/msi.c                          |    5 
 arch/x86/pci/xen.c                                  |   11 -
 drivers/base/platform-msi.c                         |  152 ++++++++-----------
 drivers/bus/fsl-mc/dprc-driver.c                    |    8 -
 drivers/bus/fsl-mc/fsl-mc-allocator.c               |    9 -
 drivers/bus/fsl-mc/fsl-mc-msi.c                     |   26 +--
 drivers/dma/mv_xor_v2.c                             |   16 --
 drivers/dma/qcom/hidma.c                            |   44 ++---
 drivers/dma/ti/k3-udma-private.c                    |    6 
 drivers/dma/ti/k3-udma.c                            |   14 -
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c         |   23 --
 drivers/irqchip/irq-mbigen.c                        |    4 
 drivers/irqchip/irq-mvebu-icu.c                     |   12 -
 drivers/irqchip/irq-ti-sci-inta.c                   |    2 
 drivers/mailbox/bcm-flexrm-mailbox.c                |    9 -
 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c    |    4 
 drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c    |    4 
 drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c |    5 
 drivers/pci/msi/irqdomain.c                         |   20 ++
 drivers/pci/msi/legacy.c                            |    6 
 drivers/pci/msi/msi.c                               |  133 ++++++----------
 drivers/pci/xen-pcifront.c                          |    2 
 drivers/perf/arm_smmuv3_pmu.c                       |    5 
 drivers/soc/fsl/dpio/dpio-driver.c                  |    8 -
 drivers/soc/ti/k3-ringacc.c                         |    6 
 drivers/soc/ti/ti_sci_inta_msi.c                    |   22 --
 drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c              |    4 
 include/linux/device.h                              |   25 ++-
 include/linux/fsl/mc.h                              |    4 
 include/linux/msi.h                                 |   95 ++++--------
 include/linux/pci.h                                 |    1 
 include/linux/soc/ti/ti_sci_inta_msi.h              |    1 
 kernel/irq/msi.c                                    |  158 +++++++++++++++-----
 35 files changed, 429 insertions(+), 458 deletions(-)

Comments

Nishanth Menon Dec. 13, 2021, 6:29 p.m. UTC | #1
On 23:18-20211210, Thomas Gleixner wrote:
[...]

> 
> It's also available from git:
> 
>      git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git msi-v3-part-2

[...]

> ---
>  drivers/dma/ti/k3-udma-private.c                    |    6 
>  drivers/dma/ti/k3-udma.c                            |   14 -
>  drivers/irqchip/irq-ti-sci-inta.c                   |    2 
>  drivers/soc/ti/k3-ringacc.c                         |    6 
>  drivers/soc/ti/ti_sci_inta_msi.c                    |   22 --
>  include/linux/soc/ti/ti_sci_inta_msi.h              |    1 

Also while testing on TI K3 platforms, I noticed:

msi_device_data_release/msi_device_destroy_sysfs in am64xx-evm / j7200
[1] https://gist.github.com/nmenon/36899c7819681026cfe1ef185fb95f33#file-am64xx-evm-txt-L1018
[2] https://gist.github.com/nmenon/36899c7819681026cfe1ef185fb95f33#file-j7200-evm-txt-L1076

Which is not present in vanilla v5.16-rc4

v5.16-rc4:
https://gist.github.com/nmenon/1aee3f0a7da47d5e9dcb7336b32a70cb

msi-v3-part-2:
https://gist.github.com/nmenon/36899c7819681026cfe1ef185fb95f33

(.config https://gist.github.com/nmenon/ec6f95303828abf16a64022d8e3a269f)

Vs:
next-20211208:
https://gist.github.com/nmenon/f5ca3558bd5c1fbe62dc5ceb420b536e
Thomas Gleixner Dec. 14, 2021, 9:41 a.m. UTC | #2
On Mon, Dec 13 2021 at 12:29, Nishanth Menon wrote:
> On 23:18-20211210, Thomas Gleixner wrote:
> Also while testing on TI K3 platforms, I noticed:
>
> msi_device_data_release/msi_device_destroy_sysfs in am64xx-evm / j7200

The warning complains about a device being released with MSI descriptors
still attached to the device. This was added by:

  5b012cede0f7 ("device: Add device::msi_data pointer and struct msi_device_data")

That's not a regression caused by this commit. The warning is just
exposing an already existing problem in the iwlwifi driver, which seems
to do:

   probe()
     setup_pci_msi[x]_interrupts()
     start_drv()
       if (try_to_load_firmware() == FAIL)
       	   device_release_driver()
                ...
                msi_device_data_release()
                    WARN()

Thanks,

        tglx
Nishanth Menon Dec. 14, 2021, 4:22 p.m. UTC | #3
On 10:41-20211214, Thomas Gleixner wrote:
> On Mon, Dec 13 2021 at 12:29, Nishanth Menon wrote:
> > On 23:18-20211210, Thomas Gleixner wrote:
> > Also while testing on TI K3 platforms, I noticed:
> >
> > msi_device_data_release/msi_device_destroy_sysfs in am64xx-evm / j7200
> 
> The warning complains about a device being released with MSI descriptors
> still attached to the device. This was added by:
> 
>   5b012cede0f7 ("device: Add device::msi_data pointer and struct msi_device_data")
> 
> That's not a regression caused by this commit. The warning is just
> exposing an already existing problem in the iwlwifi driver, which seems
> to do:
> 
>    probe()
>      setup_pci_msi[x]_interrupts()
>      start_drv()
>        if (try_to_load_firmware() == FAIL)
>        	   device_release_driver()
>                 ...
>                 msi_device_data_release()
>                     WARN()
> 

Agreed that the warning is fine, the null pointer exception that follows
[1] [2] it however does'nt look right and it can be trivially fixed with the
following fixup for ee90787487bc ("genirq/msi: Provide
msi_device_populate/destroy_sysfs()") below, with that the log looks
like [3] - the warn is good, the null pointer exception and resultant
crash could be avoided (not saying this is the best solution):

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index ab5e83f41188..24edb870c66f 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -252,11 +252,14 @@ int msi_device_populate_sysfs(struct device *dev)
  */
 void msi_device_destroy_sysfs(struct device *dev)
 {
-	const struct attribute_group **msi_irq_groups = dev->msi.data->attrs;
+	const struct attribute_group **msi_irq_groups;
 	struct device_attribute *dev_attr;
 	struct attribute **msi_attrs;
 	int count = 0;
 
+	if (!dev->msi.data)
+		return;
+	msi_irq_groups = dev->msi.data->attrs;
 	dev->msi.data->attrs = NULL;
 	if (!msi_irq_groups)
 		return;

[1] https://gist.github.com/nmenon/36899c7819681026cfe1ef185fb95f33#file-am64xx-evm-txt-L1049
[2] https://gist.github.com/nmenon/36899c7819681026cfe1ef185fb95f33#file-j7200-evm-txt-L1111

[3] https://gist.github.com/nmenon/575afe7d04463026a7e420a76c2c1c5b
	https://gist.github.com/nmenon/575afe7d04463026a7e420a76c2c1c5b#file-am64xx-evm-txt-L1018
	https://gist.github.com/nmenon/575afe7d04463026a7e420a76c2c1c5b#file-j7200-evm-txt-L1053
Thomas Gleixner Dec. 14, 2021, 4:36 p.m. UTC | #4
On Tue, Dec 14 2021 at 10:22, Nishanth Menon wrote:
> On 10:41-20211214, Thomas Gleixner wrote:
> Agreed that the warning is fine, the null pointer exception that follows
> [1] [2] it however does'nt look right and it can be trivially fixed with the
> following fixup for ee90787487bc ("genirq/msi: Provide
> msi_device_populate/destroy_sysfs()") below, with that the log looks
> like [3] - the warn is good, the null pointer exception and resultant
> crash could be avoided (not saying this is the best solution):

Aaargh.

[   13.478122] Call trace:
[   13.509042]  msi_device_destroy_sysfs+0x18/0x88
[   13.509058]  msi_domain_free_irqs+0x34/0x58
[   13.509064]  pci_msi_teardown_msi_irqs+0x30/0x3c
[   13.509072]  free_msi_irqs+0x78/0xd4
[   13.509077]  pci_disable_msix+0x138/0x164
[   13.529930]  pcim_release+0x70/0x238
[   13.529942]  devres_release_all+0x9c/0xfc
[   13.529951]  device_release_driver_internal+0x1a0/0x244
[   13.542725]  device_release_driver+0x18/0x24
[   13.542741]  iwl_req_fw_callback+0x1a28/0x1ddc [iwlwifi]
[   13.552308]  request_firmware_work_func+0x50/0x9c
[   13.552320]  process_one_work+0x194/0x25c

That's not a driver problem, that's an ordering issue vs. the devres
muck. Let me go back to the drawing board. Sigh...

Thanks,

        tglx
Thomas Gleixner Dec. 14, 2021, 5:03 p.m. UTC | #5
On Tue, Dec 14 2021 at 17:36, Thomas Gleixner wrote:
> On Tue, Dec 14 2021 at 10:22, Nishanth Menon wrote:
>> On 10:41-20211214, Thomas Gleixner wrote:
> [   13.478122] Call trace:
> [   13.509042]  msi_device_destroy_sysfs+0x18/0x88
> [   13.509058]  msi_domain_free_irqs+0x34/0x58
> [   13.509064]  pci_msi_teardown_msi_irqs+0x30/0x3c
> [   13.509072]  free_msi_irqs+0x78/0xd4
> [   13.509077]  pci_disable_msix+0x138/0x164
> [   13.529930]  pcim_release+0x70/0x238
> [   13.529942]  devres_release_all+0x9c/0xfc
> [   13.529951]  device_release_driver_internal+0x1a0/0x244
> [   13.542725]  device_release_driver+0x18/0x24
> [   13.542741]  iwl_req_fw_callback+0x1a28/0x1ddc [iwlwifi]
> [   13.552308]  request_firmware_work_func+0x50/0x9c
> [   13.552320]  process_one_work+0x194/0x25c
>
> That's not a driver problem, that's an ordering issue vs. the devres
> muck. Let me go back to the drawing board. Sigh...

Which is pretty obvious why:

   pcim_enable_device()
        devres_alloc(pcim_release...);
        ...
        pci_irq_alloc()
          msi_setup_device_data()
             devres_alloc(msi_device_data_release, ...)

and once the device is released:

    msi_device_data_release()
    ...
    pcim_release()
       pci_disable_msi[x]()

Groan....
Thomas Gleixner Dec. 14, 2021, 8:15 p.m. UTC | #6
Nishanth,

On Tue, Dec 14 2021 at 18:03, Thomas Gleixner wrote:
>     msi_device_data_release()
>     ...
>     pcim_release()
>        pci_disable_msi[x]()
>
> Groan....

I think I managed to distangle this. Can you please give:

   git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git msi-v4-part-2

and/or the full pile:

   git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git msi-v4-part-3

a test ride?

Thanks,

        tglx
Nishanth Menon Dec. 14, 2021, 8:56 p.m. UTC | #7
On 21:15-20211214, Thomas Gleixner wrote:
> Nishanth,
> 
> On Tue, Dec 14 2021 at 18:03, Thomas Gleixner wrote:
> >     msi_device_data_release()
> >     ...
> >     pcim_release()
> >        pci_disable_msi[x]()
> >
> > Groan....
> 
> I think I managed to distangle this. Can you please give:
> 
>    git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git msi-v4-part-2


Umm.. I am not entirely sure what is going on.. but all kinds of weird
corruption seems to occur with msi-v4-part-2 that does'nt seem to be
present in v5.16-rc5. (I use NFS since ethernet in K3 platforms use
inta/intr and dma that is impacted by this series).

I will try and rebase your patches on v5.16-rc4 to be sure as well and
report back later today once i get some time.

[1] https://gist.github.com/nmenon/a66e022926c4c15313c45d44313d860c msi-v4-part-2
[2] https://gist.github.com/nmenon/43085664d69ad846d596e76a06ed0656  v5.16-rc5
Thomas Gleixner Dec. 14, 2021, 9:19 p.m. UTC | #8
Nishanth,

On Tue, Dec 14 2021 at 14:56, Nishanth Menon wrote:
> On 21:15-20211214, Thomas Gleixner wrote:
>> I think I managed to distangle this. Can you please give:
>> 
>>    git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git msi-v4-part-2
>
>
> Umm.. I am not entirely sure what is going on.. but all kinds of weird
> corruption seems to occur with msi-v4-part-2 that does'nt seem to be
> present in v5.16-rc5. (I use NFS since ethernet in K3 platforms use
> inta/intr and dma that is impacted by this series).
>
> I will try and rebase your patches on v5.16-rc4 to be sure as well and
> report back later today once i get some time.
>
> [1] https://gist.github.com/nmenon/a66e022926c4c15313c45d44313d860c msi-v4-part-2
> [2] https://gist.github.com/nmenon/43085664d69ad846d596e76a06ed0656  v5.16-rc5

thanks for trying. I'll have a look again with brain awake tomorrow
morning.

Thanks,

        tglx
Thomas Gleixner Dec. 15, 2021, 4:18 p.m. UTC | #9
On Tue, Dec 14 2021 at 22:19, Thomas Gleixner wrote:
> On Tue, Dec 14 2021 at 14:56, Nishanth Menon wrote:
>
> thanks for trying. I'll have a look again with brain awake tomorrow
> morning.

Morning was busy with other things, but I found what my sleepy brain
managed to do wrong yesterday evening.

Let me reintegrate the pile and I'll send you an update.

Thanks,

        tglx
Thomas Gleixner Dec. 15, 2021, 4:35 p.m. UTC | #10
On Wed, Dec 15 2021 at 17:18, Thomas Gleixner wrote:

> On Tue, Dec 14 2021 at 22:19, Thomas Gleixner wrote:
>> On Tue, Dec 14 2021 at 14:56, Nishanth Menon wrote:
>>
>> thanks for trying. I'll have a look again with brain awake tomorrow
>> morning.
>
> Morning was busy with other things, but I found what my sleepy brain
> managed to do wrong yesterday evening.
>
> Let me reintegrate the pile and I'll send you an update.

   git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git msi-v4.1-part-2
   git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git msi-v4.2-part-3

That should cure the problem.
Nishanth Menon Dec. 15, 2021, 6:08 p.m. UTC | #11
On 17:35-20211215, Thomas Gleixner wrote:
>    git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git msi-v4.1-part-2
[...]

> That should cure the problem.

And it sure does. Thanks for looking closer and providing a fix.

https://gist.github.com/nmenon/9862a1c31b17fd6dfe0a30c54d396187
(msi-v4.1-part-2) looks clean

Also while I had detected pointer corruption in the previous v4
https://gist.github.com/nmenon/ce4d12f460db5cd511185c047d5d35d0

Running it again on v4.1 does indicate the fix is in place.
https://gist.github.com/nmenon/3231fbb0faa1b9c827b40d1ae6160626


please feel free to add:

Tested-by: Nishanth Menon <nm@ti.com>
Nishanth Menon Dec. 16, 2021, 1:45 a.m. UTC | #12
Hi Thomas,

On 17:35-20211215, Thomas Gleixner wrote:
>    git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git msi-v4.2-part-3

As you helped offline, summarizing the details on part3 of the series:

I was seeing failure[1] of NFS(DMA) on all TI K3 platforms:

[    1.013258] ti-bcdma 485c0100.dma-controller: Number of rings: 68
[    1.019963] ti-bcdma 485c0100.dma-controller: Failed to allocate IRQs -28
[    1.026938] ti-bcdma 485c0100.dma-controller: Failed to allocate MSI interrupts

Rationale as you explained:
"
-28 is ENOSPC, which is returned when the interrupt allocation in the
 MSI domain fails. Fix below.
"

Which turned out to be the fixup[2] you suggested and I confirm that
fixes the problem for me.

With the fixup in place:

Tested-by: Nishanth Menon <nm@ti.com>

for part 3 of the series as well.

Thanks once again for your help. Hope we can roll in the fixes for
part3.

[1] https://gist.github.com/nmenon/5971ab27aa626c022e276cc946e4b6c3
[2]
--- a/drivers/soc/ti/ti_sci_inta_msi.c
+++ b/drivers/soc/ti/ti_sci_inta_msi.c
@@ -68,6 +68,7 @@ static int ti_sci_inta_msi_alloc_descs(s
 	int set, i, count = 0;
 
 	memset(&msi_desc, 0, sizeof(msi_desc));
+	msi_desc.nvec_used = 1;
 
 	for (set = 0; set < res->sets; set++) {
 		for (i = 0; i < res->desc[set].num; i++, count++) {
Michael Kelley (LINUX) Dec. 16, 2021, 6:14 a.m. UTC | #13
From: Thomas Gleixner <tglx@linutronix.de> Sent: Wednesday, December 15, 2021 8:36 AM
> 
> On Wed, Dec 15 2021 at 17:18, Thomas Gleixner wrote:
> 
> > On Tue, Dec 14 2021 at 22:19, Thomas Gleixner wrote:
> >> On Tue, Dec 14 2021 at 14:56, Nishanth Menon wrote:
> >>
> >> thanks for trying. I'll have a look again with brain awake tomorrow
> >> morning.
> >
> > Morning was busy with other things, but I found what my sleepy brain
> > managed to do wrong yesterday evening.
> >
> > Let me reintegrate the pile and I'll send you an update.
> 
>    git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git msi-v4.1-part-2
>    git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git msi-v4.2-part-3
> 
> That should cure the problem.

Tested the msi-v4.2-part-3 tag in two different Azure/Hyper-V VMs.  One
is a Generation 1 VM that has legacy PCI devices and one is a Generation 2
VM with no legacy PCI devices.   Tested hot add and remove of Mellanox
CX-3 and CX-4 SR-IOV NIC virtual functions that are directly mapped into the
VM.  Also tested local NVMe devices directly mapped into one of the VMs.

No issues encountered.  So for Azure/Hyper-V specifically,

Tested-by: Michael Kelley <mikelley@microsoft.com>
Thomas Gleixner Dec. 16, 2021, 5:23 p.m. UTC | #14
Nishanth,

On Wed, Dec 15 2021 at 19:45, Nishanth Menon wrote:
> On 17:35-20211215, Thomas Gleixner wrote:
> Thanks once again for your help. Hope we can roll in the fixes for
> part3.

Sure, it's only the one-liner for ti sci. Got it folded already.

Thanks for your help and testing!

       tglx