[v2,1/1] powerpc/pci: Fix pcibios_setup_device() ordering
diff mbox series

Message ID 20190905191343.2919-2-shawn@anastas.io
State New
Headers show
Series
  • Fix IOMMU setup for hotplugged devices on pseries
Related show

Checks

Context Check Description
snowpatch_ozlabs/checkpatch success total: 0 errors, 0 warnings, 0 checks, 45 lines checked
snowpatch_ozlabs/build-pmac32 success Build succeeded
snowpatch_ozlabs/build-ppc64e success Build succeeded
snowpatch_ozlabs/build-ppc64be success Build succeeded
snowpatch_ozlabs/build-ppc64le success Build succeeded
snowpatch_ozlabs/apply_patch success Successfully applied on branch next (c317052c95bef1f977b023158e5aa929215f443d)

Commit Message

Shawn Anastasio Sept. 5, 2019, 7:13 p.m. UTC
Move PCI device setup from pcibios_add_device() and pcibios_fixup_bus() to
pcibios_bus_add_device(). This ensures that platform-specific DMA and IOMMU
setup occurs after the device has been registered in sysfs, which is a
requirement for IOMMU group assignment to work

This fixes IOMMU group assignment for hotplugged devices on pseries, where
the existing behavior results in IOMMU assignment before registration.

Thanks to Lukas Wunner <lukas@wunner.de> for the suggestion.

Signed-off-by: Shawn Anastasio <shawn@anastas.io>
---
 arch/powerpc/kernel/pci-common.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

Comments

Alexey Kardashevskiy Sept. 9, 2019, 7:59 a.m. UTC | #1
On 06/09/2019 05:13, Shawn Anastasio wrote:
> Move PCI device setup from pcibios_add_device() and pcibios_fixup_bus() to
> pcibios_bus_add_device(). This ensures that platform-specific DMA and IOMMU
> setup occurs after the device has been registered in sysfs, which is a
> requirement for IOMMU group assignment to work
> 
> This fixes IOMMU group assignment for hotplugged devices on pseries, where
> the existing behavior results in IOMMU assignment before registration.


Although this is a correct approach which we should proceed with, this
breaks adding of SRIOV VFs from pnv_tce_iommu_bus_notifier (and possibly
the bare metal PCI hotplug), I am trying to fix that now...


> 
> Thanks to Lukas Wunner <lukas@wunner.de> for the suggestion.
> 
> Signed-off-by: Shawn Anastasio <shawn@anastas.io>
> ---
>  arch/powerpc/kernel/pci-common.c | 25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index f627e15bb43c..d119c77efb69 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -261,12 +261,6 @@ int pcibios_sriov_disable(struct pci_dev *pdev)
>  
>  #endif /* CONFIG_PCI_IOV */
>  
> -void pcibios_bus_add_device(struct pci_dev *pdev)
> -{
> -	if (ppc_md.pcibios_bus_add_device)
> -		ppc_md.pcibios_bus_add_device(pdev);
> -}
> -
>  static resource_size_t pcibios_io_size(const struct pci_controller *hose)
>  {
>  #ifdef CONFIG_PPC64
> @@ -987,15 +981,17 @@ static void pcibios_setup_device(struct pci_dev *dev)
>  		ppc_md.pci_irq_fixup(dev);
>  }
>  
> -int pcibios_add_device(struct pci_dev *dev)
> +void pcibios_bus_add_device(struct pci_dev *pdev)
>  {
> -	/*
> -	 * We can only call pcibios_setup_device() after bus setup is complete,
> -	 * since some of the platform specific DMA setup code depends on it.
> -	 */
> -	if (dev->bus->is_added)
> -		pcibios_setup_device(dev);
> +	/* Perform platform-specific device setup */
> +	pcibios_setup_device(pdev);
> +
> +	if (ppc_md.pcibios_bus_add_device)
> +		ppc_md.pcibios_bus_add_device(pdev);
> +}
>  
> +int pcibios_add_device(struct pci_dev *dev)
> +{
>  #ifdef CONFIG_PCI_IOV
>  	if (ppc_md.pcibios_fixup_sriov)
>  		ppc_md.pcibios_fixup_sriov(dev);
> @@ -1037,9 +1033,6 @@ void pcibios_fixup_bus(struct pci_bus *bus)
>  
>  	/* Now fixup the bus bus */
>  	pcibios_setup_bus_self(bus);
> -
> -	/* Now fixup devices on that bus */
> -	pcibios_setup_bus_devices(bus);
>  }
>  EXPORT_SYMBOL(pcibios_fixup_bus);
>  
>
Shawn Anastasio Sept. 27, 2019, 6:33 p.m. UTC | #2
On 9/9/19 2:59 AM, Alexey Kardashevskiy wrote:
> 
> 
> On 06/09/2019 05:13, Shawn Anastasio wrote:
>> Move PCI device setup from pcibios_add_device() and pcibios_fixup_bus() to
>> pcibios_bus_add_device(). This ensures that platform-specific DMA and IOMMU
>> setup occurs after the device has been registered in sysfs, which is a
>> requirement for IOMMU group assignment to work
>>
>> This fixes IOMMU group assignment for hotplugged devices on pseries, where
>> the existing behavior results in IOMMU assignment before registration.
> 
> 
> Although this is a correct approach which we should proceed with, this
> breaks adding of SRIOV VFs from pnv_tce_iommu_bus_notifier (and possibly
> the bare metal PCI hotplug), I am trying to fix that now...

Were you able to make any progress? I can think of a couple of ways
to fix SRIOV, but they're not particularly elegant and involve
duplication.
Alexey Kardashevskiy Sept. 28, 2019, 12:18 p.m. UTC | #3
On 28/09/2019 04:33, Shawn Anastasio wrote:
> On 9/9/19 2:59 AM, Alexey Kardashevskiy wrote:
>>
>>
>> On 06/09/2019 05:13, Shawn Anastasio wrote:
>>> Move PCI device setup from pcibios_add_device() and pcibios_fixup_bus() to
>>> pcibios_bus_add_device(). This ensures that platform-specific DMA and IOMMU
>>> setup occurs after the device has been registered in sysfs, which is a
>>> requirement for IOMMU group assignment to work
>>>
>>> This fixes IOMMU group assignment for hotplugged devices on pseries, where
>>> the existing behavior results in IOMMU assignment before registration.
>>
>>
>> Although this is a correct approach which we should proceed with, this
>> breaks adding of SRIOV VFs from pnv_tce_iommu_bus_notifier (and possibly
>> the bare metal PCI hotplug), I am trying to fix that now...
> 
> Were you able to make any progress? I can think of a couple of ways
> to fix SRIOV, but they're not particularly elegant and involve
> duplication.

A bigger change for ppc+pci is coming from Oliver (I guess with your patch as well) which will tackle this one too, soon.

Patch
diff mbox series

diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index f627e15bb43c..d119c77efb69 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -261,12 +261,6 @@  int pcibios_sriov_disable(struct pci_dev *pdev)
 
 #endif /* CONFIG_PCI_IOV */
 
-void pcibios_bus_add_device(struct pci_dev *pdev)
-{
-	if (ppc_md.pcibios_bus_add_device)
-		ppc_md.pcibios_bus_add_device(pdev);
-}
-
 static resource_size_t pcibios_io_size(const struct pci_controller *hose)
 {
 #ifdef CONFIG_PPC64
@@ -987,15 +981,17 @@  static void pcibios_setup_device(struct pci_dev *dev)
 		ppc_md.pci_irq_fixup(dev);
 }
 
-int pcibios_add_device(struct pci_dev *dev)
+void pcibios_bus_add_device(struct pci_dev *pdev)
 {
-	/*
-	 * We can only call pcibios_setup_device() after bus setup is complete,
-	 * since some of the platform specific DMA setup code depends on it.
-	 */
-	if (dev->bus->is_added)
-		pcibios_setup_device(dev);
+	/* Perform platform-specific device setup */
+	pcibios_setup_device(pdev);
+
+	if (ppc_md.pcibios_bus_add_device)
+		ppc_md.pcibios_bus_add_device(pdev);
+}
 
+int pcibios_add_device(struct pci_dev *dev)
+{
 #ifdef CONFIG_PCI_IOV
 	if (ppc_md.pcibios_fixup_sriov)
 		ppc_md.pcibios_fixup_sriov(dev);
@@ -1037,9 +1033,6 @@  void pcibios_fixup_bus(struct pci_bus *bus)
 
 	/* Now fixup the bus bus */
 	pcibios_setup_bus_self(bus);
-
-	/* Now fixup devices on that bus */
-	pcibios_setup_bus_devices(bus);
 }
 EXPORT_SYMBOL(pcibios_fixup_bus);