[RFC 0/3] ARM: Allow PCI host drivers to be unloaded

[RFC 0/3] ARM: Allow PCI host drivers to be unloaded

[Thierry Reding]

Hi,

This is a set of experimental patches that allow ARM PCI host drivers to
be unloaded. The first two patches enhance the ARM core PCI code with
functions to tear down fixed PCI I/O mappings and unregister a PCI host
bridge. The third patch uses the new functionality to allow the Tegra
PCIe driver to unbind from a device.

I'm sending this as an RFC because, while I've been able to successfully
unbind and rebind the Tegra PCIe driver and verifying that a NIC
connected via PCIe still works after each new probe, I haven't fully
investigated yet whether there may be memory leaks due to missing
cleanup.

Thierry

Thierry Reding (3):
  ARM: Allow unmapping of fixed PCI I/O mappings
  ARM: Introduce pci_common_exit()
  PCI: tegra: Support driver unbinding

 arch/arm/include/asm/io.h       |  1 +
 arch/arm/include/asm/mach/pci.h |  6 ++++++
 arch/arm/kernel/bios32.c        | 29 ++++++++++++++++++++++++++---
 arch/arm/mm/ioremap.c           |  6 ++++++
 drivers/pci/host/pci-tegra.c    | 37 ++++++++++++++++++++++++++++++++++++-
 5 files changed, 75 insertions(+), 4 deletions(-)

-- 
1.8.3.4

[RFC 0/3] ARM: Allow PCI host drivers to be unloaded

[Thierry Reding]

Hi,

This is a set of experimental patches that allow ARM PCI host drivers to
be unloaded. The first two patches enhance the ARM core PCI code with
functions to tear down fixed PCI I/O mappings and unregister a PCI host
bridge. The third patch uses the new functionality to allow the Tegra
PCIe driver to unbind from a device.

I'm sending this as an RFC because, while I've been able to successfully
unbind and rebind the Tegra PCIe driver and verifying that a NIC
connected via PCIe still works after each new probe, I haven't fully
investigated yet whether there may be memory leaks due to missing
cleanup.

Thierry

Thierry Reding (3):
  ARM: Allow unmapping of fixed PCI I/O mappings
  ARM: Introduce pci_common_exit()
  PCI: tegra: Support driver unbinding

 arch/arm/include/asm/io.h       |  1 +
 arch/arm/include/asm/mach/pci.h |  6 ++++++
 arch/arm/kernel/bios32.c        | 29 ++++++++++++++++++++++++++---
 arch/arm/mm/ioremap.c           |  6 ++++++
 drivers/pci/host/pci-tegra.c    | 37 ++++++++++++++++++++++++++++++++++++-
 5 files changed, 75 insertions(+), 4 deletions(-)

-- 
1.8.3.4

[RFC 0/3] ARM: Allow PCI host drivers to be unloaded

[Thierry Reding]

Hi,

This is a set of experimental patches that allow ARM PCI host drivers to
be unloaded. The first two patches enhance the ARM core PCI code with
functions to tear down fixed PCI I/O mappings and unregister a PCI host
bridge. The third patch uses the new functionality to allow the Tegra
PCIe driver to unbind from a device.

I'm sending this as an RFC because, while I've been able to successfully
unbind and rebind the Tegra PCIe driver and verifying that a NIC
connected via PCIe still works after each new probe, I haven't fully
investigated yet whether there may be memory leaks due to missing
cleanup.

Thierry

Thierry Reding (3):
  ARM: Allow unmapping of fixed PCI I/O mappings
  ARM: Introduce pci_common_exit()
  PCI: tegra: Support driver unbinding

 arch/arm/include/asm/io.h       |  1 +
 arch/arm/include/asm/mach/pci.h |  6 ++++++
 arch/arm/kernel/bios32.c        | 29 ++++++++++++++++++++++++++---
 arch/arm/mm/ioremap.c           |  6 ++++++
 drivers/pci/host/pci-tegra.c    | 37 ++++++++++++++++++++++++++++++++++++-
 5 files changed, 75 insertions(+), 4 deletions(-)

-- 
1.8.3.4

[RFC 1/3] ARM: Allow unmapping of fixed PCI I/O mappings

[Thierry Reding]

The new pci_iounmap_io() function can be used to unmap a fixed PCI I/O
mapping as established by pci_ioremap_io(). This will be useful to
support unbinding of PCI host drivers.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 arch/arm/include/asm/io.h | 1 +
 arch/arm/mm/ioremap.c     | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index d070741..80c3826 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -172,6 +172,7 @@ static inline void __iomem *__typesafe_io(unsigned long addr)
 #define PCI_IO_VIRT_BASE	0xfee00000
 
 extern int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr);
+extern void pci_iounmap_io(unsigned int offset);
 
 /*
  * Now, pick up the machine-defined IO definitions
diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index f123d6e..c7504c9 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -448,4 +448,10 @@ int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr)
 				  __pgprot(get_mem_type(MT_DEVICE)->prot_pte));
 }
 EXPORT_SYMBOL_GPL(pci_ioremap_io);
+
+void pci_iounmap_io(unsigned int offset)
+{
+	unmap_kernel_range(PCI_IO_VIRT_BASE + offset, SZ_64K);
+}
+EXPORT_SYMBOL_GPL(pci_iounmap_io);
 #endif
-- 
1.8.3.4

[RFC 2/3] ARM: Introduce pci_common_exit()

[Thierry Reding]

In order to support building PCI host drivers as modules, functionality
is required to undo the steps performed by pci_common_init(). The PCI
core provides much of the functionality already, so add a function that
can be called by drivers to wrap the ARM specific bits.

This patch does a number of things to achieve this: it adds a .nr field
to struct pci_sys_data to keep track of the controller number that was
used to initialize it during pci_common_init(). That field is passed to
the new .teardown() callback during cleanup to undo what .setup() did.

Furthermore the list of pci_sys_data structures setup can optionally be
returned via the hw_pci structure's .sys field. If a driver initializes
it, then it is assumed to be an empty list that pci_common_init() will
append to. Otherwise the old behaviour of keeping a local list only is
preserved.

If a driver wants to support unloading, then it needs access to this
list and pass it to pci_common_exit(). This will iterate over the list,
call the new .teardown() callback and remove the root bus associated
with each entry.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 arch/arm/include/asm/mach/pci.h |  6 ++++++
 arch/arm/kernel/bios32.c        | 29 ++++++++++++++++++++++++++---
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
index 454d642..d6c352a 100644
--- a/arch/arm/include/asm/mach/pci.h
+++ b/arch/arm/include/asm/mach/pci.h
@@ -25,6 +25,7 @@ struct hw_pci {
 	struct pci_ops	*ops;
 	int		nr_controllers;
 	void		**private_data;
+	struct list_head *sys;
 	int		(*setup)(int nr, struct pci_sys_data *);
 	struct pci_bus *(*scan)(int nr, struct pci_sys_data *);
 	void		(*preinit)(void);
@@ -38,6 +39,7 @@ struct hw_pci {
 					  resource_size_t align);
 	void		(*add_bus)(struct pci_bus *bus);
 	void		(*remove_bus)(struct pci_bus *bus);
+	void		(*teardown)(int nr, struct pci_sys_data *);
 };
 
 /*
@@ -49,6 +51,7 @@ struct pci_sys_data {
 #endif
 	struct list_head node;
 	int		busnr;		/* primary bus number			*/
+	int		nr;		/* controller number			*/
 	u64		mem_offset;	/* bus->cpu memory mapping offset	*/
 	unsigned long	io_offset;	/* bus->cpu IO mapping offset		*/
 	struct pci_bus	*bus;		/* PCI bus				*/
@@ -67,6 +70,7 @@ struct pci_sys_data {
 					  resource_size_t align);
 	void		(*add_bus)(struct pci_bus *bus);
 	void		(*remove_bus)(struct pci_bus *bus);
+	void		(*teardown)(int nr, struct pci_sys_data *);
 	void		*private_data;	/* platform controller private data	*/
 };
 
@@ -84,6 +88,8 @@ static inline void pci_common_init(struct hw_pci *hw)
 	pci_common_init_dev(NULL, hw);
 }
 
+void pci_common_exit(struct list_head *head);
+
 /*
  * Setup early fixed I/O mapping.
  */
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 1ec9c87..6895520 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -475,11 +475,13 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
 		sys->domain  = hw->domain;
 #endif
 		sys->busnr   = busnr;
+		sys->nr      = nr;
 		sys->swizzle = hw->swizzle;
 		sys->map_irq = hw->map_irq;
 		sys->align_resource = hw->align_resource;
 		sys->add_bus = hw->add_bus;
 		sys->remove_bus = hw->remove_bus;
+		sys->teardown = hw->teardown;
 		INIT_LIST_HEAD(&sys->resources);
 
 		if (hw->private_data)
@@ -517,18 +519,24 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
 void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
 {
 	struct pci_sys_data *sys;
-	LIST_HEAD(head);
+	struct list_head *head;
+	LIST_HEAD(list);
+
+	if (hw->sys)
+		head = hw->sys;
+	else
+		head = &list;
 
 	pci_add_flags(PCI_REASSIGN_ALL_RSRC);
 	if (hw->preinit)
 		hw->preinit();
-	pcibios_init_hw(parent, hw, &head);
+	pcibios_init_hw(parent, hw, head);
 	if (hw->postinit)
 		hw->postinit();
 
 	pci_fixup_irqs(pcibios_swizzle, pcibios_map_irq);
 
-	list_for_each_entry(sys, &head, node) {
+	list_for_each_entry(sys, head, node) {
 		struct pci_bus *bus = sys->bus;
 
 		if (!pci_has_flag(PCI_PROBE_ONLY)) {
@@ -555,6 +563,21 @@ void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
 	}
 }
 
+void pci_common_exit(struct list_head *head)
+{
+	struct pci_sys_data *sys, *tmp;
+
+	list_for_each_entry_safe(sys, tmp, head, node) {
+		if (sys->teardown)
+			sys->teardown(sys->nr, sys);
+
+		pci_stop_root_bus(sys->bus);
+		pci_remove_root_bus(sys->bus);
+		list_del(&sys->node);
+		kfree(sys);
+	}
+}
+
 #ifndef CONFIG_PCI_HOST_ITE8152
 void pcibios_set_master(struct pci_dev *dev)
 {
-- 
1.8.3.4

[RFC 3/3] PCI: tegra: Support driver unbinding

[Thierry Reding]

Implement the platform driver's .remove() callback to free all resources
allocated during driver setup and call pci_common_exit() to cleanup ARM
specific datastructures. Unmap the fixed PCI I/O mapping by calling the
new pci_iounmap_io() function in the new .teardown() callback.

Finally, no longer set the .suppress_bind_attrs field to true to allow
the driver to unbind from a device.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/pci/host/pci-tegra.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 7356741..396f352 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -250,6 +250,7 @@ struct tegra_pcie {
 	int irq;
 
 	struct list_head busses;
+	struct list_head sys;
 	struct resource *cs;
 
 	struct resource io;
@@ -666,6 +667,11 @@ static struct pci_bus *tegra_pcie_scan_bus(int nr, struct pci_sys_data *sys)
 	return bus;
 }
 
+static void tegra_pcie_teardown(int nr, struct pci_sys_data *sys)
+{
+	pci_iounmap_io(nr * SZ_64K);
+}
+
 static irqreturn_t tegra_pcie_isr(int irq, void *arg)
 {
 	const char *err_msg[] = {
@@ -1583,7 +1589,9 @@ static int tegra_pcie_enable(struct tegra_pcie *pcie)
 	hw.map_irq = tegra_pcie_map_irq;
 	hw.add_bus = tegra_pcie_add_bus;
 	hw.scan = tegra_pcie_scan_bus;
+	hw.teardown = tegra_pcie_teardown;
 	hw.ops = &tegra_pcie_ops;
+	hw.sys = &pcie->sys;
 
 	pci_common_init_dev(pcie->dev, &hw);
 
@@ -1637,6 +1645,7 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 
 	INIT_LIST_HEAD(&pcie->busses);
 	INIT_LIST_HEAD(&pcie->ports);
+	INIT_LIST_HEAD(&pcie->sys);
 	pcie->soc_data = match->data;
 	pcie->dev = &pdev->dev;
 
@@ -1686,14 +1695,40 @@ put_resources:
 	return err;
 }
 
+static int tegra_pcie_remove(struct platform_device *pdev)
+{
+	struct tegra_pcie *pcie = platform_get_drvdata(pdev);
+	struct tegra_pcie_bus *bus, *tmp;
+	int err;
+
+	pci_common_exit(&pcie->sys);
+
+	list_for_each_entry_safe(bus, tmp, &pcie->busses, list) {
+		vunmap(bus->area->addr);
+		kfree(bus);
+	}
+
+	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+		err = tegra_pcie_disable_msi(pcie);
+		if (err < 0)
+			return err;
+	}
+
+	err = tegra_pcie_put_resources(pcie);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
 static struct platform_driver tegra_pcie_driver = {
 	.driver = {
 		.name = "tegra-pcie",
 		.owner = THIS_MODULE,
 		.of_match_table = tegra_pcie_of_match,
-		.suppress_bind_attrs = true,
 	},
 	.probe = tegra_pcie_probe,
+	.remove = tegra_pcie_remove,
 };
 module_platform_driver(tegra_pcie_driver);
 
-- 
1.8.3.4

Re: [RFC 3/3] PCI: tegra: Support driver unbinding

[Stephen Warren]

On 08/13/2013 05:12 AM, Thierry Reding wrote:
> Implement the platform driver's .remove() callback to free all resources
> allocated during driver setup and call pci_common_exit() to cleanup ARM
> specific datastructures. Unmap the fixed PCI I/O mapping by calling the
> new pci_iounmap_io() function in the new .teardown() callback.
> 
> Finally, no longer set the .suppress_bind_attrs field to true to allow
> the driver to unbind from a device.

> +static int tegra_pcie_remove(struct platform_device *pdev)
> +{
> +	struct tegra_pcie *pcie = platform_get_drvdata(pdev);
> +	struct tegra_pcie_bus *bus, *tmp;
> +	int err;
> +
> +	pci_common_exit(&pcie->sys);
> +
> +	list_for_each_entry_safe(bus, tmp, &pcie->busses, list) {
> +		vunmap(bus->area->addr);
> +		kfree(bus);
> +	}
> +
> +	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> +		err = tegra_pcie_disable_msi(pcie);
> +		if (err < 0)
> +			return err;
> +	}

Wouldn't it make sense to do that as early as possible in the function,
to make sure that no MSI accidentally fires after some of the cleanup
has already happened?

> +
> +	err = tegra_pcie_put_resources(pcie);
> +	if (err < 0)
> +		return err;
> +
> +	return 0;
> +}

Re: [RFC 3/3] PCI: tegra: Support driver unbinding

[Stephen Warren]

On 08/13/2013 05:12 AM, Thierry Reding wrote:
> Implement the platform driver's .remove() callback to free all resources
> allocated during driver setup and call pci_common_exit() to cleanup ARM
> specific datastructures. Unmap the fixed PCI I/O mapping by calling the
> new pci_iounmap_io() function in the new .teardown() callback.
> 
> Finally, no longer set the .suppress_bind_attrs field to true to allow
> the driver to unbind from a device.

> +static int tegra_pcie_remove(struct platform_device *pdev)
> +{
> +	struct tegra_pcie *pcie = platform_get_drvdata(pdev);
> +	struct tegra_pcie_bus *bus, *tmp;
> +	int err;
> +
> +	pci_common_exit(&pcie->sys);
> +
> +	list_for_each_entry_safe(bus, tmp, &pcie->busses, list) {
> +		vunmap(bus->area->addr);
> +		kfree(bus);
> +	}
> +
> +	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> +		err = tegra_pcie_disable_msi(pcie);
> +		if (err < 0)
> +			return err;
> +	}

Wouldn't it make sense to do that as early as possible in the function,
to make sure that no MSI accidentally fires after some of the cleanup
has already happened?

> +
> +	err = tegra_pcie_put_resources(pcie);
> +	if (err < 0)
> +		return err;
> +
> +	return 0;
> +}

[RFC 3/3] PCI: tegra: Support driver unbinding

[Stephen Warren]

On 08/13/2013 05:12 AM, Thierry Reding wrote:
> Implement the platform driver's .remove() callback to free all resources
> allocated during driver setup and call pci_common_exit() to cleanup ARM
> specific datastructures. Unmap the fixed PCI I/O mapping by calling the
> new pci_iounmap_io() function in the new .teardown() callback.
> 
> Finally, no longer set the .suppress_bind_attrs field to true to allow
> the driver to unbind from a device.

> +static int tegra_pcie_remove(struct platform_device *pdev)
> +{
> +	struct tegra_pcie *pcie = platform_get_drvdata(pdev);
> +	struct tegra_pcie_bus *bus, *tmp;
> +	int err;
> +
> +	pci_common_exit(&pcie->sys);
> +
> +	list_for_each_entry_safe(bus, tmp, &pcie->busses, list) {
> +		vunmap(bus->area->addr);
> +		kfree(bus);
> +	}
> +
> +	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> +		err = tegra_pcie_disable_msi(pcie);
> +		if (err < 0)
> +			return err;
> +	}

Wouldn't it make sense to do that as early as possible in the function,
to make sure that no MSI accidentally fires after some of the cleanup
has already happened?

> +
> +	err = tegra_pcie_put_resources(pcie);
> +	if (err < 0)
> +		return err;
> +
> +	return 0;
> +}

Re: [RFC 3/3] PCI: tegra: Support driver unbinding

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1600 bytes --]

On Wed, Aug 14, 2013 at 03:43:40PM -0600, Stephen Warren wrote:
> On 08/13/2013 05:12 AM, Thierry Reding wrote:
> > Implement the platform driver's .remove() callback to free all resources
> > allocated during driver setup and call pci_common_exit() to cleanup ARM
> > specific datastructures. Unmap the fixed PCI I/O mapping by calling the
> > new pci_iounmap_io() function in the new .teardown() callback.
> > 
> > Finally, no longer set the .suppress_bind_attrs field to true to allow
> > the driver to unbind from a device.
> 
> > +static int tegra_pcie_remove(struct platform_device *pdev)
> > +{
> > +	struct tegra_pcie *pcie = platform_get_drvdata(pdev);
> > +	struct tegra_pcie_bus *bus, *tmp;
> > +	int err;
> > +
> > +	pci_common_exit(&pcie->sys);
> > +
> > +	list_for_each_entry_safe(bus, tmp, &pcie->busses, list) {
> > +		vunmap(bus->area->addr);
> > +		kfree(bus);
> > +	}
> > +
> > +	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > +		err = tegra_pcie_disable_msi(pcie);
> > +		if (err < 0)
> > +			return err;
> > +	}
> 
> Wouldn't it make sense to do that as early as possible in the function,
> to make sure that no MSI accidentally fires after some of the cleanup
> has already happened?

I don't think that's strictly necessary in this case. After the call to
pci_common_exit() there are no PCI devices left, there's not even a bus
left. All MSI users should have cleaned up after themselves.

Given that I thought it more useful to mirror the setup done in .probe()
to make it clearer what's being undone (and potentially what's missing).

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[RFC 3/3] PCI: tegra: Support driver unbinding

[Thierry Reding]

On Wed, Aug 14, 2013 at 03:43:40PM -0600, Stephen Warren wrote:
> On 08/13/2013 05:12 AM, Thierry Reding wrote:
> > Implement the platform driver's .remove() callback to free all resources
> > allocated during driver setup and call pci_common_exit() to cleanup ARM
> > specific datastructures. Unmap the fixed PCI I/O mapping by calling the
> > new pci_iounmap_io() function in the new .teardown() callback.
> > 
> > Finally, no longer set the .suppress_bind_attrs field to true to allow
> > the driver to unbind from a device.
> 
> > +static int tegra_pcie_remove(struct platform_device *pdev)
> > +{
> > +	struct tegra_pcie *pcie = platform_get_drvdata(pdev);
> > +	struct tegra_pcie_bus *bus, *tmp;
> > +	int err;
> > +
> > +	pci_common_exit(&pcie->sys);
> > +
> > +	list_for_each_entry_safe(bus, tmp, &pcie->busses, list) {
> > +		vunmap(bus->area->addr);
> > +		kfree(bus);
> > +	}
> > +
> > +	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > +		err = tegra_pcie_disable_msi(pcie);
> > +		if (err < 0)
> > +			return err;
> > +	}
> 
> Wouldn't it make sense to do that as early as possible in the function,
> to make sure that no MSI accidentally fires after some of the cleanup
> has already happened?

I don't think that's strictly necessary in this case. After the call to
pci_common_exit() there are no PCI devices left, there's not even a bus
left. All MSI users should have cleaned up after themselves.

Given that I thought it more useful to mirror the setup done in .probe()
to make it clearer what's being undone (and potentially what's missing).

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130815/91dc8e38/attachment-0001.sig>

Re: [RFC 3/3] PCI: tegra: Support driver unbinding

[Stephen Warren]

On 08/15/2013 04:34 AM, Thierry Reding wrote:
> On Wed, Aug 14, 2013 at 03:43:40PM -0600, Stephen Warren wrote:
>> On 08/13/2013 05:12 AM, Thierry Reding wrote:
>>> Implement the platform driver's .remove() callback to free all
>>> resources allocated during driver setup and call
>>> pci_common_exit() to cleanup ARM specific datastructures. Unmap
>>> the fixed PCI I/O mapping by calling the new pci_iounmap_io()
>>> function in the new .teardown() callback.
>>> 
>>> Finally, no longer set the .suppress_bind_attrs field to true
>>> to allow the driver to unbind from a device.
>> 
>>> +static int tegra_pcie_remove(struct platform_device *pdev) +{ 
>>> +	struct tegra_pcie *pcie = platform_get_drvdata(pdev); +
>>> struct tegra_pcie_bus *bus, *tmp; +	int err; + +
>>> pci_common_exit(&pcie->sys); + +	list_for_each_entry_safe(bus,
>>> tmp, &pcie->busses, list) { +		vunmap(bus->area->addr); +
>>> kfree(bus); +	} + +	if (IS_ENABLED(CONFIG_PCI_MSI)) { +		err =
>>> tegra_pcie_disable_msi(pcie); +		if (err < 0) +			return err; +
>>> }
>> 
>> Wouldn't it make sense to do that as early as possible in the
>> function, to make sure that no MSI accidentally fires after some
>> of the cleanup has already happened?
> 
> I don't think that's strictly necessary in this case. After the
> call to pci_common_exit() there are no PCI devices left, there's
> not even a bus left. All MSI users should have cleaned up after
> themselves.
> 
> Given that I thought it more useful to mirror the setup done in
> .probe() to make it clearer what's being undone (and potentially
> what's missing).

That makes sense SW-wise, but what about mis-behaving HW that triggers
an MSI even when it's been told not to? I assume that
tegra_pcie_disable_msi() unrequests the IRQ, hence solves that
problem, if done early enough.

[RFC 3/3] PCI: tegra: Support driver unbinding

[Stephen Warren]

On 08/15/2013 04:34 AM, Thierry Reding wrote:
> On Wed, Aug 14, 2013 at 03:43:40PM -0600, Stephen Warren wrote:
>> On 08/13/2013 05:12 AM, Thierry Reding wrote:
>>> Implement the platform driver's .remove() callback to free all
>>> resources allocated during driver setup and call
>>> pci_common_exit() to cleanup ARM specific datastructures. Unmap
>>> the fixed PCI I/O mapping by calling the new pci_iounmap_io()
>>> function in the new .teardown() callback.
>>> 
>>> Finally, no longer set the .suppress_bind_attrs field to true
>>> to allow the driver to unbind from a device.
>> 
>>> +static int tegra_pcie_remove(struct platform_device *pdev) +{ 
>>> +	struct tegra_pcie *pcie = platform_get_drvdata(pdev); +
>>> struct tegra_pcie_bus *bus, *tmp; +	int err; + +
>>> pci_common_exit(&pcie->sys); + +	list_for_each_entry_safe(bus,
>>> tmp, &pcie->busses, list) { +		vunmap(bus->area->addr); +
>>> kfree(bus); +	} + +	if (IS_ENABLED(CONFIG_PCI_MSI)) { +		err =
>>> tegra_pcie_disable_msi(pcie); +		if (err < 0) +			return err; +
>>> }
>> 
>> Wouldn't it make sense to do that as early as possible in the
>> function, to make sure that no MSI accidentally fires after some
>> of the cleanup has already happened?
> 
> I don't think that's strictly necessary in this case. After the
> call to pci_common_exit() there are no PCI devices left, there's
> not even a bus left. All MSI users should have cleaned up after
> themselves.
> 
> Given that I thought it more useful to mirror the setup done in
> .probe() to make it clearer what's being undone (and potentially
> what's missing).

That makes sense SW-wise, but what about mis-behaving HW that triggers
an MSI even when it's been told not to? I assume that
tegra_pcie_disable_msi() unrequests the IRQ, hence solves that
problem, if done early enough.

Re: [RFC 3/3] PCI: tegra: Support driver unbinding

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2416 bytes --]

On Thu, Aug 15, 2013 at 09:21:53AM -0600, Stephen Warren wrote:
> On 08/15/2013 04:34 AM, Thierry Reding wrote:
> > On Wed, Aug 14, 2013 at 03:43:40PM -0600, Stephen Warren wrote:
> >> On 08/13/2013 05:12 AM, Thierry Reding wrote:
> >>> Implement the platform driver's .remove() callback to free all
> >>> resources allocated during driver setup and call
> >>> pci_common_exit() to cleanup ARM specific datastructures. Unmap
> >>> the fixed PCI I/O mapping by calling the new pci_iounmap_io()
> >>> function in the new .teardown() callback.
> >>> 
> >>> Finally, no longer set the .suppress_bind_attrs field to true
> >>> to allow the driver to unbind from a device.
> >> 
> >>> +static int tegra_pcie_remove(struct platform_device *pdev) +{ 
> >>> +	struct tegra_pcie *pcie = platform_get_drvdata(pdev); +
> >>> struct tegra_pcie_bus *bus, *tmp; +	int err; + +
> >>> pci_common_exit(&pcie->sys); + +	list_for_each_entry_safe(bus,
> >>> tmp, &pcie->busses, list) { +		vunmap(bus->area->addr); +
> >>> kfree(bus); +	} + +	if (IS_ENABLED(CONFIG_PCI_MSI)) { +		err =
> >>> tegra_pcie_disable_msi(pcie); +		if (err < 0) +			return err; +
> >>> }
> >> 
> >> Wouldn't it make sense to do that as early as possible in the
> >> function, to make sure that no MSI accidentally fires after some
> >> of the cleanup has already happened?
> > 
> > I don't think that's strictly necessary in this case. After the
> > call to pci_common_exit() there are no PCI devices left, there's
> > not even a bus left. All MSI users should have cleaned up after
> > themselves.
> > 
> > Given that I thought it more useful to mirror the setup done in
> > .probe() to make it clearer what's being undone (and potentially
> > what's missing).
> 
> That makes sense SW-wise, but what about mis-behaving HW that triggers
> an MSI even when it's been told not to? I assume that
> tegra_pcie_disable_msi() unrequests the IRQ, hence solves that
> problem, if done early enough.

To be honest, I'm not sure about the side-effects that this will have.
tegra_pcie_disable_msi() does quite a bit more than just masking the
interrupts. It also completely removes the IRQ domain that provides the
MSI interrupts. While I haven't tried it yet I can imagine that it will
cause crashes at a later point when drivers want to disable MSI on a
device and the IRQ domain having vanished from underneath.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[RFC 3/3] PCI: tegra: Support driver unbinding

[Thierry Reding]

On Thu, Aug 15, 2013 at 09:21:53AM -0600, Stephen Warren wrote:
> On 08/15/2013 04:34 AM, Thierry Reding wrote:
> > On Wed, Aug 14, 2013 at 03:43:40PM -0600, Stephen Warren wrote:
> >> On 08/13/2013 05:12 AM, Thierry Reding wrote:
> >>> Implement the platform driver's .remove() callback to free all
> >>> resources allocated during driver setup and call
> >>> pci_common_exit() to cleanup ARM specific datastructures. Unmap
> >>> the fixed PCI I/O mapping by calling the new pci_iounmap_io()
> >>> function in the new .teardown() callback.
> >>> 
> >>> Finally, no longer set the .suppress_bind_attrs field to true
> >>> to allow the driver to unbind from a device.
> >> 
> >>> +static int tegra_pcie_remove(struct platform_device *pdev) +{ 
> >>> +	struct tegra_pcie *pcie = platform_get_drvdata(pdev); +
> >>> struct tegra_pcie_bus *bus, *tmp; +	int err; + +
> >>> pci_common_exit(&pcie->sys); + +	list_for_each_entry_safe(bus,
> >>> tmp, &pcie->busses, list) { +		vunmap(bus->area->addr); +
> >>> kfree(bus); +	} + +	if (IS_ENABLED(CONFIG_PCI_MSI)) { +		err =
> >>> tegra_pcie_disable_msi(pcie); +		if (err < 0) +			return err; +
> >>> }
> >> 
> >> Wouldn't it make sense to do that as early as possible in the
> >> function, to make sure that no MSI accidentally fires after some
> >> of the cleanup has already happened?
> > 
> > I don't think that's strictly necessary in this case. After the
> > call to pci_common_exit() there are no PCI devices left, there's
> > not even a bus left. All MSI users should have cleaned up after
> > themselves.
> > 
> > Given that I thought it more useful to mirror the setup done in
> > .probe() to make it clearer what's being undone (and potentially
> > what's missing).
> 
> That makes sense SW-wise, but what about mis-behaving HW that triggers
> an MSI even when it's been told not to? I assume that
> tegra_pcie_disable_msi() unrequests the IRQ, hence solves that
> problem, if done early enough.

To be honest, I'm not sure about the side-effects that this will have.
tegra_pcie_disable_msi() does quite a bit more than just masking the
interrupts. It also completely removes the IRQ domain that provides the
MSI interrupts. While I haven't tried it yet I can imagine that it will
cause crashes at a later point when drivers want to disable MSI on a
device and the IRQ domain having vanished from underneath.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130819/672164dc/attachment.sig>

Re: [RFC 3/3] PCI: tegra: Support driver unbinding

[Stephen Warren]

On 08/19/2013 02:16 PM, Thierry Reding wrote:
> On Thu, Aug 15, 2013 at 09:21:53AM -0600, Stephen Warren wrote:
>> On 08/15/2013 04:34 AM, Thierry Reding wrote:
>>> On Wed, Aug 14, 2013 at 03:43:40PM -0600, Stephen Warren
>>> wrote:
>>>> On 08/13/2013 05:12 AM, Thierry Reding wrote:
>>>>> Implement the platform driver's .remove() callback to free
>>>>> all resources allocated during driver setup and call 
>>>>> pci_common_exit() to cleanup ARM specific datastructures.
>>>>> Unmap the fixed PCI I/O mapping by calling the new
>>>>> pci_iounmap_io() function in the new .teardown() callback.
>>>>> 
>>>>> Finally, no longer set the .suppress_bind_attrs field to
>>>>> true to allow the driver to unbind from a device.
>>>> 
>>>>> +static int tegra_pcie_remove(struct platform_device *pdev)
>>>>> +{ +	struct tegra_pcie *pcie = platform_get_drvdata(pdev);
>>>>> + struct tegra_pcie_bus *bus, *tmp; +	int err; + + 
>>>>> pci_common_exit(&pcie->sys); + +
>>>>> list_for_each_entry_safe(bus, tmp, &pcie->busses, list) { +
>>>>> vunmap(bus->area->addr); + kfree(bus); +	} + +	if
>>>>> (IS_ENABLED(CONFIG_PCI_MSI)) { +		err = 
>>>>> tegra_pcie_disable_msi(pcie); +		if (err < 0) +			return
>>>>> err; + }
>>>> 
>>>> Wouldn't it make sense to do that as early as possible in
>>>> the function, to make sure that no MSI accidentally fires
>>>> after some of the cleanup has already happened?
>>> 
>>> I don't think that's strictly necessary in this case. After
>>> the call to pci_common_exit() there are no PCI devices left,
>>> there's not even a bus left. All MSI users should have cleaned
>>> up after themselves.
>>> 
>>> Given that I thought it more useful to mirror the setup done
>>> in .probe() to make it clearer what's being undone (and
>>> potentially what's missing).
>> 
>> That makes sense SW-wise, but what about mis-behaving HW that
>> triggers an MSI even when it's been told not to? I assume that 
>> tegra_pcie_disable_msi() unrequests the IRQ, hence solves that 
>> problem, if done early enough.
> 
> To be honest, I'm not sure about the side-effects that this will
> have. tegra_pcie_disable_msi() does quite a bit more than just
> masking the interrupts. It also completely removes the IRQ domain
> that provides the MSI interrupts. While I haven't tried it yet I
> can imagine that it will cause crashes at a later point when
> drivers want to disable MSI on a device and the IRQ domain having
> vanished from underneath.

Surely by the time the PCIe controller device has been remove()d then
all devices for PCIe "client" devices have also been removed.

But I guess the problem is if the controller is added back, yet the
IRQ resources aren't re-parsed under the new IRQ domain? Still, that
seems like an unrelated issue to exactly where the MSI IRQ domain gets
cleaned up in the host controller's remove().

Re: [RFC 3/3] PCI: tegra: Support driver unbinding

[Stephen Warren]

On 08/19/2013 02:16 PM, Thierry Reding wrote:
> On Thu, Aug 15, 2013 at 09:21:53AM -0600, Stephen Warren wrote:
>> On 08/15/2013 04:34 AM, Thierry Reding wrote:
>>> On Wed, Aug 14, 2013 at 03:43:40PM -0600, Stephen Warren
>>> wrote:
>>>> On 08/13/2013 05:12 AM, Thierry Reding wrote:
>>>>> Implement the platform driver's .remove() callback to free
>>>>> all resources allocated during driver setup and call 
>>>>> pci_common_exit() to cleanup ARM specific datastructures.
>>>>> Unmap the fixed PCI I/O mapping by calling the new
>>>>> pci_iounmap_io() function in the new .teardown() callback.
>>>>> 
>>>>> Finally, no longer set the .suppress_bind_attrs field to
>>>>> true to allow the driver to unbind from a device.
>>>> 
>>>>> +static int tegra_pcie_remove(struct platform_device *pdev)
>>>>> +{ +	struct tegra_pcie *pcie = platform_get_drvdata(pdev);
>>>>> + struct tegra_pcie_bus *bus, *tmp; +	int err; + + 
>>>>> pci_common_exit(&pcie->sys); + +
>>>>> list_for_each_entry_safe(bus, tmp, &pcie->busses, list) { +
>>>>> vunmap(bus->area->addr); + kfree(bus); +	} + +	if
>>>>> (IS_ENABLED(CONFIG_PCI_MSI)) { +		err = 
>>>>> tegra_pcie_disable_msi(pcie); +		if (err < 0) +			return
>>>>> err; + }
>>>> 
>>>> Wouldn't it make sense to do that as early as possible in
>>>> the function, to make sure that no MSI accidentally fires
>>>> after some of the cleanup has already happened?
>>> 
>>> I don't think that's strictly necessary in this case. After
>>> the call to pci_common_exit() there are no PCI devices left,
>>> there's not even a bus left. All MSI users should have cleaned
>>> up after themselves.
>>> 
>>> Given that I thought it more useful to mirror the setup done
>>> in .probe() to make it clearer what's being undone (and
>>> potentially what's missing).
>> 
>> That makes sense SW-wise, but what about mis-behaving HW that
>> triggers an MSI even when it's been told not to? I assume that 
>> tegra_pcie_disable_msi() unrequests the IRQ, hence solves that 
>> problem, if done early enough.
> 
> To be honest, I'm not sure about the side-effects that this will
> have. tegra_pcie_disable_msi() does quite a bit more than just
> masking the interrupts. It also completely removes the IRQ domain
> that provides the MSI interrupts. While I haven't tried it yet I
> can imagine that it will cause crashes at a later point when
> drivers want to disable MSI on a device and the IRQ domain having
> vanished from underneath.

Surely by the time the PCIe controller device has been remove()d then
all devices for PCIe "client" devices have also been removed.

But I guess the problem is if the controller is added back, yet the
IRQ resources aren't re-parsed under the new IRQ domain? Still, that
seems like an unrelated issue to exactly where the MSI IRQ domain gets
cleaned up in the host controller's remove().

[RFC 3/3] PCI: tegra: Support driver unbinding

[Stephen Warren]

On 08/19/2013 02:16 PM, Thierry Reding wrote:
> On Thu, Aug 15, 2013 at 09:21:53AM -0600, Stephen Warren wrote:
>> On 08/15/2013 04:34 AM, Thierry Reding wrote:
>>> On Wed, Aug 14, 2013 at 03:43:40PM -0600, Stephen Warren
>>> wrote:
>>>> On 08/13/2013 05:12 AM, Thierry Reding wrote:
>>>>> Implement the platform driver's .remove() callback to free
>>>>> all resources allocated during driver setup and call 
>>>>> pci_common_exit() to cleanup ARM specific datastructures.
>>>>> Unmap the fixed PCI I/O mapping by calling the new
>>>>> pci_iounmap_io() function in the new .teardown() callback.
>>>>> 
>>>>> Finally, no longer set the .suppress_bind_attrs field to
>>>>> true to allow the driver to unbind from a device.
>>>> 
>>>>> +static int tegra_pcie_remove(struct platform_device *pdev)
>>>>> +{ +	struct tegra_pcie *pcie = platform_get_drvdata(pdev);
>>>>> + struct tegra_pcie_bus *bus, *tmp; +	int err; + + 
>>>>> pci_common_exit(&pcie->sys); + +
>>>>> list_for_each_entry_safe(bus, tmp, &pcie->busses, list) { +
>>>>> vunmap(bus->area->addr); + kfree(bus); +	} + +	if
>>>>> (IS_ENABLED(CONFIG_PCI_MSI)) { +		err = 
>>>>> tegra_pcie_disable_msi(pcie); +		if (err < 0) +			return
>>>>> err; + }
>>>> 
>>>> Wouldn't it make sense to do that as early as possible in
>>>> the function, to make sure that no MSI accidentally fires
>>>> after some of the cleanup has already happened?
>>> 
>>> I don't think that's strictly necessary in this case. After
>>> the call to pci_common_exit() there are no PCI devices left,
>>> there's not even a bus left. All MSI users should have cleaned
>>> up after themselves.
>>> 
>>> Given that I thought it more useful to mirror the setup done
>>> in .probe() to make it clearer what's being undone (and
>>> potentially what's missing).
>> 
>> That makes sense SW-wise, but what about mis-behaving HW that
>> triggers an MSI even when it's been told not to? I assume that 
>> tegra_pcie_disable_msi() unrequests the IRQ, hence solves that 
>> problem, if done early enough.
> 
> To be honest, I'm not sure about the side-effects that this will
> have. tegra_pcie_disable_msi() does quite a bit more than just
> masking the interrupts. It also completely removes the IRQ domain
> that provides the MSI interrupts. While I haven't tried it yet I
> can imagine that it will cause crashes at a later point when
> drivers want to disable MSI on a device and the IRQ domain having
> vanished from underneath.

Surely by the time the PCIe controller device has been remove()d then
all devices for PCIe "client" devices have also been removed.

But I guess the problem is if the controller is added back, yet the
IRQ resources aren't re-parsed under the new IRQ domain? Still, that
seems like an unrelated issue to exactly where the MSI IRQ domain gets
cleaned up in the host controller's remove().

Re: [RFC 3/3] PCI: tegra: Support driver unbinding

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3758 bytes --]

On Mon, Aug 19, 2013 at 02:55:44PM -0600, Stephen Warren wrote:
> On 08/19/2013 02:16 PM, Thierry Reding wrote:
> > On Thu, Aug 15, 2013 at 09:21:53AM -0600, Stephen Warren wrote:
> >> On 08/15/2013 04:34 AM, Thierry Reding wrote:
> >>> On Wed, Aug 14, 2013 at 03:43:40PM -0600, Stephen Warren
> >>> wrote:
> >>>> On 08/13/2013 05:12 AM, Thierry Reding wrote:
> >>>>> Implement the platform driver's .remove() callback to free
> >>>>> all resources allocated during driver setup and call 
> >>>>> pci_common_exit() to cleanup ARM specific datastructures.
> >>>>> Unmap the fixed PCI I/O mapping by calling the new
> >>>>> pci_iounmap_io() function in the new .teardown() callback.
> >>>>> 
> >>>>> Finally, no longer set the .suppress_bind_attrs field to
> >>>>> true to allow the driver to unbind from a device.
> >>>> 
> >>>>> +static int tegra_pcie_remove(struct platform_device *pdev)
> >>>>> +{ +	struct tegra_pcie *pcie = platform_get_drvdata(pdev);
> >>>>> + struct tegra_pcie_bus *bus, *tmp; +	int err; + + 
> >>>>> pci_common_exit(&pcie->sys); + +
> >>>>> list_for_each_entry_safe(bus, tmp, &pcie->busses, list) { +
> >>>>> vunmap(bus->area->addr); + kfree(bus); +	} + +	if
> >>>>> (IS_ENABLED(CONFIG_PCI_MSI)) { +		err = 
> >>>>> tegra_pcie_disable_msi(pcie); +		if (err < 0) +			return
> >>>>> err; + }
> >>>> 
> >>>> Wouldn't it make sense to do that as early as possible in
> >>>> the function, to make sure that no MSI accidentally fires
> >>>> after some of the cleanup has already happened?
> >>> 
> >>> I don't think that's strictly necessary in this case. After
> >>> the call to pci_common_exit() there are no PCI devices left,
> >>> there's not even a bus left. All MSI users should have cleaned
> >>> up after themselves.
> >>> 
> >>> Given that I thought it more useful to mirror the setup done
> >>> in .probe() to make it clearer what's being undone (and
> >>> potentially what's missing).
> >> 
> >> That makes sense SW-wise, but what about mis-behaving HW that
> >> triggers an MSI even when it's been told not to? I assume that 
> >> tegra_pcie_disable_msi() unrequests the IRQ, hence solves that 
> >> problem, if done early enough.
> > 
> > To be honest, I'm not sure about the side-effects that this will
> > have. tegra_pcie_disable_msi() does quite a bit more than just
> > masking the interrupts. It also completely removes the IRQ domain
> > that provides the MSI interrupts. While I haven't tried it yet I
> > can imagine that it will cause crashes at a later point when
> > drivers want to disable MSI on a device and the IRQ domain having
> > vanished from underneath.
> 
> Surely by the time the PCIe controller device has been remove()d then
> all devices for PCIe "client" devices have also been removed.

The PCIe controller is the device being removed. Part of that removal
involves stopping and removing all PCI devices. That's done as part of
pci_common_exit().

But I was under the impression that you were arguing that the call to
tegra_pcie_disable_msi() should be the first call in .remove() in order
to prevent any spurious MSIs from occurring. That in turn would mean
calling tegra_pcie_disable_msi() before pci_common_exit(), and that
would lead to the problem that I described.

> But I guess the problem is if the controller is added back, yet the
> IRQ resources aren't re-parsed under the new IRQ domain? Still, that
> seems like an unrelated issue to exactly where the MSI IRQ domain gets
> cleaned up in the host controller's remove().

I don't think that should be a problem. Given that both the MSI IRQ
domain and the PCI devices will be setup from scratch I don't see how
any stale resources could mess things up.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC 3/3] PCI: tegra: Support driver unbinding

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3758 bytes --]

On Mon, Aug 19, 2013 at 02:55:44PM -0600, Stephen Warren wrote:
> On 08/19/2013 02:16 PM, Thierry Reding wrote:
> > On Thu, Aug 15, 2013 at 09:21:53AM -0600, Stephen Warren wrote:
> >> On 08/15/2013 04:34 AM, Thierry Reding wrote:
> >>> On Wed, Aug 14, 2013 at 03:43:40PM -0600, Stephen Warren
> >>> wrote:
> >>>> On 08/13/2013 05:12 AM, Thierry Reding wrote:
> >>>>> Implement the platform driver's .remove() callback to free
> >>>>> all resources allocated during driver setup and call 
> >>>>> pci_common_exit() to cleanup ARM specific datastructures.
> >>>>> Unmap the fixed PCI I/O mapping by calling the new
> >>>>> pci_iounmap_io() function in the new .teardown() callback.
> >>>>> 
> >>>>> Finally, no longer set the .suppress_bind_attrs field to
> >>>>> true to allow the driver to unbind from a device.
> >>>> 
> >>>>> +static int tegra_pcie_remove(struct platform_device *pdev)
> >>>>> +{ +	struct tegra_pcie *pcie = platform_get_drvdata(pdev);
> >>>>> + struct tegra_pcie_bus *bus, *tmp; +	int err; + + 
> >>>>> pci_common_exit(&pcie->sys); + +
> >>>>> list_for_each_entry_safe(bus, tmp, &pcie->busses, list) { +
> >>>>> vunmap(bus->area->addr); + kfree(bus); +	} + +	if
> >>>>> (IS_ENABLED(CONFIG_PCI_MSI)) { +		err = 
> >>>>> tegra_pcie_disable_msi(pcie); +		if (err < 0) +			return
> >>>>> err; + }
> >>>> 
> >>>> Wouldn't it make sense to do that as early as possible in
> >>>> the function, to make sure that no MSI accidentally fires
> >>>> after some of the cleanup has already happened?
> >>> 
> >>> I don't think that's strictly necessary in this case. After
> >>> the call to pci_common_exit() there are no PCI devices left,
> >>> there's not even a bus left. All MSI users should have cleaned
> >>> up after themselves.
> >>> 
> >>> Given that I thought it more useful to mirror the setup done
> >>> in .probe() to make it clearer what's being undone (and
> >>> potentially what's missing).
> >> 
> >> That makes sense SW-wise, but what about mis-behaving HW that
> >> triggers an MSI even when it's been told not to? I assume that 
> >> tegra_pcie_disable_msi() unrequests the IRQ, hence solves that 
> >> problem, if done early enough.
> > 
> > To be honest, I'm not sure about the side-effects that this will
> > have. tegra_pcie_disable_msi() does quite a bit more than just
> > masking the interrupts. It also completely removes the IRQ domain
> > that provides the MSI interrupts. While I haven't tried it yet I
> > can imagine that it will cause crashes at a later point when
> > drivers want to disable MSI on a device and the IRQ domain having
> > vanished from underneath.
> 
> Surely by the time the PCIe controller device has been remove()d then
> all devices for PCIe "client" devices have also been removed.

The PCIe controller is the device being removed. Part of that removal
involves stopping and removing all PCI devices. That's done as part of
pci_common_exit().

But I was under the impression that you were arguing that the call to
tegra_pcie_disable_msi() should be the first call in .remove() in order
to prevent any spurious MSIs from occurring. That in turn would mean
calling tegra_pcie_disable_msi() before pci_common_exit(), and that
would lead to the problem that I described.

> But I guess the problem is if the controller is added back, yet the
> IRQ resources aren't re-parsed under the new IRQ domain? Still, that
> seems like an unrelated issue to exactly where the MSI IRQ domain gets
> cleaned up in the host controller's remove().

I don't think that should be a problem. Given that both the MSI IRQ
domain and the PCI devices will be setup from scratch I don't see how
any stale resources could mess things up.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[RFC 3/3] PCI: tegra: Support driver unbinding

[Thierry Reding]

On Mon, Aug 19, 2013 at 02:55:44PM -0600, Stephen Warren wrote:
> On 08/19/2013 02:16 PM, Thierry Reding wrote:
> > On Thu, Aug 15, 2013 at 09:21:53AM -0600, Stephen Warren wrote:
> >> On 08/15/2013 04:34 AM, Thierry Reding wrote:
> >>> On Wed, Aug 14, 2013 at 03:43:40PM -0600, Stephen Warren
> >>> wrote:
> >>>> On 08/13/2013 05:12 AM, Thierry Reding wrote:
> >>>>> Implement the platform driver's .remove() callback to free
> >>>>> all resources allocated during driver setup and call 
> >>>>> pci_common_exit() to cleanup ARM specific datastructures.
> >>>>> Unmap the fixed PCI I/O mapping by calling the new
> >>>>> pci_iounmap_io() function in the new .teardown() callback.
> >>>>> 
> >>>>> Finally, no longer set the .suppress_bind_attrs field to
> >>>>> true to allow the driver to unbind from a device.
> >>>> 
> >>>>> +static int tegra_pcie_remove(struct platform_device *pdev)
> >>>>> +{ +	struct tegra_pcie *pcie = platform_get_drvdata(pdev);
> >>>>> + struct tegra_pcie_bus *bus, *tmp; +	int err; + + 
> >>>>> pci_common_exit(&pcie->sys); + +
> >>>>> list_for_each_entry_safe(bus, tmp, &pcie->busses, list) { +
> >>>>> vunmap(bus->area->addr); + kfree(bus); +	} + +	if
> >>>>> (IS_ENABLED(CONFIG_PCI_MSI)) { +		err = 
> >>>>> tegra_pcie_disable_msi(pcie); +		if (err < 0) +			return
> >>>>> err; + }
> >>>> 
> >>>> Wouldn't it make sense to do that as early as possible in
> >>>> the function, to make sure that no MSI accidentally fires
> >>>> after some of the cleanup has already happened?
> >>> 
> >>> I don't think that's strictly necessary in this case. After
> >>> the call to pci_common_exit() there are no PCI devices left,
> >>> there's not even a bus left. All MSI users should have cleaned
> >>> up after themselves.
> >>> 
> >>> Given that I thought it more useful to mirror the setup done
> >>> in .probe() to make it clearer what's being undone (and
> >>> potentially what's missing).
> >> 
> >> That makes sense SW-wise, but what about mis-behaving HW that
> >> triggers an MSI even when it's been told not to? I assume that 
> >> tegra_pcie_disable_msi() unrequests the IRQ, hence solves that 
> >> problem, if done early enough.
> > 
> > To be honest, I'm not sure about the side-effects that this will
> > have. tegra_pcie_disable_msi() does quite a bit more than just
> > masking the interrupts. It also completely removes the IRQ domain
> > that provides the MSI interrupts. While I haven't tried it yet I
> > can imagine that it will cause crashes at a later point when
> > drivers want to disable MSI on a device and the IRQ domain having
> > vanished from underneath.
> 
> Surely by the time the PCIe controller device has been remove()d then
> all devices for PCIe "client" devices have also been removed.

The PCIe controller is the device being removed. Part of that removal
involves stopping and removing all PCI devices. That's done as part of
pci_common_exit().

But I was under the impression that you were arguing that the call to
tegra_pcie_disable_msi() should be the first call in .remove() in order
to prevent any spurious MSIs from occurring. That in turn would mean
calling tegra_pcie_disable_msi() before pci_common_exit(), and that
would lead to the problem that I described.

> But I guess the problem is if the controller is added back, yet the
> IRQ resources aren't re-parsed under the new IRQ domain? Still, that
> seems like an unrelated issue to exactly where the MSI IRQ domain gets
> cleaned up in the host controller's remove().

I don't think that should be a problem. Given that both the MSI IRQ
domain and the PCI devices will be setup from scratch I don't see how
any stale resources could mess things up.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130819/6609a805/attachment.sig>

Re: [RFC 3/3] PCI: tegra: Support driver unbinding

[Stephen Warren]

On 08/19/2013 03:52 PM, Thierry Reding wrote:
> On Mon, Aug 19, 2013 at 02:55:44PM -0600, Stephen Warren wrote:
>> On 08/19/2013 02:16 PM, Thierry Reding wrote:
>>> On Thu, Aug 15, 2013 at 09:21:53AM -0600, Stephen Warren
>>> wrote:
>>>> On 08/15/2013 04:34 AM, Thierry Reding wrote:
>>>>> On Wed, Aug 14, 2013 at 03:43:40PM -0600, Stephen Warren 
>>>>> wrote:
>>>>>> On 08/13/2013 05:12 AM, Thierry Reding wrote:
>>>>>>> Implement the platform driver's .remove() callback to
>>>>>>> free all resources allocated during driver setup and
>>>>>>> call pci_common_exit() to cleanup ARM specific
>>>>>>> datastructures. Unmap the fixed PCI I/O mapping by
>>>>>>> calling the new pci_iounmap_io() function in the new
>>>>>>> .teardown() callback.
>>>>>>> 
>>>>>>> Finally, no longer set the .suppress_bind_attrs field
>>>>>>> to true to allow the driver to unbind from a device.
>>>>>> 
>>>>>>> +static int tegra_pcie_remove(struct platform_device
>>>>>>> *pdev) +{ +	struct tegra_pcie *pcie =
>>>>>>> platform_get_drvdata(pdev); + struct tegra_pcie_bus
>>>>>>> *bus, *tmp; +	int err; + + pci_common_exit(&pcie->sys);
>>>>>>> + + list_for_each_entry_safe(bus, tmp, &pcie->busses,
>>>>>>> list) { + vunmap(bus->area->addr); + kfree(bus); +	} +
>>>>>>> +	if (IS_ENABLED(CONFIG_PCI_MSI)) { +		err = 
>>>>>>> tegra_pcie_disable_msi(pcie); +		if (err < 0) +
>>>>>>> return err; + }
>>>>>> 
>>>>>> Wouldn't it make sense to do that as early as possible
>>>>>> in the function, to make sure that no MSI accidentally
>>>>>> fires after some of the cleanup has already happened?
>>>>> 
>>>>> I don't think that's strictly necessary in this case.
>>>>> After the call to pci_common_exit() there are no PCI
>>>>> devices left, there's not even a bus left. All MSI users
>>>>> should have cleaned up after themselves.
>>>>> 
>>>>> Given that I thought it more useful to mirror the setup
>>>>> done in .probe() to make it clearer what's being undone
>>>>> (and potentially what's missing).
>>>> 
>>>> That makes sense SW-wise, but what about mis-behaving HW
>>>> that triggers an MSI even when it's been told not to? I
>>>> assume that tegra_pcie_disable_msi() unrequests the IRQ,
>>>> hence solves that problem, if done early enough.
>>> 
>>> To be honest, I'm not sure about the side-effects that this
>>> will have. tegra_pcie_disable_msi() does quite a bit more than
>>> just masking the interrupts. It also completely removes the IRQ
>>> domain that provides the MSI interrupts. While I haven't tried
>>> it yet I can imagine that it will cause crashes at a later
>>> point when drivers want to disable MSI on a device and the IRQ
>>> domain having vanished from underneath.
>> 
>> Surely by the time the PCIe controller device has been remove()d
>> then all devices for PCIe "client" devices have also been
>> removed.
> 
> The PCIe controller is the device being removed. Part of that
> removal involves stopping and removing all PCI devices. That's done
> as part of pci_common_exit().
> 
> But I was under the impression that you were arguing that the call
> to tegra_pcie_disable_msi() should be the first call in .remove()
> in order to prevent any spurious MSIs from occurring. That in turn
> would mean calling tegra_pcie_disable_msi() before
> pci_common_exit(), and that would lead to the problem that I
> described.

Earlier yes, but perhaps not first. Right now the order is:

1) pci_common_exit

2) unmap some memory, free some buses

3) tear down MSI infra-structure

I suppose if the MSI IRQ handler is guaranteed to never touch the
stuff torn down by (2) then the code is fine as-is, but it might be
clearer to do the following instead:

1) pci_common_exit

2) tear down MSI infra-structure (and indeed unregister non-MSI IRQ too)

3) unmap some memory, free some buses

... since then no matter what, no IRQ can be handled before any
resource is torn down.

also, perhaps when initially responding I missed that
pci_common_exit() is what forcibly removed all PCIe "client" device
drivers; in other sub-systems, client devices take refcounts on their
resources, so the resource can't be .remove()d until all the objects
referencing them have been manually removed, but PCIe apparently works
the other way around - removing the controller removes all the users?

[RFC 3/3] PCI: tegra: Support driver unbinding

[Stephen Warren]

On 08/19/2013 03:52 PM, Thierry Reding wrote:
> On Mon, Aug 19, 2013 at 02:55:44PM -0600, Stephen Warren wrote:
>> On 08/19/2013 02:16 PM, Thierry Reding wrote:
>>> On Thu, Aug 15, 2013 at 09:21:53AM -0600, Stephen Warren
>>> wrote:
>>>> On 08/15/2013 04:34 AM, Thierry Reding wrote:
>>>>> On Wed, Aug 14, 2013 at 03:43:40PM -0600, Stephen Warren 
>>>>> wrote:
>>>>>> On 08/13/2013 05:12 AM, Thierry Reding wrote:
>>>>>>> Implement the platform driver's .remove() callback to
>>>>>>> free all resources allocated during driver setup and
>>>>>>> call pci_common_exit() to cleanup ARM specific
>>>>>>> datastructures. Unmap the fixed PCI I/O mapping by
>>>>>>> calling the new pci_iounmap_io() function in the new
>>>>>>> .teardown() callback.
>>>>>>> 
>>>>>>> Finally, no longer set the .suppress_bind_attrs field
>>>>>>> to true to allow the driver to unbind from a device.
>>>>>> 
>>>>>>> +static int tegra_pcie_remove(struct platform_device
>>>>>>> *pdev) +{ +	struct tegra_pcie *pcie =
>>>>>>> platform_get_drvdata(pdev); + struct tegra_pcie_bus
>>>>>>> *bus, *tmp; +	int err; + + pci_common_exit(&pcie->sys);
>>>>>>> + + list_for_each_entry_safe(bus, tmp, &pcie->busses,
>>>>>>> list) { + vunmap(bus->area->addr); + kfree(bus); +	} +
>>>>>>> +	if (IS_ENABLED(CONFIG_PCI_MSI)) { +		err = 
>>>>>>> tegra_pcie_disable_msi(pcie); +		if (err < 0) +
>>>>>>> return err; + }
>>>>>> 
>>>>>> Wouldn't it make sense to do that as early as possible
>>>>>> in the function, to make sure that no MSI accidentally
>>>>>> fires after some of the cleanup has already happened?
>>>>> 
>>>>> I don't think that's strictly necessary in this case.
>>>>> After the call to pci_common_exit() there are no PCI
>>>>> devices left, there's not even a bus left. All MSI users
>>>>> should have cleaned up after themselves.
>>>>> 
>>>>> Given that I thought it more useful to mirror the setup
>>>>> done in .probe() to make it clearer what's being undone
>>>>> (and potentially what's missing).
>>>> 
>>>> That makes sense SW-wise, but what about mis-behaving HW
>>>> that triggers an MSI even when it's been told not to? I
>>>> assume that tegra_pcie_disable_msi() unrequests the IRQ,
>>>> hence solves that problem, if done early enough.
>>> 
>>> To be honest, I'm not sure about the side-effects that this
>>> will have. tegra_pcie_disable_msi() does quite a bit more than
>>> just masking the interrupts. It also completely removes the IRQ
>>> domain that provides the MSI interrupts. While I haven't tried
>>> it yet I can imagine that it will cause crashes at a later
>>> point when drivers want to disable MSI on a device and the IRQ
>>> domain having vanished from underneath.
>> 
>> Surely by the time the PCIe controller device has been remove()d
>> then all devices for PCIe "client" devices have also been
>> removed.
> 
> The PCIe controller is the device being removed. Part of that
> removal involves stopping and removing all PCI devices. That's done
> as part of pci_common_exit().
> 
> But I was under the impression that you were arguing that the call
> to tegra_pcie_disable_msi() should be the first call in .remove()
> in order to prevent any spurious MSIs from occurring. That in turn
> would mean calling tegra_pcie_disable_msi() before
> pci_common_exit(), and that would lead to the problem that I
> described.

Earlier yes, but perhaps not first. Right now the order is:

1) pci_common_exit

2) unmap some memory, free some buses

3) tear down MSI infra-structure

I suppose if the MSI IRQ handler is guaranteed to never touch the
stuff torn down by (2) then the code is fine as-is, but it might be
clearer to do the following instead:

1) pci_common_exit

2) tear down MSI infra-structure (and indeed unregister non-MSI IRQ too)

3) unmap some memory, free some buses

... since then no matter what, no IRQ can be handled before any
resource is torn down.

also, perhaps when initially responding I missed that
pci_common_exit() is what forcibly removed all PCIe "client" device
drivers; in other sub-systems, client devices take refcounts on their
resources, so the resource can't be .remove()d until all the objects
referencing them have been manually removed, but PCIe apparently works
the other way around - removing the controller removes all the users?