[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
> 

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
> 

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
> 

[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