[PATCH 1/2] PCI/VMD: Detach resources after stopping root bus

[PATCH 1/2] PCI/VMD: Detach resources after stopping root bus

[Jon Derrick]

Fixes the ugly warning:
[  181.940162] Trying to free nonexistent resource <e5a10000-e5a13fff>

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 drivers/pci/controller/vmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index fd2dbd7..46ed80f 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -813,12 +813,12 @@ static void vmd_remove(struct pci_dev *dev)
 {
 	struct vmd_dev *vmd = pci_get_drvdata(dev);
 
-	vmd_detach_resources(vmd);
 	sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
 	pci_stop_root_bus(vmd->bus);
 	pci_remove_root_bus(vmd->bus);
 	vmd_cleanup_srcu(vmd);
 	vmd_teardown_dma_ops(vmd);
+	vmd_detach_resources(vmd);
 	irq_domain_remove(vmd->irq_domain);
 }
 
-- 
1.8.3.1

[PATCH 2/2] PCI/VMD: Expose VMD host-bridge

[Jon Derrick]

In preparation for kernel host-bridge enhancements, and to take
advantage of pci_host_probe()'s calling of
pcie_bus_configure_settings(), convert the VMD driver to expose a host
bridge rather than a root bus.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 drivers/pci/controller/vmd.c | 54 ++++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 20 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 46ed80f..ca05679 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -93,7 +93,7 @@ struct vmd_dev {
 	struct pci_sysdata	sysdata;
 	struct resource		resources[3];
 	struct irq_domain	*irq_domain;
-	struct pci_bus		*bus;
+	struct pci_host_bridge	*host;
 
 #ifdef CONFIG_X86_DEV_DMA_OPS
 	struct dma_map_ops	dma_ops;
@@ -582,7 +582,6 @@ static int vmd_find_free_domain(void)
 static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 {
 	struct pci_sysdata *sd = &vmd->sysdata;
-	struct fwnode_handle *fn;
 	struct resource *res;
 	u32 upper_bits;
 	unsigned long flags;
@@ -690,37 +689,51 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 		return sd->domain;
 
 	sd->node = pcibus_to_node(vmd->dev->bus);
-
-	fn = irq_domain_alloc_named_id_fwnode("VMD-MSI", vmd->sysdata.domain);
-	if (!fn)
+	sd->fwnode = irq_domain_alloc_named_id_fwnode("VMD-MSI",
+						      vmd->sysdata.domain);
+	if (!sd->fwnode)
 		return -ENODEV;
 
-	vmd->irq_domain = pci_msi_create_irq_domain(fn, &vmd_msi_domain_info,
+	vmd->irq_domain = pci_msi_create_irq_domain(sd->fwnode,
+						    &vmd_msi_domain_info,
 						    x86_vector_domain);
-	irq_domain_free_fwnode(fn);
 	if (!vmd->irq_domain)
-		return -ENODEV;
+		goto free_fwnode;
 
+	vmd->irq_domain->fwnode = sd->fwnode;
 	pci_add_resource(&resources, &vmd->resources[0]);
 	pci_add_resource_offset(&resources, &vmd->resources[1], offset[0]);
 	pci_add_resource_offset(&resources, &vmd->resources[2], offset[1]);
 
-	vmd->bus = pci_create_root_bus(&vmd->dev->dev, busn_start, &vmd_ops,
-				       sd, &resources);
-	if (!vmd->bus) {
-		pci_free_resource_list(&resources);
-		irq_domain_remove(vmd->irq_domain);
-		return -ENODEV;
-	}
+	vmd->host = devm_pci_alloc_host_bridge(&vmd->dev->dev, 0);
+	if (!vmd->host)
+		goto free_irqdomain;
+
+	list_splice_init(&resources, &vmd->host->windows);
+	vmd->host->busnr = busn_start;
+	vmd->host->dev.parent = &vmd->dev->dev;
+	vmd->host->ops = &vmd_ops;
+	vmd->host->sysdata = sd;
 
 	vmd_attach_resources(vmd);
 	vmd_setup_dma_ops(vmd);
-	dev_set_msi_domain(&vmd->bus->dev, vmd->irq_domain);
-	pci_rescan_bus(vmd->bus);
+	dev_set_msi_domain(&vmd->host->dev, vmd->irq_domain);
+	if (pci_host_probe(vmd->host))
+		goto detach_resources;
 
-	WARN(sysfs_create_link(&vmd->dev->dev.kobj, &vmd->bus->dev.kobj,
+	WARN(sysfs_create_link(&vmd->dev->dev.kobj, &vmd->host->bus->dev.kobj,
 			       "domain"), "Can't create symlink to domain\n");
 	return 0;
+
+detach_resources:
+	vmd_detach_resources(vmd);
+free_fwnode:
+	irq_domain_free_fwnode(vmd->irq_domain->fwnode);
+free_irqdomain:
+	pci_free_resource_list(&resources);
+	irq_domain_remove(vmd->irq_domain);
+
+	return -ENODEV;
 }
 
 static irqreturn_t vmd_irq(int irq, void *data)
@@ -814,11 +827,12 @@ static void vmd_remove(struct pci_dev *dev)
 	struct vmd_dev *vmd = pci_get_drvdata(dev);
 
 	sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
-	pci_stop_root_bus(vmd->bus);
-	pci_remove_root_bus(vmd->bus);
+	pci_stop_root_bus(vmd->host->bus);
+	pci_remove_root_bus(vmd->host->bus);
 	vmd_cleanup_srcu(vmd);
 	vmd_teardown_dma_ops(vmd);
 	vmd_detach_resources(vmd);
+	irq_domain_free_fwnode(vmd->irq_domain->fwnode);
 	irq_domain_remove(vmd->irq_domain);
 }
 
-- 
1.8.3.1

Re: [PATCH 2/2] PCI/VMD: Expose VMD host-bridge

[Lorenzo Pieralisi]

On Tue, Sep 04, 2018 at 07:09:51PM -0600, Jon Derrick wrote:
> In preparation for kernel host-bridge enhancements, and to take
> advantage of pci_host_probe()'s calling of
> pcie_bus_configure_settings(), convert the VMD driver to expose a host
> bridge rather than a root bus.
> 
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
>  drivers/pci/controller/vmd.c | 54 ++++++++++++++++++++++++++++----------------
>  1 file changed, 34 insertions(+), 20 deletions(-)

Hi Jon,

sorry for the pedantry but I think the right way to introduce this
change is first to add a call:

pcie_bus_configure_settings()

in one patch (that I think you need since that's the main reason
you convert code to pci_host_probe() API, correct ?) and then refactor
the code to expose the host bridge in a subsequent patch.

We should not mix refactoring and adding new features in a single patch.

Thanks,
Lorenzo

> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 46ed80f..ca05679 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -93,7 +93,7 @@ struct vmd_dev {
>  	struct pci_sysdata	sysdata;
>  	struct resource		resources[3];
>  	struct irq_domain	*irq_domain;
> -	struct pci_bus		*bus;
> +	struct pci_host_bridge	*host;
>  
>  #ifdef CONFIG_X86_DEV_DMA_OPS
>  	struct dma_map_ops	dma_ops;
> @@ -582,7 +582,6 @@ static int vmd_find_free_domain(void)
>  static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  {
>  	struct pci_sysdata *sd = &vmd->sysdata;
> -	struct fwnode_handle *fn;
>  	struct resource *res;
>  	u32 upper_bits;
>  	unsigned long flags;
> @@ -690,37 +689,51 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  		return sd->domain;
>  
>  	sd->node = pcibus_to_node(vmd->dev->bus);
> -
> -	fn = irq_domain_alloc_named_id_fwnode("VMD-MSI", vmd->sysdata.domain);
> -	if (!fn)
> +	sd->fwnode = irq_domain_alloc_named_id_fwnode("VMD-MSI",
> +						      vmd->sysdata.domain);
> +	if (!sd->fwnode)
>  		return -ENODEV;
>  
> -	vmd->irq_domain = pci_msi_create_irq_domain(fn, &vmd_msi_domain_info,
> +	vmd->irq_domain = pci_msi_create_irq_domain(sd->fwnode,
> +						    &vmd_msi_domain_info,
>  						    x86_vector_domain);
> -	irq_domain_free_fwnode(fn);
>  	if (!vmd->irq_domain)
> -		return -ENODEV;
> +		goto free_fwnode;
>  
> +	vmd->irq_domain->fwnode = sd->fwnode;
>  	pci_add_resource(&resources, &vmd->resources[0]);
>  	pci_add_resource_offset(&resources, &vmd->resources[1], offset[0]);
>  	pci_add_resource_offset(&resources, &vmd->resources[2], offset[1]);
>  
> -	vmd->bus = pci_create_root_bus(&vmd->dev->dev, busn_start, &vmd_ops,
> -				       sd, &resources);
> -	if (!vmd->bus) {
> -		pci_free_resource_list(&resources);
> -		irq_domain_remove(vmd->irq_domain);
> -		return -ENODEV;
> -	}
> +	vmd->host = devm_pci_alloc_host_bridge(&vmd->dev->dev, 0);
> +	if (!vmd->host)
> +		goto free_irqdomain;
> +
> +	list_splice_init(&resources, &vmd->host->windows);
> +	vmd->host->busnr = busn_start;
> +	vmd->host->dev.parent = &vmd->dev->dev;
> +	vmd->host->ops = &vmd_ops;
> +	vmd->host->sysdata = sd;
>  
>  	vmd_attach_resources(vmd);
>  	vmd_setup_dma_ops(vmd);
> -	dev_set_msi_domain(&vmd->bus->dev, vmd->irq_domain);
> -	pci_rescan_bus(vmd->bus);
> +	dev_set_msi_domain(&vmd->host->dev, vmd->irq_domain);
> +	if (pci_host_probe(vmd->host))
> +		goto detach_resources;
>  
> -	WARN(sysfs_create_link(&vmd->dev->dev.kobj, &vmd->bus->dev.kobj,
> +	WARN(sysfs_create_link(&vmd->dev->dev.kobj, &vmd->host->bus->dev.kobj,
>  			       "domain"), "Can't create symlink to domain\n");
>  	return 0;
> +
> +detach_resources:
> +	vmd_detach_resources(vmd);
> +free_fwnode:
> +	irq_domain_free_fwnode(vmd->irq_domain->fwnode);
> +free_irqdomain:
> +	pci_free_resource_list(&resources);
> +	irq_domain_remove(vmd->irq_domain);
> +
> +	return -ENODEV;
>  }
>  
>  static irqreturn_t vmd_irq(int irq, void *data)
> @@ -814,11 +827,12 @@ static void vmd_remove(struct pci_dev *dev)
>  	struct vmd_dev *vmd = pci_get_drvdata(dev);
>  
>  	sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
> -	pci_stop_root_bus(vmd->bus);
> -	pci_remove_root_bus(vmd->bus);
> +	pci_stop_root_bus(vmd->host->bus);
> +	pci_remove_root_bus(vmd->host->bus);
>  	vmd_cleanup_srcu(vmd);
>  	vmd_teardown_dma_ops(vmd);
>  	vmd_detach_resources(vmd);
> +	irq_domain_free_fwnode(vmd->irq_domain->fwnode);
>  	irq_domain_remove(vmd->irq_domain);
>  }
>  
> -- 
> 1.8.3.1
> 

Re: [PATCH 1/2] PCI/VMD: Detach resources after stopping root bus

[Lorenzo Pieralisi]

On Tue, Sep 04, 2018 at 07:09:50PM -0600, Jon Derrick wrote:
> Fixes the ugly warning:
> [  181.940162] Trying to free nonexistent resource <e5a10000-e5a13fff>

It would be good to explain why that warning triggers and why this
patch fixes it.

Lorenzo

> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
>  drivers/pci/controller/vmd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index fd2dbd7..46ed80f 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -813,12 +813,12 @@ static void vmd_remove(struct pci_dev *dev)
>  {
>  	struct vmd_dev *vmd = pci_get_drvdata(dev);
>  
> -	vmd_detach_resources(vmd);
>  	sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
>  	pci_stop_root_bus(vmd->bus);
>  	pci_remove_root_bus(vmd->bus);
>  	vmd_cleanup_srcu(vmd);
>  	vmd_teardown_dma_ops(vmd);
> +	vmd_detach_resources(vmd);
>  	irq_domain_remove(vmd->irq_domain);
>  }
>  
> -- 
> 1.8.3.1
> 

[PATCH 0/2] VMD fixes for 4.20

[Jon Derrick]

Hello Lorenzo, Keith, Bjorn

These are VMD fixes for 4.20
First one looks to fix an orphaned struct resource issue when removing the VMD
module


Second patch enables firmware-first error handling on a VMD domain.

Because the endpoint is somewhat divorced from the VMD domain, the driver
doesn't use a standard ACPI method of signaling firmware-first on the domain.
The idea behind firmware-first in VMD is that the endpoint will advertise its
support using the interface bit in the endpoint. The driver enables SMI system
error messages in the root port control register. The firmware is allowed to
handle the interrupt as it sees fit, but returns control to the VMD driver by
synthesizing an MSI message. The kernel native AER driver may be used for
further processing, so is left enabled. If the firmware doesn't need the native
driver, of course it can handle and clear all errors and the subsequent MSI
will be eventually dropped as it goes through the VMD ISR.

At this time, I'm unaware of how the errors will be represented in the error
status headers. In the past they could be seen as a combination of the VMD
endpoint device's bus number and the erroring device's bus number, but could be
different depending on the type of error and if the erroring endpoint was lost
(completion timeout). Currently the native driver handles these errors by
scanning every device on the bus for error status and there's no plan to change
that mechanism for future VMD devices.

Follow-up to:
https://patchwork.ozlabs.org/patch/966128/
https://patchwork.ozlabs.org/patch/964254/

Jon Derrick (2):
  PCI/VMD: Detach resources after stopping root bus
  PCI/VMD: Set up firmware-first if capable

 drivers/pci/controller/vmd.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[PATCH 1/2] PCI/VMD: Detach resources after stopping root bus

[Jon Derrick]

The VMD removal path calls pci_stop_root_bus, which tears down the pcie
tree, including detaching all of the attached drivers. During driver
detachment, devices may use pci_release_region to release resources.
This path relies on the resource being accessible in resource tree.

By detaching the child domain from the parent resource domain prior to
stopping the bus, we are preventing the list traversal from finding the
resource to be freed. If we instead detach the resource after stopping
the bus, we will have properly freed the resource and detaching is
simply accounting at that point.

Without this order, the resource is never freed and is orphaned on VMD
removal, leading to warning:
[  181.940162] Trying to free nonexistent resource <e5a10000-e5a13fff>

Fixes 2c2c5c5cd213aea38c850bb6edc9b7f77f29802f:
"x86/PCI: VMD: Attach VMD resources to parent domain's resource tree"
Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 drivers/pci/controller/vmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index fd2dbd7..46ed80f 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -813,12 +813,12 @@ static void vmd_remove(struct pci_dev *dev)
 {
 	struct vmd_dev *vmd = pci_get_drvdata(dev);
 
-	vmd_detach_resources(vmd);
 	sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
 	pci_stop_root_bus(vmd->bus);
 	pci_remove_root_bus(vmd->bus);
 	vmd_cleanup_srcu(vmd);
 	vmd_teardown_dma_ops(vmd);
+	vmd_detach_resources(vmd);
 	irq_domain_remove(vmd->irq_domain);
 }
 
-- 
1.8.3.1

Re: [PATCH 1/2] PCI/VMD: Detach resources after stopping root bus

[Keith Busch]

On Mon, Oct 15, 2018 at 05:48:07PM -0700, Jon Derrick wrote:
> The VMD removal path calls pci_stop_root_bus, which tears down the pcie
> tree, including detaching all of the attached drivers. During driver
> detachment, devices may use pci_release_region to release resources.
> This path relies on the resource being accessible in resource tree.
> 
> By detaching the child domain from the parent resource domain prior to
> stopping the bus, we are preventing the list traversal from finding the
> resource to be freed. If we instead detach the resource after stopping
> the bus, we will have properly freed the resource and detaching is
> simply accounting at that point.
> 
> Without this order, the resource is never freed and is orphaned on VMD
> removal, leading to warning:
> [  181.940162] Trying to free nonexistent resource <e5a10000-e5a13fff>
> 
> Fixes 2c2c5c5cd213aea38c850bb6edc9b7f77f29802f:
> "x86/PCI: VMD: Attach VMD resources to parent domain's resource tree"
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>

Looks good.

Reviewed-by: Keith Busch <keith.busch@intel.com>

> ---
>  drivers/pci/controller/vmd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index fd2dbd7..46ed80f 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -813,12 +813,12 @@ static void vmd_remove(struct pci_dev *dev)
>  {
>  	struct vmd_dev *vmd = pci_get_drvdata(dev);
>  
> -	vmd_detach_resources(vmd);
>  	sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
>  	pci_stop_root_bus(vmd->bus);
>  	pci_remove_root_bus(vmd->bus);
>  	vmd_cleanup_srcu(vmd);
>  	vmd_teardown_dma_ops(vmd);
> +	vmd_detach_resources(vmd);
>  	irq_domain_remove(vmd->irq_domain);
>  }
>  
> -- 
> 1.8.3.1
> 

Re: [PATCH 1/2] PCI/VMD: Detach resources after stopping root bus

[Lorenzo Pieralisi]

On Mon, Oct 15, 2018 at 06:48:07PM -0600, Jon Derrick wrote:
> The VMD removal path calls pci_stop_root_bus, which tears down the pcie
> tree, including detaching all of the attached drivers. During driver
> detachment, devices may use pci_release_region to release resources.
> This path relies on the resource being accessible in resource tree.
> 
> By detaching the child domain from the parent resource domain prior to
> stopping the bus, we are preventing the list traversal from finding the
> resource to be freed. If we instead detach the resource after stopping
> the bus, we will have properly freed the resource and detaching is
> simply accounting at that point.
> 
> Without this order, the resource is never freed and is orphaned on VMD
> removal, leading to warning:
> [  181.940162] Trying to free nonexistent resource <e5a10000-e5a13fff>
> 
> Fixes 2c2c5c5cd213aea38c850bb6edc9b7f77f29802f:
> "x86/PCI: VMD: Attach VMD resources to parent domain's resource tree"
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
>  drivers/pci/controller/vmd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I have applied this patch to pci/vmd for v4.20, thanks.

Lorenzo

> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index fd2dbd7..46ed80f 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -813,12 +813,12 @@ static void vmd_remove(struct pci_dev *dev)
>  {
>  	struct vmd_dev *vmd = pci_get_drvdata(dev);
>  
> -	vmd_detach_resources(vmd);
>  	sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
>  	pci_stop_root_bus(vmd->bus);
>  	pci_remove_root_bus(vmd->bus);
>  	vmd_cleanup_srcu(vmd);
>  	vmd_teardown_dma_ops(vmd);
> +	vmd_detach_resources(vmd);
>  	irq_domain_remove(vmd->irq_domain);
>  }
>  
> -- 
> 1.8.3.1
>