Re: [PATCH] ppc/iommu: prevent false TCE leak message
On Fri, Dec 28, 2012 at 01:21:35PM +0800, Gavin Shan wrote:
On Thu, Dec 27, 2012 at 02:28:06PM -0200, Thadeu Lima de Souza Cascardo wrote:
When a device DMA window includes the address 0, it's reserved in the
TCE bitmap to avoid returning that address to drivers.
When the device is removed, the bitmap is checked for any mappings not
removed by the driver, indicating a possible DMA mapping leak. Since the
reserved address is not cleared, a message is printed, warning of such a
leak.
Check for the reservation, and clear it before checking for any other
standing mappings.
Signed-off-by: Thadeu Lima de Souza Cascardo casca...@linux.vnet.ibm.com
---
arch/powerpc/kernel/iommu.c |5 +
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index 8226c6c..226e9e5 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -717,6 +717,11 @@ void iommu_free_table(struct iommu_table *tbl, const
char *node_name)
return;
}
+/* In case we have reserved the first bit, we should not emit
+ * the warning below. */
At least, the comment would look like:
/*
* xxx
*/
Sure. New code should always follow coding style. :-)
How do you suggest merging with the comment below? I think it's closer
to the code it comments about, so I'd rather keep it where it is.
+if (tbl-it_offset == 0)
+clear_bit(0, tbl-it_map);
+
/* verify that table contains no entries */
/* it_size is in entries, and we're examining 64 at a time */
The comment would be merged as well? :-)
I also considered replacing this code by this:
/* verify that table contains no entries */
- /* it_size is in entries, and we're examining 64 at a time */
- for (i = 0; i (tbl-it_size/64); i++) {
- if (tbl-it_map[i] != 0) {
+ if (find_first_bit(tbl-it_map, tbl-it_size) tbl-it_size)
printk(KERN_WARNING %s: Unexpected TCEs for
%s\n,
__func__, node_name);
- break;
- }
- }
I'll resend the unreserving patch with the fixed comment styling, but
without merging comments. And I will send this other patch for comments.
Regards.
Thadeu Cascardo.
for (i = 0; i (tbl-it_size/64); i++) {
Thanks,
Gavin
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 1/2] ppc/iommu: prevent false TCE leak message
When a device DMA window includes the address 0, it's reserved in the
TCE bitmap to avoid returning that address to drivers.
When the device is removed, the bitmap is checked for any mappings not
removed by the driver, indicating a possible DMA mapping leak. Since the
reserved address is not cleared, a message is printed, warning of such a
leak.
Check for the reservation, and clear it before checking for any other
standing mappings.
Signed-off-by: Thadeu Lima de Souza Cascardo casca...@linux.vnet.ibm.com
---
arch/powerpc/kernel/iommu.c |7 +++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index 8226c6c..6d48ff8 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -717,6 +717,13 @@ void iommu_free_table(struct iommu_table *tbl, const char
*node_name)
return;
}
+ /*
+* In case we have reserved the first bit, we should not emit
+* the warning below.
+*/
+ if (tbl-it_offset == 0)
+ clear_bit(0, tbl-it_map);
+
/* verify that table contains no entries */
/* it_size is in entries, and we're examining 64 at a time */
for (i = 0; i (tbl-it_size/64); i++) {
--
1.7.1
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 2/2] ppc/iommu: use find_first_bit to look up entries in the iommu table
Signed-off-by: Thadeu Lima de Souza Cascardo casca...@linux.vnet.ibm.com
---
arch/powerpc/kernel/iommu.c |7 +--
1 files changed, 1 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index 6d48ff8..91e2b99 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -725,14 +725,9 @@ void iommu_free_table(struct iommu_table *tbl, const char
*node_name)
clear_bit(0, tbl-it_map);
/* verify that table contains no entries */
- /* it_size is in entries, and we're examining 64 at a time */
- for (i = 0; i (tbl-it_size/64); i++) {
- if (tbl-it_map[i] != 0) {
+ if (find_first_bit(tbl-it_map, tbl-it_size) tbl-it_size)
printk(KERN_WARNING %s: Unexpected TCEs for %s\n,
__func__, node_name);
- break;
- }
- }
/* calculate bitmap size in bytes */
bitmap_sz = (tbl-it_size + 7) / 8;
--
1.7.1
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 1/2] EEH/OF: checking for CONFIG_EEH is not needed
The functions used are already defined as empty inline functions for the case where EEH is disabled. Signed-off-by: Thadeu Lima de Souza Cascardo casca...@linux.vnet.ibm.com --- arch/powerpc/kernel/of_platform.c |4 1 files changed, 0 insertions(+), 4 deletions(-) diff --git a/arch/powerpc/kernel/of_platform.c b/arch/powerpc/kernel/of_platform.c index 2049f2d..c5fc6b2 100644 --- a/arch/powerpc/kernel/of_platform.c +++ b/arch/powerpc/kernel/of_platform.c @@ -71,10 +71,8 @@ static int __devinit of_pci_phb_probe(struct platform_device *dev) eeh_dev_phb_init_dynamic(phb); /* Register devices with EEH */ -#ifdef CONFIG_EEH if (dev-dev.of_node-child) eeh_add_device_tree_early(dev-dev.of_node); -#endif /* CONFIG_EEH */ /* Scan the bus */ pcibios_scan_phb(phb); @@ -88,9 +86,7 @@ static int __devinit of_pci_phb_probe(struct platform_device *dev) pcibios_claim_one_bus(phb-bus); /* Finish EEH setup */ -#ifdef CONFIG_EEH eeh_add_device_tree_late(phb-bus); -#endif /* Add probed PCI devices to the device model */ pci_bus_add_devices(phb-bus); -- 1.7.1 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 2/2] ppc/EEH: fix crash when adding a device in a slot with DDW
The DDW code uses a eeh_dev struct from the pci_dev. However, this is
not set until eeh_add_device_late is called.
Since pci_bus_add_devices is called before eeh_add_device_late, the PCI
devices are added to the bus, making drivers' probe hooks to be called.
These will call set_dma_mask, which will call the DDW code, which will
require the eeh_dev struct from pci_dev. This would result in a crash,
due to a NULL dereference.
Calling eeh_add_device_late after pci_bus_add_devices would make the
system BUG, because device files shouldn't be added to devices there
were not added to the system. So, a new function is needed to add such
files only after pci_bus_add_devices have been called.
Cc: sta...@vger.kernel.org
Signed-off-by: Thadeu Lima de Souza Cascardo casca...@linux.vnet.ibm.com
---
arch/powerpc/include/asm/eeh.h |3 +++
arch/powerpc/kernel/of_platform.c|3 +++
arch/powerpc/kernel/pci-common.c |7 +--
arch/powerpc/platforms/pseries/eeh.c | 24 +++-
4 files changed, 34 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index b0ef738..0f816da 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -201,6 +201,7 @@ int eeh_dev_check_failure(struct eeh_dev *edev);
void __init eeh_addr_cache_build(void);
void eeh_add_device_tree_early(struct device_node *);
void eeh_add_device_tree_late(struct pci_bus *);
+void eeh_add_sysfs_files(struct pci_bus *);
void eeh_remove_bus_device(struct pci_dev *, int);
/**
@@ -240,6 +241,8 @@ static inline void eeh_add_device_tree_early(struct
device_node *dn) { }
static inline void eeh_add_device_tree_late(struct pci_bus *bus) { }
+static inline void eeh_add_sysfs_files(struct pci_bus *bus) { }
+
static inline void eeh_remove_bus_device(struct pci_dev *dev, int purge_pe) { }
static inline void eeh_lock(void) { }
diff --git a/arch/powerpc/kernel/of_platform.c
b/arch/powerpc/kernel/of_platform.c
index c5fc6b2..500dd32 100644
--- a/arch/powerpc/kernel/of_platform.c
+++ b/arch/powerpc/kernel/of_platform.c
@@ -91,6 +91,9 @@ static int __devinit of_pci_phb_probe(struct platform_device
*dev)
/* Add probed PCI devices to the device model */
pci_bus_add_devices(phb-bus);
+ /* sysfs files should only be added after devices are added */
+ eeh_add_sysfs_files(phb-bus);
+
return 0;
}
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 7f94f76..4d3de7e 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1480,11 +1480,14 @@ void pcibios_finish_adding_to_bus(struct pci_bus *bus)
pcibios_allocate_bus_resources(bus);
pcibios_claim_one_bus(bus);
+ /* Fixup EEH */
+ eeh_add_device_tree_late(bus);
+
/* Add new devices to global lists. Register in proc, sysfs. */
pci_bus_add_devices(bus);
- /* Fixup EEH */
- eeh_add_device_tree_late(bus);
+ /* sysfs files should only be added after devices are added */
+ eeh_add_sysfs_files(bus);
}
EXPORT_SYMBOL_GPL(pcibios_finish_adding_to_bus);
diff --git a/arch/powerpc/platforms/pseries/eeh.c
b/arch/powerpc/platforms/pseries/eeh.c
index 9a04322..6b73d6c 100644
--- a/arch/powerpc/platforms/pseries/eeh.c
+++ b/arch/powerpc/platforms/pseries/eeh.c
@@ -788,7 +788,6 @@ static void eeh_add_device_late(struct pci_dev *dev)
dev-dev.archdata.edev = edev;
eeh_addr_cache_insert_dev(dev);
- eeh_sysfs_add_device(dev);
}
/**
@@ -815,6 +814,29 @@ void eeh_add_device_tree_late(struct pci_bus *bus)
EXPORT_SYMBOL_GPL(eeh_add_device_tree_late);
/**
+ * eeh_add_sysfs_files - Add EEH sysfs files for the indicated PCI bus
+ * @bus: PCI bus
+ *
+ * This routine must be used to add EEH sysfs files for PCI
+ * devices which are attached to the indicated PCI bus. The PCI bus
+ * is added after system boot through hotplug or dlpar.
+ */
+void eeh_add_sysfs_files(struct pci_bus *bus)
+{
+ struct pci_dev *dev;
+
+ list_for_each_entry(dev, bus-devices, bus_list) {
+ eeh_sysfs_add_device(dev);
+ if (dev-hdr_type == PCI_HEADER_TYPE_BRIDGE) {
+ struct pci_bus *subbus = dev-subordinate;
+ if (subbus)
+ eeh_add_sysfs_files(subbus);
+ }
+ }
+}
+EXPORT_SYMBOL_GPL(eeh_add_sysfs_files);
+
+/**
* eeh_remove_device - Undo EEH setup for the indicated pci device
* @dev: pci device to be removed
* @purge_pe: remove the PE or not
--
1.7.1
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
