[PATCH 00/23] PCI, x86: pci root bus hotplug support

[PATCH 00/23] PCI, x86: pci root bus hotplug support

[Yinghai Lu]

will add pci_stop_and_remove_bus() to support remove bus in
/sys/devices/pci.../pci_bus/...

it support pci root bus removal.

To rescan root, need to
echo 1 > /sys/bus/pci/rescan_root

this patcheset include some IOMM and dmar and pnpacpi fix with device refcount leaking.

The patches need to apply to pci/for-linus and pci/linux-next
and [PATCH 00/36] PCI: pci_host_bridge related cleanup and busn_alloc

could get from
        git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-root-bus-hotplug

Thanks

Yinghai

Yinghai Lu (23):
  PCI, sys: Use device_type and attr_groups with pci dev
  PCI, sysfs: create rescan_bridge under /sys/.../pci/devices/... for pci bridges
  PCI: Add pci_bus_add_single_device()
  PCI: make pci_rescan_bus_bridge_resize use pci_scan_bridge instead
  PCI: clean up rescan_bus_bridge_resize
  PCI: rescan with bus or bridge using callback method too
  pci, dmar: Update dmar units devices list during hotplug
  PNPACPI: Fix device ref leaking in acpi_pnp_match
  IOMMU: Fix tboot force iommu logic
  PCI, x86: Fix non acpi path pci_sysdata leaking with release_fn
  PCI: separate out pci_assign_unassigned_bus_resources()
  PCI: Move back pci_rescan_bus()
  PCI: move pci_stop_and_remove_behind_bridge down
  PCI: add __pci_remove_bus_devices()
  PCI: add pci_stop_and_remove_bus()
  PCI: add pci bus removal through /sys/.../pci_bus/.../remove
  PCI, ACPI: make acpi_pci_root_remove remove pci root bus too
  PCI, ACPI: add acpi_pci_root_rescan()
  PCI: add /sys/bus/pci/rescan_root
  PCI: add __pci_scan_root_bus() that can skip bus_add
  x86, PCI: add __pci_scan_root_bus_on_node() that can skip bus_add
  x86, PCI: add __pcibios_scan_specific_bus that can skip bus_add
  x86, PCI: add pcibios_root_rescan

 Documentation/ABI/testing/sysfs-bus-pci |   27 +++++
 arch/x86/include/asm/pci.h              |    2 +
 arch/x86/pci/common.c                   |   28 +++--
 arch/x86/pci/legacy.c                   |   44 ++++++-
 drivers/acpi/pci_root.c                 |   66 +++++++++++
 drivers/iommu/intel-iommu.c             |  193 ++++++++++++++++++++++++++++--
 drivers/pci/bus.c                       |   39 ++++++
 drivers/pci/pci-sysfs.c                 |  137 ++++++++++++++++++++--
 drivers/pci/pci.h                       |    1 +
 drivers/pci/probe.c                     |   43 ++++++-
 drivers/pci/remove.c                    |   77 ++++++++-----
 drivers/pci/setup-bus.c                 |   20 +---
 drivers/pnp/pnpacpi/core.c              |    7 +-
 include/linux/dmar.h                    |    4 +
 include/linux/pci.h                     |    6 +
 15 files changed, 604 insertions(+), 90 deletions(-)

-- 
1.7.7

[PATCH 01/23] PCI, sys: Use device_type and attr_groups with pci dev

[Yinghai Lu]

We want to create rescan in sys only for pci bridge instead of all pci dev.

We could use attribute_groups/is_visible method to do that.

Now pci dev does not use device type yet. So add pci_dev_type to take
attr_groups with it.

Add pci_dev_bridge_attrs_are_visible() to control attr_bridge_group only create
attr for bridge.

This is the framework related change, later could attr to bridge_attr_group,
to make those attr only show up on pci bridge device.

Also We could add more attr groups with is_visible to reduce messness in
	pci_create_sysfs_dev_files. ( at least for boot_vga one )

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/pci-sysfs.c |   30 ++++++++++++++++++++++++++++++
 drivers/pci/pci.h       |    1 +
 drivers/pci/probe.c     |    1 +
 3 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index a55e248..39d15e6 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1335,3 +1335,33 @@ static int __init pci_sysfs_init(void)
 }
 
 late_initcall(pci_sysfs_init);
+
+static struct attribute *pci_dev_bridge_attrs[] = {
+	NULL,
+};
+
+static umode_t pci_dev_bridge_attrs_are_visible(struct kobject *kobj,
+						struct attribute *a, int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (!pdev->subordinate)
+		return 0;
+
+	return a->mode;
+}
+
+static struct attribute_group pci_dev_bridge_attr_group = {
+	.attrs = pci_dev_bridge_attrs,
+	.is_visible = pci_dev_bridge_attrs_are_visible,
+};
+
+static const struct attribute_group *pci_dev_attr_groups[] = {
+	&pci_dev_bridge_attr_group,
+	NULL,
+};
+
+struct device_type pci_dev_type = {
+	.groups = pci_dev_attr_groups,
+};
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index e494347..13b1159 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -162,6 +162,7 @@ static inline int pci_no_d1d2(struct pci_dev *dev)
 }
 extern struct device_attribute pci_dev_attrs[];
 extern struct device_attribute pcibus_dev_attrs[];
+extern struct device_type pci_dev_type;
 #ifdef CONFIG_HOTPLUG
 extern struct bus_attribute pci_bus_attrs[];
 #else
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 67071b0..144f6f0 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1138,6 +1138,7 @@ int pci_setup_device(struct pci_dev *dev)
 	dev->sysdata = dev->bus->sysdata;
 	dev->dev.parent = dev->bus->bridge;
 	dev->dev.bus = &pci_bus_type;
+	dev->dev.type = &pci_dev_type;
 	dev->hdr_type = hdr_type & 0x7f;
 	dev->multifunction = !!(hdr_type & 0x80);
 	dev->error_state = pci_channel_io_normal;
-- 
1.7.7

[PATCH 02/23] PCI, sysfs: create rescan_bridge under /sys/.../pci/devices/... for pci bridges

[Yinghai Lu]

Current code will create rescan for every pci device under parent bus.
that is not right. the device is already there, there is no reason to rescan it.

We could have rescan for pci bridges. less confusing.

Need to move rescan attr to pci dev bridge attribute group.
And We should rescan bridge's secondary bus instead of primary bus.

-v3: Use device_type for pci dev.
-v4: Seperate pci device type change out
-v5: add rescan_bridge for bridge type, and still keep the old rescan.
	may remove the old rescan later.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Randy Dunlap <rdunlap@xenotime.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 Documentation/ABI/testing/sysfs-bus-pci |   10 ++++++++++
 drivers/pci/pci-sysfs.c                 |   24 ++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index 34f5110..95f0f37 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -111,6 +111,16 @@ Description:
 		from this part of the device tree.
 		Depends on CONFIG_HOTPLUG.
 
+What:		/sys/bus/pci/devices/.../rescan_bridge
+Date:		February 2012
+Contact:	Linux PCI developers <linux-pci@vger.kernel.org>
+Description:
+		Writing a non-zero value to this attribute will
+		force a rescan of the bridge and all child buses, and
+		re-discover devices removed earlier from this part of
+		the device tree.
+		Depends on CONFIG_HOTPLUG.
+
 What:		/sys/bus/pci/devices/.../reset
 Date:		July 2009
 Contact:	Michael S. Tsirkin <mst@redhat.com>
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 39d15e6..d5c8ffb 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -325,6 +325,27 @@ dev_rescan_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
+static ssize_t
+dev_bridge_rescan_store(struct device *dev, struct device_attribute *attr,
+		 const char *buf, size_t count)
+{
+	unsigned long val;
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (kstrtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	if (val) {
+		mutex_lock(&pci_remove_rescan_mutex);
+		pci_rescan_bus(pdev->subordinate);
+		mutex_unlock(&pci_remove_rescan_mutex);
+	}
+	return count;
+}
+
+static struct device_attribute pci_dev_bridge_rescan_attr =
+	__ATTR(rescan_bridge, (S_IWUSR|S_IWGRP), NULL, dev_bridge_rescan_store);
+
 static void remove_callback(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
@@ -1337,6 +1358,9 @@ static int __init pci_sysfs_init(void)
 late_initcall(pci_sysfs_init);
 
 static struct attribute *pci_dev_bridge_attrs[] = {
+#ifdef CONFIG_HOTPLUG
+	&pci_dev_bridge_rescan_attr.attr,
+#endif
 	NULL,
 };
 
-- 
1.7.7

[PATCH 03/23] PCI: Add pci_bus_add_single_device()

[Yinghai Lu]

will use it to pci_bus_bridge_scan_resize(0 to make bridge will
have pci_bus directory created.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/bus.c   |   39 +++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h |    1 +
 2 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 4ce5ef2..1eb7944 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -255,6 +255,45 @@ void pci_bus_add_devices(const struct pci_bus *bus)
 	}
 }
 
+void pci_bus_add_single_device(struct pci_dev *dev)
+{
+	struct pci_bus *child;
+	int retval;
+
+	/* Skip already-added devices */
+	if (!dev->is_added) {
+		retval = pci_bus_add_device(dev);
+		if (retval)
+			dev_err(&dev->dev, "Error adding device, continuing\n");
+	}
+
+	BUG_ON(!dev->is_added);
+
+	child = dev->subordinate;
+	/*
+	 * If there is an unattached subordinate bus, attach
+	 * it and then scan for unattached PCI devices.
+	 */
+	if (child) {
+		if (list_empty(&child->node)) {
+			down_write(&pci_bus_sem);
+			list_add_tail(&child->node, &dev->bus->children);
+			up_write(&pci_bus_sem);
+		}
+		pci_bus_add_devices(child);
+
+		/*
+		 * register the bus with sysfs as the parent is now
+		 * properly registered.
+		 */
+		if (!child->is_added) {
+			retval = pci_bus_add_child(child);
+			if (retval)
+				dev_err(&dev->dev, "Error adding bus, continuing\n");
+		}
+	}
+}
+
 void pci_enable_bridges(struct pci_bus *bus)
 {
 	struct pci_dev *dev;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 40606c1..28556cb 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -665,6 +665,7 @@ void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
 void pcibios_scan_specific_bus(int busn);
 extern struct pci_bus *pci_find_bus(int domain, int busnr);
 void pci_bus_add_devices(const struct pci_bus *bus);
+void pci_bus_add_single_device(struct pci_dev *dev);
 struct pci_bus *pci_scan_bus_parented(struct device *parent, int bus,
 				      struct pci_ops *ops, void *sysdata);
 struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata);
-- 
1.7.7

[PATCH 04/23] PCI: make pci_rescan_bus_bridge_resize use pci_scan_bridge instead

[Yinghai Lu]

So after remove all children and then using setpci change bus register and rescan
bridge could use new set bus number.

Otherwise need to rescan parent bus, it would have too much overhead.

also need to use pci_bus_add_single_device to make sure new change bus have directory
 /sys/../.../pci_bus.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/probe.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 144f6f0..5e39819 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2043,14 +2043,16 @@ EXPORT_SYMBOL(pci_scan_bus);
  */
 unsigned int __ref pci_rescan_bus_bridge_resize(struct pci_dev *bridge)
 {
-	unsigned int max;
-	struct pci_bus *bus = bridge->subordinate;
+	unsigned int max = 0;
+	int pass;
+	struct pci_bus *bus = bridge->bus;
 
-	max = pci_scan_child_bus(bus);
+	for (pass = 0; pass < 2; pass++)
+		max = pci_scan_bridge(bus, bridge, max, pass);
 
 	pci_assign_unassigned_bridge_resources(bridge);
 
-	pci_bus_add_devices(bus);
+	pci_bus_add_single_device(bridge);
 
 	return max;
 }
-- 
1.7.7

[PATCH 05/23] PCI: clean up rescan_bus_bridge_resize

[Yinghai Lu]

Should only use it with bridge instead of bus.
it could get new subordinate. so can not use it with reguar bus.

in that case, use may need to make all children devices get removed already.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/pci-sysfs.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index d5c8ffb..2049b2f 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -337,7 +337,7 @@ dev_bridge_rescan_store(struct device *dev, struct device_attribute *attr,
 
 	if (val) {
 		mutex_lock(&pci_remove_rescan_mutex);
-		pci_rescan_bus(pdev->subordinate);
+		pci_rescan_bus_bridge_resize(pdev);
 		mutex_unlock(&pci_remove_rescan_mutex);
 	}
 	return count;
@@ -387,10 +387,7 @@ dev_bus_rescan_store(struct device *dev, struct device_attribute *attr,
 
 	if (val) {
 		mutex_lock(&pci_remove_rescan_mutex);
-		if (!pci_is_root_bus(bus) && list_empty(&bus->devices))
-			pci_rescan_bus_bridge_resize(bus->self);
-		else
-			pci_rescan_bus(bus);
+		pci_rescan_bus(bus);
 		mutex_unlock(&pci_remove_rescan_mutex);
 	}
 	return count;
-- 
1.7.7

[PATCH 06/23] PCI: rescan with bus or bridge using callback method too

[Yinghai Lu]

Just like removal.

Because We could add new bus under the bridges...

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/pci-sysfs.c |   43 +++++++++++++++++++++++++++++++------------
 1 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 2049b2f..1794508 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -325,21 +325,31 @@ dev_rescan_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
+static void bridge_rescan_callback(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	mutex_lock(&pci_remove_rescan_mutex);
+	pci_rescan_bus_bridge_resize(pdev);
+	mutex_unlock(&pci_remove_rescan_mutex);
+}
+
 static ssize_t
 dev_bridge_rescan_store(struct device *dev, struct device_attribute *attr,
 		 const char *buf, size_t count)
 {
+	int ret = 0;
 	unsigned long val;
-	struct pci_dev *pdev = to_pci_dev(dev);
 
 	if (kstrtoul(buf, 0, &val) < 0)
 		return -EINVAL;
 
-	if (val) {
-		mutex_lock(&pci_remove_rescan_mutex);
-		pci_rescan_bus_bridge_resize(pdev);
-		mutex_unlock(&pci_remove_rescan_mutex);
-	}
+	if (val)
+		ret = device_schedule_callback(dev, bridge_rescan_callback);
+
+	if (ret)
+		count = ret;
+
 	return count;
 }
 
@@ -375,21 +385,30 @@ remove_store(struct device *dev, struct device_attribute *dummy,
 	return count;
 }
 
+static void bus_rescan_callback(struct device *dev)
+{
+	struct pci_bus *bus = to_pci_bus(dev);
+
+	mutex_lock(&pci_remove_rescan_mutex);
+	pci_rescan_bus(bus);
+	mutex_unlock(&pci_remove_rescan_mutex);
+}
 static ssize_t
 dev_bus_rescan_store(struct device *dev, struct device_attribute *attr,
 		 const char *buf, size_t count)
 {
+	int ret = 0;
 	unsigned long val;
-	struct pci_bus *bus = to_pci_bus(dev);
 
 	if (strict_strtoul(buf, 0, &val) < 0)
 		return -EINVAL;
 
-	if (val) {
-		mutex_lock(&pci_remove_rescan_mutex);
-		pci_rescan_bus(bus);
-		mutex_unlock(&pci_remove_rescan_mutex);
-	}
+	if (val)
+		ret = device_schedule_callback(dev, bus_rescan_callback);
+
+	if (ret)
+		count = ret;
+
 	return count;
 }
 
-- 
1.7.7

[PATCH 07/23] pci, dmar: Update dmar units devices list during hotplug

[Yinghai Lu]

When do pci remove/rescan on system that have more iommus, got

[  894.089745] Set context mapping for c4:00.0
[  894.110890] mpt2sas3: Allocated physical memory: size(4293 kB)
[  894.112556] mpt2sas3: Current Controller Queue Depth(1883), Max Controller Queue Depth(2144)
[  894.127278] mpt2sas3: Scatter Gather Elements per IO(128)
[  894.361295] DRHD: handling fault status reg 2
[  894.364053] DMAR:[DMA Read] Request device [c4:00.0] fault addr fffbe000
[  894.364056] DMAR:[fault reason 02] Present bit in context entry is cl

it turns out when remove/rescan, pci dev will be freed and will get another new dev.
but drhd units still keep old one... so dmar_find_matched_drhd_unit will
return wrong drhd and iommu for the device that is not on first iommu.

So need to update devices in drhd_units during pci remove/rescan.

Could save domain/bus/device/function aside in the list and according that info
restore new dev to drhd_units later.
Then dmar_find_matched_drdh_unit and device_to_iommu could return right drhd and iommu.

Add remove_dev_from_drhd/restore_dev_to_drhd functions to do the real work.
call them in device ADD_DEVICE and UNBOUND_DRIVER

Need to do the samething to atsr.  (expose dmar_atsr_units and add atsru->segment)

After patch, will right iommu for the new dev and will not get DMAR error any more.

-v2: add pci_dev_put/pci_dev_get to make refcount consistent.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Vinod Koul <vinod.koul@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: iommu@lists.linux-foundation.org
---
 drivers/iommu/intel-iommu.c |  187 ++++++++++++++++++++++++++++++++++++++++---
 include/linux/dmar.h        |    4 +
 2 files changed, 181 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index c9c6053..39ef2ce 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -665,6 +665,159 @@ static struct intel_iommu *device_to_iommu(int segment, u8 bus, u8 devfn)
 	return NULL;
 }
 
+#ifdef CONFIG_HOTPLUG
+struct dev_dmaru {
+	struct list_head list;
+	void *dmaru;
+	int index;
+	int segment;
+	unsigned char bus;
+	unsigned int devfn;
+};
+
+static int
+save_dev_dmaru(int segment, unsigned char bus, unsigned int devfn,
+		 void *dmaru, int index, struct list_head *lh)
+{
+	struct dev_dmaru *m;
+
+	m = kzalloc(sizeof(*m), GFP_KERNEL);
+	if (!m)
+		return -ENOMEM;
+
+	m->segment = segment;
+	m->bus     = bus;
+	m->devfn   = devfn;
+	m->dmaru   = dmaru;
+	m->index   = index;
+
+	list_add(&m->list, lh);
+
+	return 0;
+}
+
+static void
+*get_dev_dmaru(int segment, unsigned char bus, unsigned int devfn,
+		int *index, struct list_head *lh)
+{
+	struct dev_dmaru *m;
+	void *dmaru = NULL;
+
+	list_for_each_entry(m, lh, list) {
+		if (m->segment == segment &&
+		    m->bus == bus && m->devfn == devfn) {
+			*index = m->index;
+			dmaru  = m->dmaru;
+			list_del(&m->list);
+			kfree(m);
+			break;
+		}
+	}
+
+	return dmaru;
+}
+
+static LIST_HEAD(saved_dev_drhd_list);
+
+static void remove_dev_from_drhd(struct pci_dev *dev)
+{
+	struct dmar_drhd_unit *drhd = NULL;
+	int segment = pci_domain_nr(dev->bus);
+	int i;
+
+	for_each_drhd_unit(drhd) {
+		if (drhd->ignored)
+			continue;
+		if (segment != drhd->segment)
+			continue;
+
+		for (i = 0; i < drhd->devices_cnt; i++) {
+			if (drhd->devices[i] == dev) {
+				/* save it at first if it is in drhd */
+				save_dev_dmaru(segment, dev->bus->number,
+						dev->devfn, drhd, i,
+						&saved_dev_drhd_list);
+				/* always remove it */
+				pci_dev_put(dev);
+				drhd->devices[i] = NULL;
+				return;
+			}
+		}
+	}
+}
+
+static void restore_dev_to_drhd(struct pci_dev *dev)
+{
+	struct dmar_drhd_unit *drhd = NULL;
+	int i;
+
+	/* find the stored drhd */
+	drhd = get_dev_dmaru(pci_domain_nr(dev->bus), dev->bus->number,
+				 dev->devfn, &i, &saved_dev_drhd_list);
+	/* restore that into drhd */
+	if (drhd)
+		drhd->devices[i] = pci_dev_get(dev);
+}
+#else
+static void remove_dev_from_drhd(struct pci_dev *dev)
+{
+}
+
+static void restore_dev_to_drhd(struct pci_dev *dev)
+{
+}
+#endif
+
+#if defined(CONFIG_DMAR) && defined(CONFIG_HOTPLUG)
+static LIST_HEAD(saved_dev_atsr_list);
+
+static void remove_dev_from_atsr(struct pci_dev *dev)
+{
+	struct dmar_atsr_unit *atsr = NULL;
+	int segment = pci_domain_nr(dev->bus);
+	int i;
+
+	for_each_atsr_unit(atsr) {
+		if (segment != atsr->segment)
+			continue;
+
+		for (i = 0; i < atsr->devices_cnt; i++) {
+			if (atsr->devices[i] == dev) {
+				/* save it at first if it is in drhd */
+				save_dev_dmaru(segment, dev->bus->number,
+						dev->devfn, atsr, i,
+						&saved_dev_atsr_list);
+				/* always remove it */
+				pci_dev_put(dev);
+				atsr->devices[i] = NULL;
+				return;
+			}
+		}
+	}
+}
+
+static void restore_dev_to_atsr(struct pci_dev *dev)
+{
+	struct dmar_atsr_unit *atsr = NULL;
+	int i;
+
+	/* find the stored atsr */
+	atsr = get_dev_dmaru(pci_domain_nr(dev->bus), dev->bus->number,
+				 dev->devfn, &i, &saved_dev_atsr_list);
+	/* restore that into atsr */
+	if (atsr)
+		atsr->devices[i] = pci_dev_get(dev);
+}
+#else
+static void remove_dev_from_atsr(struct pci_dev *dev)
+{
+}
+
+static void restore_dev_to_atsr(struct pci_dev *dev)
+{
+}
+#endif
+
 static void domain_flush_cache(struct dmar_domain *domain,
 			       void *addr, int size)
 {
@@ -3474,6 +3627,7 @@ int __init dmar_parse_one_atsr(struct acpi_dmar_header *hdr)
 
 	atsru->hdr = hdr;
 	atsru->include_all = atsr->flags & 0x1;
+	atsru->segment = atsr->segment;
 
 	list_add(&atsru->list, &dmar_atsr_units);
 
@@ -3505,16 +3659,13 @@ int dmar_find_matched_atsr_unit(struct pci_dev *dev)
 {
 	int i;
 	struct pci_bus *bus;
-	struct acpi_dmar_atsr *atsr;
 	struct dmar_atsr_unit *atsru;
 
 	dev = pci_physfn(dev);
 
-	list_for_each_entry(atsru, &dmar_atsr_units, list) {
-		atsr = container_of(atsru->hdr, struct acpi_dmar_atsr, header);
-		if (atsr->segment == pci_domain_nr(dev->bus))
+	list_for_each_entry(atsru, &dmar_atsr_units, list)
+		if (atsru->segment == pci_domain_nr(dev->bus))
 			goto found;
-	}
 
 	return 0;
 
@@ -3574,20 +3725,36 @@ static int device_notifier(struct notifier_block *nb,
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct dmar_domain *domain;
 
-	if (iommu_no_mapping(dev))
+	if (unlikely(dev->bus != &pci_bus_type))
 		return 0;
 
-	domain = find_domain(pdev);
-	if (!domain)
-		return 0;
+	switch (action) {
+	case BUS_NOTIFY_UNBOUND_DRIVER:
+		if (iommu_no_mapping(dev))
+			goto out;
+
+		if (iommu_pass_through)
+			goto out;
+
+		domain = find_domain(pdev);
+		if (!domain)
+			goto out;
 
-	if (action == BUS_NOTIFY_UNBOUND_DRIVER && !iommu_pass_through) {
 		domain_remove_one_dev_info(domain, pdev);
 
 		if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) &&
 		    !(domain->flags & DOMAIN_FLAG_STATIC_IDENTITY) &&
 		    list_empty(&domain->devices))
 			domain_exit(domain);
+out:
+		remove_dev_from_drhd(pdev);
+		remove_dev_from_atsr(pdev);
+
+		break;
+	case BUS_NOTIFY_ADD_DEVICE:
+		restore_dev_to_drhd(pdev);
+		restore_dev_to_atsr(pdev);
+		break;
 	}
 
 	return 0;
diff --git a/include/linux/dmar.h b/include/linux/dmar.h
index 731a609..57e1375 100644
--- a/include/linux/dmar.h
+++ b/include/linux/dmar.h
@@ -236,9 +236,13 @@ struct dmar_atsr_unit {
 	struct acpi_dmar_header *hdr;	/* ACPI header */
 	struct pci_dev **devices;	/* target devices */
 	int devices_cnt;		/* target device count */
+	u16 segment;			/* PCI domain */
 	u8 include_all:1;		/* include all ports */
 };
 
+#define for_each_atsr_unit(atsr) \
+	list_for_each_entry(atsr, &dmar_atsr_units, list)
+
 int dmar_parse_rmrr_atsr_dev(void);
 extern int dmar_parse_one_rmrr(struct acpi_dmar_header *header);
 extern int dmar_parse_one_atsr(struct acpi_dmar_header *header);
-- 
1.7.7

[PATCH 08/23] PNPACPI: Fix device ref leaking in acpi_pnp_match

[Yinghai Lu]

During testing pci root bus removal, found some root bus bridge is not freed.

If booting with pnpacpi=off, those hostbridge could be freed without problem.

It turns out that some devices reference are not released during acpi_pnp_match.

that match should not hold one device ref during every calling.

Add put_device calling before returning.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: stable@kernel.org
Cc: Len Brown <lenb@kernel.org>
Cc: Adam Belay <abelay@mit.edu>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-acpi@vger.kernel.org
---
 drivers/pnp/pnpacpi/core.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
index b00c176..d21e8f5 100644
--- a/drivers/pnp/pnpacpi/core.c
+++ b/drivers/pnp/pnpacpi/core.c
@@ -321,9 +321,14 @@ static int __init acpi_pnp_match(struct device *dev, void *_pnp)
 {
 	struct acpi_device *acpi = to_acpi_device(dev);
 	struct pnp_dev *pnp = _pnp;
+	struct device *physical_device;
+
+	physical_device = acpi_get_physical_device(acpi->handle);
+	if (physical_device)
+		put_device(physical_device);
 
 	/* true means it matched */
-	return !acpi_get_physical_device(acpi->handle)
+	return !physical_device
 	    && compare_pnp_id(pnp->id, acpi_device_hid(acpi));
 }
 
-- 
1.7.7

[PATCH 09/23] IOMMU: Fix tboot force iommu logic

[Yinghai Lu]

Should check dmar_disabled just after tboot_force_iommu.

otherwise when tboot is not used, and intel_iommu=off, and nointrmap
still get dmar_table_init() called. that will cause some get_device
calling, and it will have some device refcount leaking.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: iommu@lists.linux-foundation.org
---
 drivers/iommu/intel-iommu.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 39ef2ce..f1879d3 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3771,6 +3771,9 @@ int __init intel_iommu_init(void)
 	/* VT-d is required for a TXT/tboot launch, so enforce that */
 	force_on = tboot_force_iommu();
 
+	if (no_iommu || dmar_disabled)
+		return -ENODEV;
+
 	if (dmar_table_init()) {
 		if (force_on)
 			panic("tboot: Failed to initialize DMAR table\n");
@@ -3783,9 +3786,6 @@ int __init intel_iommu_init(void)
 		return 	-ENODEV;
 	}
 
-	if (no_iommu || dmar_disabled)
-		return -ENODEV;
-
 	if (iommu_init_mempool()) {
 		if (force_on)
 			panic("tboot: Failed to initialize iommu memory\n");
-- 
1.7.7

[PATCH 10/23] PCI, x86: Fix non acpi path pci_sysdata leaking with release_fn

[Yinghai Lu]

non acpi path, or root is not probed from acpi, during root bus removal
will get warning about leaking from pci_scan_bus_on_node.

Fix it with setting pci_host_bridge release function.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 arch/x86/pci/common.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 8e04ec5..35931f6 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -618,17 +618,19 @@ int pci_ext_cfg_avail(struct pci_dev *dev)
 		return 0;
 }
 
+static void release_pci_sysdata(struct pci_host_bridge *bridge)
+{
+	struct pci_sysdata *sd = bridge->release_data;
+
+	kfree(sd);
+}
+
 struct pci_bus * __devinit pci_scan_bus_on_node(int busno, struct pci_ops *ops, int node)
 {
 	LIST_HEAD(resources);
 	struct pci_bus *bus = NULL;
 	struct pci_sysdata *sd;
 
-	/*
-	 * Allocate per-root-bus (not per bus) arch-specific data.
-	 * TODO: leak; this memory is never freed.
-	 * It's arguable whether it's worth the trouble to care.
-	 */
 	sd = kzalloc(sizeof(*sd), GFP_KERNEL);
 	if (!sd) {
 		printk(KERN_ERR "PCI: OOM, skipping PCI bus %02x\n", busno);
@@ -638,7 +640,10 @@ struct pci_bus * __devinit pci_scan_bus_on_node(int busno, struct pci_ops *ops,
 	x86_pci_root_bus_resources(busno, &resources);
 	printk(KERN_DEBUG "PCI: Probing PCI hardware (bus %02x)\n", busno);
 	bus = pci_scan_root_bus(NULL, busno, ops, sd, &resources);
-	if (!bus) {
+	if (bus)
+		pci_set_host_bridge_release(to_pci_host_bridge(bus->bridge),
+					release_pci_sysdata, sd);
+	else {
 		pci_free_resource_list(&resources);
 		kfree(sd);
 	}
-- 
1.7.7

[PATCH 11/23] PCI: separate out pci_assign_unassigned_bus_resources()

[Yinghai Lu]

it is main portion of pci_rescan_bus().

Separate it out and will use it later for pci root bus rescan.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/setup-bus.c |   34 ++++++++++++++++++++--------------
 include/linux/pci.h     |    1 +
 2 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 8fa2d4b..6a66139 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1488,25 +1488,12 @@ enable_all:
 }
 EXPORT_SYMBOL_GPL(pci_assign_unassigned_bridge_resources);
 
-#ifdef CONFIG_HOTPLUG
-/**
- * pci_rescan_bus - scan a PCI bus for devices.
- * @bus: PCI bus to scan
- *
- * Scan a PCI bus and child buses for new devices, adds them,
- * and enables them.
- *
- * Returns the max number of subordinate bus discovered.
- */
-unsigned int __ref pci_rescan_bus(struct pci_bus *bus)
+void pci_assign_unassigned_bus_resources(struct pci_bus *bus)
 {
-	unsigned int max;
 	struct pci_dev *dev;
 	LIST_HEAD(add_list); /* list of resources that
 					want additional resources */
 
-	max = pci_scan_child_bus(bus);
-
 	down_read(&pci_bus_sem);
 	list_for_each_entry(dev, &bus->devices, bus_list)
 		if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||
@@ -1519,6 +1506,25 @@ unsigned int __ref pci_rescan_bus(struct pci_bus *bus)
 	BUG_ON(!list_empty(&add_list));
 
 	pci_enable_bridges(bus);
+}
+#ifdef CONFIG_HOTPLUG
+/**
+ * pci_rescan_bus - scan a PCI bus for devices.
+ * @bus: PCI bus to scan
+ *
+ * Scan a PCI bus and child buses for new devices, adds them,
+ * and enables them.
+ *
+ * Returns the max number of subordinate bus discovered.
+ */
+unsigned int __ref pci_rescan_bus(struct pci_bus *bus)
+{
+	unsigned int max;
+
+	max = pci_scan_child_bus(bus);
+
+	pci_assign_unassigned_bus_resources(bus);
+
 	pci_bus_add_devices(bus);
 
 	return max;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 28556cb..4f30cd1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -910,6 +910,7 @@ void pci_bus_size_bridges(struct pci_bus *bus);
 int pci_claim_resource(struct pci_dev *, int);
 void pci_assign_unassigned_resources(void);
 void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge);
+void pci_assign_unassigned_bus_resources(struct pci_bus *bus);
 void pdev_enable_device(struct pci_dev *);
 int pci_enable_resources(struct pci_dev *, int mask);
 void pci_fixup_irqs(u8 (*)(struct pci_dev *, u8 *),
-- 
1.7.7

[PATCH 12/23] PCI: Move back pci_rescan_bus()

[Yinghai Lu]

Now we have pci_assign_unassigned_bus_resources() in place.

So move back pci_rescan_bus to probe.c

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/probe.c     |   23 +++++++++++++++++++++++
 drivers/pci/setup-bus.c |   24 ------------------------
 2 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 5e39819..6240d2c 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2057,6 +2057,29 @@ unsigned int __ref pci_rescan_bus_bridge_resize(struct pci_dev *bridge)
 	return max;
 }
 
+/**
+ * pci_rescan_bus - scan a PCI bus for devices.
+ * @bus: PCI bus to scan
+ *
+ * Scan a PCI bus and child buses for new devices, adds them,
+ * and enables them.
+ *
+ * Returns the max number of subordinate bus discovered.
+ */
+unsigned int __ref pci_rescan_bus(struct pci_bus *bus)
+{
+	unsigned int max;
+
+	max = pci_scan_child_bus(bus);
+
+	pci_assign_unassigned_bus_resources(bus);
+
+	pci_bus_add_devices(bus);
+
+	return max;
+}
+EXPORT_SYMBOL_GPL(pci_rescan_bus);
+
 EXPORT_SYMBOL(pci_add_new_bus);
 EXPORT_SYMBOL(pci_scan_slot);
 EXPORT_SYMBOL(pci_scan_bridge);
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 6a66139..f35a679 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1507,27 +1507,3 @@ void pci_assign_unassigned_bus_resources(struct pci_bus *bus)
 
 	pci_enable_bridges(bus);
 }
-#ifdef CONFIG_HOTPLUG
-/**
- * pci_rescan_bus - scan a PCI bus for devices.
- * @bus: PCI bus to scan
- *
- * Scan a PCI bus and child buses for new devices, adds them,
- * and enables them.
- *
- * Returns the max number of subordinate bus discovered.
- */
-unsigned int __ref pci_rescan_bus(struct pci_bus *bus)
-{
-	unsigned int max;
-
-	max = pci_scan_child_bus(bus);
-
-	pci_assign_unassigned_bus_resources(bus);
-
-	pci_bus_add_devices(bus);
-
-	return max;
-}
-EXPORT_SYMBOL_GPL(pci_rescan_bus);
-#endif
-- 
1.7.7

[PATCH 13/23] PCI: move pci_stop_and_remove_behind_bridge down

[Yinghai Lu]

later it will use pci_stop_bus_devices instead of pci_stop_behind_bridge.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/remove.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 04a4861..243d59b 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -129,21 +129,6 @@ static void pci_stop_behind_bridge(struct pci_dev *dev)
 			pci_stop_bus_device(pci_dev_b(l));
 }
 
-/**
- * pci_stop_and_remove_behind_bridge - stop and remove all devices behind
- *					 a PCI bridge
- * @dev: PCI bridge device
- *
- * Remove all devices on the bus, except for the parent bridge.
- * This also removes any child buses, and any devices they may
- * contain in a depth-first manner.
- */
-void pci_stop_and_remove_behind_bridge(struct pci_dev *dev)
-{
-	pci_stop_behind_bridge(dev);
-	__pci_remove_behind_bridge(dev);
-}
-
 static void pci_stop_bus_devices(struct pci_bus *bus)
 {
 	struct list_head *l, *n;
@@ -163,6 +148,21 @@ static void pci_stop_bus_devices(struct pci_bus *bus)
 }
 
 /**
+ * pci_stop_and_remove_behind_bridge - stop and remove all devices behind
+ *					 a PCI bridge
+ * @dev: PCI bridge device
+ *
+ * Remove all devices on the bus, except for the parent bridge.
+ * This also removes any child buses, and any devices they may
+ * contain in a depth-first manner.
+ */
+void pci_stop_and_remove_behind_bridge(struct pci_dev *dev)
+{
+	pci_stop_behind_bridge(dev);
+	__pci_remove_behind_bridge(dev);
+}
+
+/**
  * pci_stop_bus_device - stop a PCI device and any children
  * @dev: the device to stop
  *
-- 
1.7.7

[PATCH 14/23] PCI: add __pci_remove_bus_devices()

[Yinghai Lu]

will use it with pci_stop_and_remove_bus later.

also remove __pci_remove_behind_bridge and pci_stop_behind_bridge.

they are same except one take bridge and one take bus.

and we already have pci_stop_bus_devices()

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/remove.c |   28 +++++++++++-----------------
 1 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 243d59b..62c348c 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -78,7 +78,7 @@ void pci_remove_bus(struct pci_bus *pci_bus)
 }
 EXPORT_SYMBOL(pci_remove_bus);
 
-static void __pci_remove_behind_bridge(struct pci_dev *dev);
+static void __pci_remove_bus_devices(struct pci_bus *bus);
 /**
  * pci_stop_and_remove_bus_device - remove a PCI device and any children
  * @dev: the device to remove
@@ -96,7 +96,7 @@ void __pci_remove_bus_device(struct pci_dev *dev)
 	if (dev->subordinate) {
 		struct pci_bus *b = dev->subordinate;
 
-		__pci_remove_behind_bridge(dev);
+		__pci_remove_bus_devices(b);
 		pci_remove_bus(b);
 		dev->subordinate = NULL;
 	}
@@ -111,22 +111,12 @@ void pci_stop_and_remove_bus_device(struct pci_dev *dev)
 	__pci_remove_bus_device(dev);
 }
 
-static void __pci_remove_behind_bridge(struct pci_dev *dev)
+static void __pci_remove_bus_devices(struct pci_bus *bus)
 {
 	struct list_head *l, *n;
 
-	if (dev->subordinate)
-		list_for_each_safe(l, n, &dev->subordinate->devices)
-			__pci_remove_bus_device(pci_dev_b(l));
-}
-
-static void pci_stop_behind_bridge(struct pci_dev *dev)
-{
-	struct list_head *l, *n;
-
-	if (dev->subordinate)
-		list_for_each_safe(l, n, &dev->subordinate->devices)
-			pci_stop_bus_device(pci_dev_b(l));
+	list_for_each_safe(l, n, &bus->devices)
+		__pci_remove_bus_device(pci_dev_b(l));
 }
 
 static void pci_stop_bus_devices(struct pci_bus *bus)
@@ -158,8 +148,12 @@ static void pci_stop_bus_devices(struct pci_bus *bus)
  */
 void pci_stop_and_remove_behind_bridge(struct pci_dev *dev)
 {
-	pci_stop_behind_bridge(dev);
-	__pci_remove_behind_bridge(dev);
+	struct pci_bus *bus = dev->subordinate;
+
+	if (bus) {
+		pci_stop_bus_devices(bus);
+		__pci_remove_bus_devices(bus);
+	}
 }
 
 /**
-- 
1.7.7

[PATCH 15/23] PCI: add pci_stop_and_remove_bus()

[Yinghai Lu]

it supports both pci root bus and pci bus under pci bridge.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/remove.c |   27 +++++++++++++++++++++++++++
 include/linux/pci.h  |    1 +
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 62c348c..13a1555 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -172,6 +172,33 @@ void pci_stop_bus_device(struct pci_dev *dev)
 	pci_stop_dev(dev);
 }
 
+static void pci_stop_host_bridge(struct pci_host_bridge *bridge)
+{
+	device_unregister(&bridge->dev);
+}
+/*
+ * it will support pci root bus too, in that case we need
+ *  stop and remove host bridge
+ */
+void pci_stop_and_remove_bus(struct pci_bus *bus)
+{
+	struct pci_host_bridge *host_bridge = NULL;
+
+	pci_stop_bus_devices(bus);
+
+	if (pci_is_root_bus(bus)) {
+		host_bridge = to_pci_host_bridge(bus->bridge);
+		pci_stop_host_bridge(host_bridge);
+	}
+
+	__pci_remove_bus_devices(bus);
+
+	pci_remove_bus(bus);
+
+	if (host_bridge)
+		host_bridge->bus = NULL;
+}
+
 EXPORT_SYMBOL(pci_stop_and_remove_bus_device);
 EXPORT_SYMBOL(pci_stop_and_remove_behind_bridge);
 EXPORT_SYMBOL_GPL(pci_stop_bus_device);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 4f30cd1..3d5feda 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -702,6 +702,7 @@ extern void pci_dev_put(struct pci_dev *dev);
 extern void pci_remove_bus(struct pci_bus *b);
 extern void __pci_remove_bus_device(struct pci_dev *dev);
 extern void pci_stop_and_remove_bus_device(struct pci_dev *dev);
+void pci_stop_and_remove_bus(struct pci_bus *bus);
 extern void pci_stop_bus_device(struct pci_dev *dev);
 void pci_setup_cardbus(struct pci_bus *bus);
 extern void pci_sort_breadthfirst(void);
-- 
1.7.7

[PATCH 16/23] PCI: add pci bus removal through /sys/.../pci_bus/.../remove

[Yinghai Lu]

it supports both pci root bus and pci bus under pci bridge.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Randy Dunlap <rdunlap@xenotime.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-doc@vger.kernel.org
---
 Documentation/ABI/testing/sysfs-bus-pci |    8 ++++++++
 drivers/pci/pci-sysfs.c                 |   28 ++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index 95f0f37..22392de 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -92,6 +92,14 @@ Description:
 		hot-remove the PCI device and any of its children.
 		Depends on CONFIG_HOTPLUG.
 
+What:		/sys/bus/pci/devices/.../pci_bus/.../remove
+Date:		March 2012
+Contact:	Linux PCI developers <linux-pci@vger.kernel.org>
+Description:
+		Writing a non-zero value to this attribute will
+		hot-remove the PCI bus and any of its children.
+		Depends on CONFIG_HOTPLUG.
+
 What:		/sys/bus/pci/devices/.../pci_bus/.../rescan
 Date:		May 2011
 Contact:	Linux PCI developers <linux-pci@vger.kernel.org>
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 1794508..f855b4b 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -385,6 +385,33 @@ remove_store(struct device *dev, struct device_attribute *dummy,
 	return count;
 }
 
+static void bus_remove_callback(struct device *dev)
+{
+	struct pci_bus *bus = to_pci_bus(dev);
+
+	mutex_lock(&pci_remove_rescan_mutex);
+	pci_stop_and_remove_bus(bus);
+	mutex_unlock(&pci_remove_rescan_mutex);
+}
+static ssize_t
+dev_bus_remove_store(struct device *dev, struct device_attribute *attr,
+		 const char *buf, size_t count)
+{
+	int ret = 0;
+	unsigned long val;
+
+	if (kstrtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	if (val)
+		ret = device_schedule_callback(dev, bus_remove_callback);
+
+	if (ret)
+		count = ret;
+
+	return count;
+}
+
 static void bus_rescan_callback(struct device *dev)
 {
 	struct pci_bus *bus = to_pci_bus(dev);
@@ -444,6 +471,7 @@ struct device_attribute pci_dev_attrs[] = {
 struct device_attribute pcibus_dev_attrs[] = {
 #ifdef CONFIG_HOTPLUG
 	__ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store),
+	__ATTR(remove, (S_IWUSR|S_IWGRP), NULL, dev_bus_remove_store),
 #endif
 	__ATTR(cpuaffinity, S_IRUGO, pci_bus_show_cpumaskaffinity, NULL),
 	__ATTR(cpulistaffinity, S_IRUGO, pci_bus_show_cpulistaffinity, NULL),
-- 
1.7.7

[PATCH 17/23] PCI, ACPI: make acpi_pci_root_remove remove pci root bus too

[Yinghai Lu]

it will call new added pci_stop_and_remove_bus() to stop/remove pci root bus.

also checking if that pci_root_bus get removed already in bus remove in /sys

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-acpi@vger.kernel.org
---
 drivers/acpi/pci_root.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 7aff631..b38e347 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -643,10 +643,24 @@ static int acpi_pci_root_remove(struct acpi_device *device, int type)
 {
 	struct acpi_pci_root *root = acpi_driver_data(device);
 
+	/* that root bus could be removed already */
+	if (!pci_find_bus(root->segment, root->secondary.start)) {
+		dev_printk(KERN_DEBUG, &device->dev,
+		  "freeing acpi_pci_root, but pci root bus was removed before");
+		goto out;
+	}
+
 	device_set_run_wake(root->bus->bridge, false);
 	pci_acpi_remove_bus_pm_notifier(device);
 
+	dev_printk(KERN_DEBUG, &device->dev,
+		"freeing acpi_pci_root, will remove pci root bus at first");
+	pci_stop_and_remove_bus(root->bus);
+
+out:
+	list_del(&root->node);
 	kfree(root);
+
 	return 0;
 }
 
-- 
1.7.7

[PATCH 18/23] PCI, ACPI: add acpi_pci_root_rescan()

[Yinghai Lu]

it will rescan all acpi pci root if related pci root bus get removed before.

Signed-off-by: Yinghai <yinghai@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-acpi@vger.kernel.org
---
 drivers/acpi/pci_root.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 52 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index b38e347..e1814e0 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -679,3 +679,55 @@ static int __init acpi_pci_root_init(void)
 }
 
 subsys_initcall(acpi_pci_root_init);
+
+static acpi_status
+rescan_root_bridge(acpi_handle handle, u32 lvl, void *context, void **rv)
+{
+	struct acpi_device *device, *pdevice;
+	struct acpi_pci_root *root;
+	acpi_handle phandle;
+	int ret_val;
+
+	if (!acpi_is_root_bridge(handle))
+		return AE_OK;
+
+	acpi_get_parent(handle, &phandle);
+	if (acpi_bus_get_device(phandle, &pdevice)) {
+		printk(KERN_DEBUG "no parent device, assuming NULL\n");
+		pdevice = NULL;
+	}
+
+	if (!acpi_bus_get_device(handle, &device)) {
+		/* check if  pci root_bus is removed */
+		root = acpi_driver_data(device);
+		if (pci_find_bus(root->segment, root->secondary.start))
+			return AE_OK;
+
+		printk(KERN_DEBUG "bus exists... trim\n");
+		/* this shouldn't be in here, so remove
+		 * the bus then re-add it...
+		 */
+		ret_val = acpi_bus_trim(device, 1);
+		printk(KERN_DEBUG "acpi_bus_trim return %x\n", ret_val);
+	}
+
+	ret_val = acpi_bus_add(&device, pdevice, handle, ACPI_BUS_TYPE_DEVICE);
+	if (ret_val) {
+		printk(KERN_ERR "error adding bus, %x\n", -ret_val);
+		goto out;
+	}
+
+	root = acpi_driver_data(device);
+	pci_assign_unassigned_bus_resources(root->bus);
+
+	ret_val = acpi_bus_start(device);
+
+out:
+	return ret_val;
+}
+
+void acpi_pci_root_rescan(void)
+{
+	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+		ACPI_UINT32_MAX, rescan_root_bridge, NULL, NULL, NULL);
+}
-- 
1.7.7

[PATCH 19/23] PCI: add /sys/bus/pci/rescan_root

[Yinghai Lu]

It will be used to rediscover removed pci root buses.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Randy Dunlap <rdunlap@xenotime.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-doc@vger.kernel.org
---
 Documentation/ABI/testing/sysfs-bus-pci |    9 +++++++++
 drivers/pci/pci-sysfs.c                 |   21 +++++++++++++++++++++
 2 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index 22392de..eb826bd 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -66,6 +66,15 @@ Description:
 		re-discover previously removed devices.
 		Depends on CONFIG_HOTPLUG.
 
+What:		/sys/bus/pci/rescan_root
+Date:		March 2012
+Contact:	Linux PCI developers <linux-pci@vger.kernel.org>
+Description:
+		Writing a non-zero value to this attribute will
+		cause a rescan of all PCI root buses in the system, and
+		re-discover previously removed PCI root buses.
+		Depends on CONFIG_HOTPLUG.
+
 What:		/sys/bus/pci/devices/.../msi_irqs/
 Date:		September, 2011
 Contact:	Neil Horman <nhorman@tuxdriver.com>
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index f855b4b..d6221e5 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -302,8 +302,29 @@ static ssize_t bus_rescan_store(struct bus_type *bus, const char *buf,
 	return count;
 }
 
+void __weak acpi_pci_root_rescan(void) { }
+void __weak pcibios_root_rescan(void) { }
+
+static ssize_t bus_rescan_root_store(struct bus_type *bus, const char *buf,
+				size_t count)
+{
+	unsigned long val;
+
+	if (strict_strtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	if (val) {
+		mutex_lock(&pci_remove_rescan_mutex);
+		acpi_pci_root_rescan();
+		pcibios_root_rescan();
+		mutex_unlock(&pci_remove_rescan_mutex);
+	}
+	return count;
+}
+
 struct bus_attribute pci_bus_attrs[] = {
 	__ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, bus_rescan_store),
+	__ATTR(rescan_root, (S_IWUSR|S_IWGRP), NULL, bus_rescan_root_store),
 	__ATTR_NULL
 };
 
-- 
1.7.7

[PATCH 20/23] PCI: add __pci_scan_root_bus() that can skip bus_add

[Yinghai Lu]

So could insert pus pci_assign_unassigned_bus_resources() before do bus_add.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/probe.c |   13 ++++++++++---
 include/linux/pci.h |    3 +++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6240d2c..708412a 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1953,8 +1953,9 @@ void pci_bus_release_busn_res(struct pci_bus *b)
 			res, ret ? "can not be" : "is");
 }
 
-struct pci_bus * __devinit pci_scan_root_bus(struct device *parent, int bus,
-		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+struct pci_bus * __devinit __pci_scan_root_bus(struct device *parent, int bus,
+		struct pci_ops *ops, void *sysdata, struct list_head *resources,
+		bool bus_add)
 {
 	struct pci_bus *b;
 	struct pci_host_bridge_window *window, *n;
@@ -1985,9 +1986,15 @@ struct pci_bus * __devinit pci_scan_root_bus(struct device *parent, int bus,
 	if (!found)
 		pci_bus_update_busn_res_end(b, b->subordinate);
 
-	pci_bus_add_devices(b);
+	if (bus_add)
+		pci_bus_add_devices(b);
 	return b;
 }
+struct pci_bus * __devinit pci_scan_root_bus(struct device *parent, int bus,
+		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+{
+	return __pci_scan_root_bus(parent, bus, ops, sysdata, resources, true);
+}
 EXPORT_SYMBOL(pci_scan_root_bus);
 
 /* Deprecated; use pci_scan_root_bus() instead */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 3d5feda..1efb80d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -675,6 +675,9 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 void pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
 void pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
 void pci_bus_release_busn_res(struct pci_bus *b);
+struct pci_bus * __devinit __pci_scan_root_bus(struct device *parent, int bus,
+		struct pci_ops *ops, void *sysdata, struct list_head *resources,
+		bool bus_add);
 struct pci_bus * __devinit pci_scan_root_bus(struct device *parent, int bus,
 					     struct pci_ops *ops, void *sysdata,
 					     struct list_head *resources);
-- 
1.7.7

[PATCH 21/23] x86, PCI: add __pci_scan_root_bus_on_node() that can skip bus_add

[Yinghai Lu]

So could insert pus pci_assign_unassigned_bus_resources() before do bus_add.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 arch/x86/include/asm/pci.h |    2 ++
 arch/x86/pci/common.c      |   11 ++++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h
index df75d07..bb015c7 100644
--- a/arch/x86/include/asm/pci.h
+++ b/arch/x86/include/asm/pci.h
@@ -24,6 +24,8 @@ extern int noioapicquirk;
 extern int noioapicreroute;
 
 /* scan a bus after allocating a pci_sysdata for it */
+struct pci_bus * __devinit __pci_scan_bus_on_node(int busno,
+			 struct pci_ops *ops, int node, bool bus_add);
 extern struct pci_bus *pci_scan_bus_on_node(int busno, struct pci_ops *ops,
 					    int node);
 extern struct pci_bus *pci_scan_bus_with_sysdata(int busno);
diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 35931f6..2ebdc231 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -625,7 +625,8 @@ static void release_pci_sysdata(struct pci_host_bridge *bridge)
 	kfree(sd);
 }
 
-struct pci_bus * __devinit pci_scan_bus_on_node(int busno, struct pci_ops *ops, int node)
+struct pci_bus * __devinit __pci_scan_bus_on_node(int busno,
+			 struct pci_ops *ops, int node, bool bus_add)
 {
 	LIST_HEAD(resources);
 	struct pci_bus *bus = NULL;
@@ -639,7 +640,7 @@ struct pci_bus * __devinit pci_scan_bus_on_node(int busno, struct pci_ops *ops,
 	sd->node = node;
 	x86_pci_root_bus_resources(busno, &resources);
 	printk(KERN_DEBUG "PCI: Probing PCI hardware (bus %02x)\n", busno);
-	bus = pci_scan_root_bus(NULL, busno, ops, sd, &resources);
+	bus = __pci_scan_root_bus(NULL, busno, ops, sd, &resources, bus_add);
 	if (bus)
 		pci_set_host_bridge_release(to_pci_host_bridge(bus->bridge),
 					release_pci_sysdata, sd);
@@ -650,7 +651,11 @@ struct pci_bus * __devinit pci_scan_bus_on_node(int busno, struct pci_ops *ops,
 
 	return bus;
 }
-
+struct pci_bus * __devinit pci_scan_bus_on_node(int busno,
+			 struct pci_ops *ops, int node)
+{
+	return __pci_scan_bus_on_node(busno, ops, node, true);
+}
 struct pci_bus * __devinit pci_scan_bus_with_sysdata(int busno)
 {
 	return pci_scan_bus_on_node(busno, &pci_root_ops, -1);
-- 
1.7.7

[PATCH 22/23] x86, PCI: add __pcibios_scan_specific_bus that can skip bus_add

[Yinghai Lu]

So could insert pci_assign_unassigned_bus_resources() before do bus_add.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 arch/x86/pci/legacy.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/arch/x86/pci/legacy.c b/arch/x86/pci/legacy.c
index a1df191..aab0e41 100644
--- a/arch/x86/pci/legacy.c
+++ b/arch/x86/pci/legacy.c
@@ -34,25 +34,32 @@ int __init pci_legacy_init(void)
 	return 0;
 }
 
-void __devinit pcibios_scan_specific_bus(int busn)
+static __devinit struct pci_bus *__pcibios_scan_specific_bus(int busn,
+					 bool bus_add)
 {
 	int devfn;
 	long node;
 	u32 l;
 
-	if (pci_find_bus(0, busn))
-		return;
-
 	node = get_mp_bus_to_node(busn);
 	for (devfn = 0; devfn < 256; devfn += 8) {
 		if (!raw_pci_read(0, busn, devfn, PCI_VENDOR_ID, 2, &l) &&
 		    l != 0x0000 && l != 0xffff) {
 			DBG("Found device at %02x:%02x [%04x]\n", busn, devfn, l);
 			printk(KERN_INFO "PCI: Discovered peer bus %02x\n", busn);
-			pci_scan_bus_on_node(busn, &pci_root_ops, node);
-			return;
+			return __pci_scan_bus_on_node(busn, &pci_root_ops, node,
+					 bus_add);
 		}
 	}
+
+	return NULL;
+}
+void __devinit pcibios_scan_specific_bus(int busn)
+{
+	if (pci_find_bus(0, busn))
+		return;
+
+	__pcibios_scan_specific_bus(busn, true);
 }
 EXPORT_SYMBOL_GPL(pcibios_scan_specific_bus);
 
-- 
1.7.7

[PATCH 23/23] x86, PCI: add pcibios_root_rescan

[Yinghai Lu]

need use it to rescan root bus that was not added via acpi probe.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 arch/x86/pci/legacy.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/arch/x86/pci/legacy.c b/arch/x86/pci/legacy.c
index aab0e41..7a7c78f 100644
--- a/arch/x86/pci/legacy.c
+++ b/arch/x86/pci/legacy.c
@@ -79,3 +79,28 @@ int __init pci_subsys_init(void)
 	return 0;
 }
 subsys_initcall(pci_subsys_init);
+
+void __devinit pcibios_root_rescan(void)
+{
+	int busn;
+	struct pci_bus *bus;
+
+	if (pcibios_last_bus <= 0 || pcibios_last_bus > 0xff)
+		return;
+
+	for (busn = 0; busn <= pcibios_last_bus; busn++) {
+		bus = pci_find_bus(0, busn);
+
+		if (bus)
+			continue;
+
+		bus = __pcibios_scan_specific_bus(busn, false);
+
+		if (!bus)
+			continue;
+
+		pci_assign_unassigned_bus_resources(bus);
+
+		pci_bus_add_devices(bus);
+	}
+}
-- 
1.7.7

Re: [PATCH 23/23] x86, PCI: add pcibios_root_rescan

[Bjorn Helgaas]

On Tue, Mar 6, 2012 at 12:14 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> need use it to rescan root bus that was not added via acpi probe.
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  arch/x86/pci/legacy.c |   25 +++++++++++++++++++++++++
>  1 files changed, 25 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/pci/legacy.c b/arch/x86/pci/legacy.c
> index aab0e41..7a7c78f 100644
> --- a/arch/x86/pci/legacy.c
> +++ b/arch/x86/pci/legacy.c
> @@ -79,3 +79,28 @@ int __init pci_subsys_init(void)
>        return 0;
>  }
>  subsys_initcall(pci_subsys_init);
> +
> +void __devinit pcibios_root_rescan(void)
> +{
> +       int busn;
> +       struct pci_bus *bus;
> +
> +       if (pcibios_last_bus <= 0 || pcibios_last_bus > 0xff)
> +               return;
> +
> +       for (busn = 0; busn <= pcibios_last_bus; busn++) {
> +               bus = pci_find_bus(0, busn);
> +
> +               if (bus)
> +                       continue;
> +
> +               bus = __pcibios_scan_specific_bus(busn, false);
> +
> +               if (!bus)
> +                       continue;
> +
> +               pci_assign_unassigned_bus_resources(bus);
> +
> +               pci_bus_add_devices(bus);
> +       }
> +}

Does anybody call this?

Do we need it?  I assume we only care about host bridge hotplug via ACPI on x86.

Please wait a while (at least a day or two) before reposting your
series so people have time to take a look at the current one.  It's a
real nuisance to be working through a large series like this and get
bombed with a repost of the whole thing that tweaks some minor detail.
 I'd rather take some time and get as much of the discussion out of
the way on V1 as possible before getting to V2, V3, V4, etc.

Bjorn

Re: [PATCH 23/23] x86, PCI: add pcibios_root_rescan

[Yinghai Lu]

On Tue, Mar 6, 2012 at 3:13 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Mar 6, 2012 at 12:14 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> need use it to rescan root bus that was not added via acpi probe.
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> ---
>>  arch/x86/pci/legacy.c |   25 +++++++++++++++++++++++++
>>  1 files changed, 25 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/x86/pci/legacy.c b/arch/x86/pci/legacy.c
>> index aab0e41..7a7c78f 100644
>> --- a/arch/x86/pci/legacy.c
>> +++ b/arch/x86/pci/legacy.c
>> @@ -79,3 +79,28 @@ int __init pci_subsys_init(void)
>>        return 0;
>>  }
>>  subsys_initcall(pci_subsys_init);
>> +
>> +void __devinit pcibios_root_rescan(void)
>> +{
>> +       int busn;
>> +       struct pci_bus *bus;
>> +
>> +       if (pcibios_last_bus <= 0 || pcibios_last_bus > 0xff)
>> +               return;
>> +
>> +       for (busn = 0; busn <= pcibios_last_bus; busn++) {
>> +               bus = pci_find_bus(0, busn);
>> +
>> +               if (bus)
>> +                       continue;
>> +
>> +               bus = __pcibios_scan_specific_bus(busn, false);
>> +
>> +               if (!bus)
>> +                       continue;
>> +
>> +               pci_assign_unassigned_bus_resources(bus);
>> +
>> +               pci_bus_add_devices(bus);
>> +       }
>> +}
>
> Does anybody call this?

yes, in

[PATCH 19/23] PCI: add /sys/bus/pci/rescan_root

>
> Do we need it?  I assume we only care about host bridge hotplug via ACPI on x86.

my sandbridge system has cpu bus 0x7f, and 0xff in DSDT.

but nehalem and westmere system does not have cpu bus 0xf8, ... 0xff.

>
> Please wait a while (at least a day or two) before reposting your
> series so people have time to take a look at the current one.  It's a
> real nuisance to be working through a large series like this and get
> bombed with a repost of the whole thing that tweaks some minor detail.
>  I'd rather take some time and get as much of the discussion out of
> the way on V1 as possible before getting to V2, V3, V4, etc.

Sure.

Re: [PATCH 23/23] x86, PCI: add pcibios_root_rescan

[Bjorn Helgaas]

[Sorry, gmail made this HTML, so LKML rejected it.  Sigh.  Please
respond to this one, not the HTML one.]

On Tue, Mar 6, 2012 at 5:09 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Mar 6, 2012 at 3:13 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Tue, Mar 6, 2012 at 12:14 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> need use it to rescan root bus that was not added via acpi probe.
>>>
>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>> ---
>>>  arch/x86/pci/legacy.c |   25 +++++++++++++++++++++++++
>>>  1 files changed, 25 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/x86/pci/legacy.c b/arch/x86/pci/legacy.c
>>> index aab0e41..7a7c78f 100644
>>> --- a/arch/x86/pci/legacy.c
>>> +++ b/arch/x86/pci/legacy.c
>>> @@ -79,3 +79,28 @@ int __init pci_subsys_init(void)
>>>        return 0;
>>>  }
>>>  subsys_initcall(pci_subsys_init);
>>> +
>>> +void __devinit pcibios_root_rescan(void)
>>> +{
>>> +       int busn;
>>> +       struct pci_bus *bus;
>>> +
>>> +       if (pcibios_last_bus <= 0 || pcibios_last_bus > 0xff)
>>> +               return;
>>> +
>>> +       for (busn = 0; busn <= pcibios_last_bus; busn++) {
>>> +               bus = pci_find_bus(0, busn);
>>> +
>>> +               if (bus)
>>> +                       continue;
>>> +
>>> +               bus = __pcibios_scan_specific_bus(busn, false);
>>> +
>>> +               if (!bus)
>>> +                       continue;
>>> +
>>> +               pci_assign_unassigned_bus_resources(bus);
>>> +
>>> +               pci_bus_add_devices(bus);
>>> +       }
>>> +}
>>
>> Do we need it?  I assume we only care about host bridge hotplug via ACPI on x86.
>
> my sandbridge system has cpu bus 0x7f, and 0xff in DSDT.
>
> but nehalem and westmere system does not have cpu bus 0xf8, ... 0xff.

I think you're saying that on some machines, the BIOS decided not to
expose host bridges leading to CPU devices, and you want to discover
those devices anyway.  (What's the reason you want to discover them?)

I guess that's OK for now, but I don't like this strategy in general.
The expectation of BIOS writers is that the OS will discover things
using the ACPI namespace and not go grubbing around outside that.
Sometimes the BIOS has good reasons to hide things from the OS, e.g.,
to keep the OS from tripping over hardware defects, to reduce support
calls by preventing users from making dangerous changes, etc.

Bjorn

Re: [PATCH 08/23] PNPACPI: Fix device ref leaking in acpi_pnp_match

[Bjorn Helgaas]

On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> During testing pci root bus removal, found some root bus bridge is not freed.
>
> If booting with pnpacpi=off, those hostbridge could be freed without problem.
>
> It turns out that some devices reference are not released during acpi_pnp_match.
>
> that match should not hold one device ref during every calling.
>
> Add put_device calling before returning.
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: stable@kernel.org
> Cc: Len Brown <lenb@kernel.org>
> Cc: Adam Belay <abelay@mit.edu>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-acpi@vger.kernel.org
> ---
>  drivers/pnp/pnpacpi/core.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
> index b00c176..d21e8f5 100644
> --- a/drivers/pnp/pnpacpi/core.c
> +++ b/drivers/pnp/pnpacpi/core.c
> @@ -321,9 +321,14 @@ static int __init acpi_pnp_match(struct device *dev, void *_pnp)
>  {
>        struct acpi_device *acpi = to_acpi_device(dev);
>        struct pnp_dev *pnp = _pnp;
> +       struct device *physical_device;
> +
> +       physical_device = acpi_get_physical_device(acpi->handle);
> +       if (physical_device)
> +               put_device(physical_device);
>
>        /* true means it matched */
> -       return !acpi_get_physical_device(acpi->handle)
> +       return !physical_device
>            && compare_pnp_id(pnp->id, acpi_device_hid(acpi));
>  }
>

I spent about an hour convincing myself that this patch does the right
thing.  I *think* it does, but it certainly is not obvious.  It's
always nicer if the get/put are in the same function or in functions
that obviously correspond to each other.  But you didn't create this
mess, so I don't hold you responsible for fixing it :)

Bjorn

Re: [PATCH 19/23] PCI: add /sys/bus/pci/rescan_root

[Bjorn Helgaas]

On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>
> It will be used to rediscover removed pci root buses.
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: Randy Dunlap <rdunlap@xenotime.net>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-doc@vger.kernel.org
> ---
>  Documentation/ABI/testing/sysfs-bus-pci |    9 +++++++++
>  drivers/pci/pci-sysfs.c                 |   21 +++++++++++++++++++++
>  2 files changed, 30 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci
> b/Documentation/ABI/testing/sysfs-bus-pci
> index 22392de..eb826bd 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -66,6 +66,15 @@ Description:
>                re-discover previously removed devices.
>                Depends on CONFIG_HOTPLUG.
>
> +What:          /sys/bus/pci/rescan_root
> +Date:          March 2012
> +Contact:       Linux PCI developers <linux-pci@vger.kernel.org>
> +Description:
> +               Writing a non-zero value to this attribute will
> +               cause a rescan of all PCI root buses in the system, and
> +               re-discover previously removed PCI root buses.

This seems sort of backwards to me.  Architecture-specific code
enumerated the host bridges/root buses in the first place, not the PCI
core.  Now we're asking the PCI core (/sys/bus/pci) to re-enumerate
them.

But maybe it's still the right answer, I dunno.

> +               Depends on CONFIG_HOTPLUG.
> +
>  What:          /sys/bus/pci/devices/.../msi_irqs/
>  Date:          September, 2011
>  Contact:       Neil Horman <nhorman@tuxdriver.com>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index f855b4b..d6221e5 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -302,8 +302,29 @@ static ssize_t bus_rescan_store(struct bus_type *bus,
> const char *buf,
>        return count;
>  }
>
> +void __weak acpi_pci_root_rescan(void) { }
> +void __weak pcibios_root_rescan(void) { }
> +
> +static ssize_t bus_rescan_root_store(struct bus_type *bus, const char
> *buf,
> +                               size_t count)
> +{
> +       unsigned long val;
> +
> +       if (strict_strtoul(buf, 0, &val) < 0)
> +               return -EINVAL;
> +
> +       if (val) {
> +               mutex_lock(&pci_remove_rescan_mutex);
> +               acpi_pci_root_rescan();

This architecture-dependent function name should not be here.  How
will we make this work for powerpc, sparc, etc.?

> +               pcibios_root_rescan();
> +               mutex_unlock(&pci_remove_rescan_mutex);
> +       }
> +       return count;
> +}
> +
>  struct bus_attribute pci_bus_attrs[] = {
>        __ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, bus_rescan_store),
> +       __ATTR(rescan_root, (S_IWUSR|S_IWGRP), NULL,
> bus_rescan_root_store),
>        __ATTR_NULL
>  };
>
> --
> 1.7.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 18/23] PCI, ACPI: add acpi_pci_root_rescan()

[Bjorn Helgaas]

On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> it will rescan all acpi pci root if related pci root bus get removed before.
>
> Signed-off-by: Yinghai <yinghai@kernel.org>
> Cc: Len Brown <lenb@kernel.org>
> Cc: linux-acpi@vger.kernel.org
> ---
>  drivers/acpi/pci_root.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 52 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index b38e347..e1814e0 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -679,3 +679,55 @@ static int __init acpi_pci_root_init(void)
>  }
>
>  subsys_initcall(acpi_pci_root_init);
> +
> +static acpi_status
> +rescan_root_bridge(acpi_handle handle, u32 lvl, void *context, void **rv)
> +{
> +       struct acpi_device *device, *pdevice;
> +       struct acpi_pci_root *root;
> +       acpi_handle phandle;
> +       int ret_val;
> +
> +       if (!acpi_is_root_bridge(handle))
> +               return AE_OK;
> +
> +       acpi_get_parent(handle, &phandle);
> +       if (acpi_bus_get_device(phandle, &pdevice)) {
> +               printk(KERN_DEBUG "no parent device, assuming NULL\n");

This printk (and the others below) does not have enough context.  A
user or developer will have no clue what it relates to.  Ideally they
would at least be dev_printk().

> +               pdevice = NULL;

Is a host bridge without a parent possible?  Seems dubious to me.

> +       }
> +
> +       if (!acpi_bus_get_device(handle, &device)) {
> +               /* check if  pci root_bus is removed */
> +               root = acpi_driver_data(device);
> +               if (pci_find_bus(root->segment, root->secondary.start))
> +                       return AE_OK;
> +
> +               printk(KERN_DEBUG "bus exists... trim\n");
> +               /* this shouldn't be in here, so remove
> +                * the bus then re-add it...
> +                */
> +               ret_val = acpi_bus_trim(device, 1);
> +               printk(KERN_DEBUG "acpi_bus_trim return %x\n", ret_val);
> +       }
> +
> +       ret_val = acpi_bus_add(&device, pdevice, handle, ACPI_BUS_TYPE_DEVICE);
> +       if (ret_val) {
> +               printk(KERN_ERR "error adding bus, %x\n", -ret_val);
> +               goto out;
> +       }
> +
> +       root = acpi_driver_data(device);
> +       pci_assign_unassigned_bus_resources(root->bus);
> +
> +       ret_val = acpi_bus_start(device);
> +
> +out:
> +       return ret_val;
> +}
> +
> +void acpi_pci_root_rescan(void)
> +{
> +       acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> +               ACPI_UINT32_MAX, rescan_root_bridge, NULL, NULL, NULL);
> +}

Walking the ACPI namespace is rarely the right thing to do.  Why do we
need it here?

Is this because we don't handle hotplug of ACPI devices in general?
The ACPI core *should* be noticing device removal and addition and
calling the driver .remove() and .add() methods directly.  I don't
think that's completely implemented, but I wish somebody would put
some effort into that rather than adding more hacky workarounds.

+cc Matthew Garrett, because I think he cares about this area too.

Re: [PATCH 00/23] PCI, x86: pci root bus hotplug support

[Bjorn Helgaas]

On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> will add pci_stop_and_remove_bus() to support remove bus in
> /sys/devices/pci.../pci_bus/...
>
> it support pci root bus removal.

Does this series support hot-add, too, or just removal?

If it is intended to support hot-add, I didn't see any support for
_CBA, which I *think* is the way hotplug-capable host bridges are
required to report MMCONFIG areas.

> To rescan root, need to
> echo 1 > /sys/bus/pci/rescan_root
>
> this patcheset include some IOMM and dmar and pnpacpi fix with device refcount leaking.
>
> The patches need to apply to pci/for-linus and pci/linux-next
> and [PATCH 00/36] PCI: pci_host_bridge related cleanup and busn_alloc
>
> could get from
>        git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-root-bus-hotplug
>
> Thanks
>
> Yinghai
>
> Yinghai Lu (23):
>  PCI, sys: Use device_type and attr_groups with pci dev
>  PCI, sysfs: create rescan_bridge under /sys/.../pci/devices/... for pci bridges
>  PCI: Add pci_bus_add_single_device()
>  PCI: make pci_rescan_bus_bridge_resize use pci_scan_bridge instead
>  PCI: clean up rescan_bus_bridge_resize
>  PCI: rescan with bus or bridge using callback method too
>  pci, dmar: Update dmar units devices list during hotplug
>  PNPACPI: Fix device ref leaking in acpi_pnp_match
>  IOMMU: Fix tboot force iommu logic
>  PCI, x86: Fix non acpi path pci_sysdata leaking with release_fn
>  PCI: separate out pci_assign_unassigned_bus_resources()
>  PCI: Move back pci_rescan_bus()
>  PCI: move pci_stop_and_remove_behind_bridge down
>  PCI: add __pci_remove_bus_devices()
>  PCI: add pci_stop_and_remove_bus()
>  PCI: add pci bus removal through /sys/.../pci_bus/.../remove
>  PCI, ACPI: make acpi_pci_root_remove remove pci root bus too
>  PCI, ACPI: add acpi_pci_root_rescan()
>  PCI: add /sys/bus/pci/rescan_root
>  PCI: add __pci_scan_root_bus() that can skip bus_add
>  x86, PCI: add __pci_scan_root_bus_on_node() that can skip bus_add
>  x86, PCI: add __pcibios_scan_specific_bus that can skip bus_add
>  x86, PCI: add pcibios_root_rescan
>
>  Documentation/ABI/testing/sysfs-bus-pci |   27 +++++
>  arch/x86/include/asm/pci.h              |    2 +
>  arch/x86/pci/common.c                   |   28 +++--
>  arch/x86/pci/legacy.c                   |   44 ++++++-
>  drivers/acpi/pci_root.c                 |   66 +++++++++++
>  drivers/iommu/intel-iommu.c             |  193 ++++++++++++++++++++++++++++--
>  drivers/pci/bus.c                       |   39 ++++++
>  drivers/pci/pci-sysfs.c                 |  137 ++++++++++++++++++++--
>  drivers/pci/pci.h                       |    1 +
>  drivers/pci/probe.c                     |   43 ++++++-
>  drivers/pci/remove.c                    |   77 ++++++++-----
>  drivers/pci/setup-bus.c                 |   20 +---
>  drivers/pnp/pnpacpi/core.c              |    7 +-
>  include/linux/dmar.h                    |    4 +
>  include/linux/pci.h                     |    6 +
>  15 files changed, 604 insertions(+), 90 deletions(-)
>
> --
> 1.7.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 23/23] x86, PCI: add pcibios_root_rescan

[Yinghai Lu]

On Tue, Mar 6, 2012 at 7:49 PM, Bjorn Helgaas <bjorn.helgaas@gmail.com> wrote:

>> my sandbridge system has cpu bus 0x7f, and 0xff in DSDT.
>>
>> but nehalem and westmere system does not have cpu bus 0xf8, ... 0xff.
>
> I think you're saying that on some machines, the BIOS decided not to
> expose host bridges leading to CPU devices, and you want to discover
> those devices anyway.  (What's the reason you want to discover them?)

current code for mmconf will set pcibios_last_bios according to mmconf
bus range.

so it legacy peer root bus fix up will discover those cpu buses.

then if some one remove them through
echo 1 > /sys/devices/pci0000:ff/pci_bus/0000:ff/remove

then

echo 1 > /sys/bus/pci/rescan_root

will not recover those cpu buses without this patch.

Yinghai

Re: [PATCH 19/23] PCI: add /sys/bus/pci/rescan_root

[Yinghai Lu]

On Tue, Mar 6, 2012 at 8:31 PM, Bjorn Helgaas <bjorn.helgaas@gmail.com> wrote:
> On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>
>> It will be used to rediscover removed pci root buses.
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> Cc: Randy Dunlap <rdunlap@xenotime.net>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: linux-doc@vger.kernel.org
>> ---
>>  Documentation/ABI/testing/sysfs-bus-pci |    9 +++++++++
>>  drivers/pci/pci-sysfs.c                 |   21 +++++++++++++++++++++
>>  2 files changed, 30 insertions(+), 0 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-pci
>> b/Documentation/ABI/testing/sysfs-bus-pci
>> index 22392de..eb826bd 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-pci
>> +++ b/Documentation/ABI/testing/sysfs-bus-pci
>> @@ -66,6 +66,15 @@ Description:
>>                re-discover previously removed devices.
>>                Depends on CONFIG_HOTPLUG.
>>
>> +What:          /sys/bus/pci/rescan_root
>> +Date:          March 2012
>> +Contact:       Linux PCI developers <linux-pci@vger.kernel.org>
>> +Description:
>> +               Writing a non-zero value to this attribute will
>> +               cause a rescan of all PCI root buses in the system, and
>> +               re-discover previously removed PCI root buses.
>
> This seems sort of backwards to me.  Architecture-specific code
> enumerated the host bridges/root buses in the first place, not the PCI
> core.  Now we're asking the PCI core (/sys/bus/pci) to re-enumerate
> them.
>
> But maybe it's still the right answer, I dunno.
>
>> +               Depends on CONFIG_HOTPLUG.
>> +
>>  What:          /sys/bus/pci/devices/.../msi_irqs/
>>  Date:          September, 2011
>>  Contact:       Neil Horman <nhorman@tuxdriver.com>
>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> index f855b4b..d6221e5 100644
>> --- a/drivers/pci/pci-sysfs.c
>> +++ b/drivers/pci/pci-sysfs.c
>> @@ -302,8 +302,29 @@ static ssize_t bus_rescan_store(struct bus_type *bus,
>> const char *buf,
>>        return count;
>>  }
>>
>> +void __weak acpi_pci_root_rescan(void) { }
>> +void __weak pcibios_root_rescan(void) { }
>> +
>> +static ssize_t bus_rescan_root_store(struct bus_type *bus, const char
>> *buf,
>> +                               size_t count)
>> +{
>> +       unsigned long val;
>> +
>> +       if (strict_strtoul(buf, 0, &val) < 0)
>> +               return -EINVAL;
>> +
>> +       if (val) {
>> +               mutex_lock(&pci_remove_rescan_mutex);
>> +               acpi_pci_root_rescan();
>
> This architecture-dependent function name should not be here.  How
> will we make this work for powerpc, sparc, etc.?
>
>> +               pcibios_root_rescan();
>> +               mutex_unlock(&pci_remove_rescan_mutex);

how about arch_pci_root_rescan(), and for x86 will let include
arch_pci_root_rescan()
pcibios_root_rescan().

need to check arch like powerpc,  sparc  to see if could
add pcibios_root_rescan()

Yinghai

Re: [PATCH 00/23] PCI, x86: pci root bus hotplug support

[Yinghai Lu]

On Tue, Mar 6, 2012 at 8:44 PM, Bjorn Helgaas <bjorn.helgaas@gmail.com> wrote:
> On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> Does this series support hot-add, too, or just removal?
>
> If it is intended to support hot-add, I didn't see any support for
> _CBA, which I *think* is the way hotplug-capable host bridges are
> required to report MMCONFIG areas.

to make the whole hotplug working with intel new cpus with IIO,
in addition to physical cpu and memory hotplug, will still need
a. ioapic controller hotplug
b. intr-remapping hotplug
c. intel-iommu hotplug
d. mmconf support.
e. root bus hotplug

this patch set is only involved with partitally root bus removal/add back.
and assume ioapic, intr-remap, intel-iommu, and mmconf related are
working from early initialization.

that will help us to help to fix bugs like devices refcount leaking.

Yinghai

Re: [PATCH 23/23] x86, PCI: add pcibios_root_rescan

[Bjorn Helgaas]

On Tue, Mar 6, 2012 at 11:29 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Mar 6, 2012 at 7:49 PM, Bjorn Helgaas <bjorn.helgaas@gmail.com> wrote:
>
>>> my sandbridge system has cpu bus 0x7f, and 0xff in DSDT.
>>>
>>> but nehalem and westmere system does not have cpu bus 0xf8, ... 0xff.
>>
>> I think you're saying that on some machines, the BIOS decided not to
>> expose host bridges leading to CPU devices, and you want to discover
>> those devices anyway.  (What's the reason you want to discover them?)
>
> current code for mmconf will set pcibios_last_bios according to mmconf
> bus range.
>
> so it legacy peer root bus fix up will discover those cpu buses.
>
> then if some one remove them through
> echo 1 > /sys/devices/pci0000:ff/pci_bus/0000:ff/remove
>
> then
>
> echo 1 > /sys/bus/pci/rescan_root
>
> will not recover those cpu buses without this patch.

What's the reason we need to discover those CPU devices in the first place?

I don't think the current policy of blindly probing for things should
be continued forever.

Re: [PATCH 16/23] PCI: add pci bus removal through /sys/.../pci_bus/.../remove

[Bjorn Helgaas]

On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> it supports both pci root bus and pci bus under pci bridge.
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: Randy Dunlap <rdunlap@xenotime.net>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-doc@vger.kernel.org
> ---
>  Documentation/ABI/testing/sysfs-bus-pci |    8 ++++++++
>  drivers/pci/pci-sysfs.c                 |   28 ++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index 95f0f37..22392de 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -92,6 +92,14 @@ Description:
>                hot-remove the PCI device and any of its children.
>                Depends on CONFIG_HOTPLUG.
>
> +What:          /sys/bus/pci/devices/.../pci_bus/.../remove
> +Date:          March 2012
> +Contact:       Linux PCI developers <linux-pci@vger.kernel.org>
> +Description:
> +               Writing a non-zero value to this attribute will
> +               hot-remove the PCI bus and any of its children.

Is this the interface we want?  It seems like the ultimate goal is to
remove the *host bridge*, i.e., the PNP0A08 device on x86.  If that's
the case, the logical thing seems like a
/sys/bus/acpi/devices/PNP0A08:00/remove file.

All the current "remove" attributes are for *devices*, e.g.,
/sys/devices/pci0000:00/0000:00:03.0/remove, not for *buses*.

I'm not sure it makes sense to talk about removing a "bus" and leaving
the upstream bridge (either a host bridge or a P2P bridge).  I think
it'd make more sense to remove the bridge itself, which would of
course have the consequence of removing the secondary bus.

> +               Depends on CONFIG_HOTPLUG.
> +
>  What:          /sys/bus/pci/devices/.../pci_bus/.../rescan
>  Date:          May 2011
>  Contact:       Linux PCI developers <linux-pci@vger.kernel.org>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 1794508..f855b4b 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -385,6 +385,33 @@ remove_store(struct device *dev, struct device_attribute *dummy,
>        return count;
>  }
>
> +static void bus_remove_callback(struct device *dev)
> +{
> +       struct pci_bus *bus = to_pci_bus(dev);
> +
> +       mutex_lock(&pci_remove_rescan_mutex);
> +       pci_stop_and_remove_bus(bus);
> +       mutex_unlock(&pci_remove_rescan_mutex);
> +}
> +static ssize_t
> +dev_bus_remove_store(struct device *dev, struct device_attribute *attr,
> +                const char *buf, size_t count)
> +{
> +       int ret = 0;
> +       unsigned long val;
> +
> +       if (kstrtoul(buf, 0, &val) < 0)
> +               return -EINVAL;
> +
> +       if (val)
> +               ret = device_schedule_callback(dev, bus_remove_callback);
> +
> +       if (ret)
> +               count = ret;
> +
> +       return count;
> +}
> +
>  static void bus_rescan_callback(struct device *dev)
>  {
>        struct pci_bus *bus = to_pci_bus(dev);
> @@ -444,6 +471,7 @@ struct device_attribute pci_dev_attrs[] = {
>  struct device_attribute pcibus_dev_attrs[] = {
>  #ifdef CONFIG_HOTPLUG
>        __ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store),
> +       __ATTR(remove, (S_IWUSR|S_IWGRP), NULL, dev_bus_remove_store),
>  #endif
>        __ATTR(cpuaffinity, S_IRUGO, pci_bus_show_cpumaskaffinity, NULL),
>        __ATTR(cpulistaffinity, S_IRUGO, pci_bus_show_cpulistaffinity, NULL),
> --
> 1.7.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 16/23] PCI: add pci bus removal through /sys/.../pci_bus/.../remove

[Yinghai Lu]

On Wed, Mar 7, 2012 at 4:03 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> it supports both pci root bus and pci bus under pci bridge.
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> Cc: Randy Dunlap <rdunlap@xenotime.net>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: linux-doc@vger.kernel.org
>> ---
>>  Documentation/ABI/testing/sysfs-bus-pci |    8 ++++++++
>>  drivers/pci/pci-sysfs.c                 |   28 ++++++++++++++++++++++++++++
>>  2 files changed, 36 insertions(+), 0 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
>> index 95f0f37..22392de 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-pci
>> +++ b/Documentation/ABI/testing/sysfs-bus-pci
>> @@ -92,6 +92,14 @@ Description:
>>                hot-remove the PCI device and any of its children.
>>                Depends on CONFIG_HOTPLUG.
>>
>> +What:          /sys/bus/pci/devices/.../pci_bus/.../remove
>> +Date:          March 2012
>> +Contact:       Linux PCI developers <linux-pci@vger.kernel.org>
>> +Description:
>> +               Writing a non-zero value to this attribute will
>> +               hot-remove the PCI bus and any of its children.
>
> Is this the interface we want?  It seems like the ultimate goal is to
> remove the *host bridge*, i.e., the PNP0A08 device on x86.  If that's
> the case, the logical thing seems like a
> /sys/bus/acpi/devices/PNP0A08:00/remove file.
>
> All the current "remove" attributes are for *devices*, e.g.,
> /sys/devices/pci0000:00/0000:00:03.0/remove, not for *buses*.

could add that later.

I am thinking to add some non-intrusive way to make notification work
for root-bus hot removal/add.
...

also in some case, cpu buses get scan during booting, could use that
to remove those not needed
root buses.

>
> I'm not sure it makes sense to talk about removing a "bus" and leaving
> the upstream bridge (either a host bridge or a P2P bridge).  I think
> it'd make more sense to remove the bridge itself, which would of
> course have the consequence of removing the secondary bus.

for root bus, that remove pci_host_bridge and pci root bus.

for pci bus under pci bridge, will remove that pci bus, but will still
keep  that pci bridge.
that should be ok. just like some pci bridge is there, and later can
not create child bus for it.

there is one case: during test busn_alloc, i need to remove all device
on one bus, and
use setpci to change bridge bus number register. then use rescan
bridge to create new bus.

with this one, I just need to remove that bus, instead of remove
children devices one by one.

Yinghai

Re: [PATCH 23/23] x86, PCI: add pcibios_root_rescan

[Yinghai Lu]

On Wed, Mar 7, 2012 at 3:32 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> What's the reason we need to discover those CPU devices in the first place?
>
> I don't think the current policy of blindly probing for things should
> be continued forever.

Do you want to revert follow commit?

BTW, edac never work on nehalem-ex and westmere-ex.

commit a3170c1f924ce2565c4e160b9b095e65c03b2dc6
Author: Jan Beulich <JBeulich@novell.com>
Date:   Wed Feb 23 10:08:10 2011 +0000

    x86/PCI: derive pcibios_last_bus from ACPI MCFG

    On various newer Intel systems the PCI bus(ses) the non-core devices
    live on aren't getting announced by ACPI except through the bus range
    covered by mmconfig. At least the i7core-edac driver depends on these
    devices getting detected.

    Mauro, could you check whether with this change the Xeon 55xx hack in
    that driver can go away altogether, and with it the bogus exporting of
    pcibios_scan_specific_bus()?

    Signed-off-by: Jan Beulich <jbeulich@novell.com>
    Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
    Cc: Aristeu Sergio <arozansk@redhat.com>
    Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index e282886..750c346 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -606,6 +606,16 @@ static void __init __pci_mmcfg_init(int early)
        if (list_empty(&pci_mmcfg_list))
                return;

+       if (pcibios_last_bus < 0) {
+               const struct pci_mmcfg_region *cfg;
+
+               list_for_each_entry(cfg, &pci_mmcfg_list, list) {
+                       if (cfg->segment)
+                               break;
+                       pcibios_last_bus = cfg->end_bus;
+               }
+       }
+
        if (pci_mmcfg_arch_init())
                pci_probe = (pci_probe & ~PCI_PROBE_MASK) | PCI_PROBE_MMCONF;
        else {

Re: [PATCH 23/23] x86, PCI: add pcibios_root_rescan

[Bjorn Helgaas]

On Wed, Mar 7, 2012 at 5:58 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Mar 7, 2012 at 3:32 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>> What's the reason we need to discover those CPU devices in the first place?
>>
>> I don't think the current policy of blindly probing for things should
>> be continued forever.
>
> Do you want to revert follow commit?
>
> BTW, edac never work on nehalem-ex and westmere-ex.

Oh, yes, now I remember.  Apparently the i7core-edac driver relies on
these devices.  We had a discussion about this a year ago:

    http://www.spinics.net/lists/linux-pci/msg10426.html

[+cc Jan and Mauro]

Personally, I think we should put a stake in the ground and say "For
machines newer than 2013, Linux will not blindly probe for PCI
devices.  If you want EDAC functionality, make sure your BIOS exposes
the appropriate PCI host bridges."  If OEMs do care about EDAC, it's a
simple BIOS change to do this.

I certainly don't think we need to add hotplug or bus rescan
functionality to cover this case.  This sort of stuff makes
maintenance MUCH harder because we have to worry about all these
corner cases.

Bjorn

> commit a3170c1f924ce2565c4e160b9b095e65c03b2dc6
> Author: Jan Beulich <JBeulich@novell.com>
> Date:   Wed Feb 23 10:08:10 2011 +0000
>
>    x86/PCI: derive pcibios_last_bus from ACPI MCFG
>
>    On various newer Intel systems the PCI bus(ses) the non-core devices
>    live on aren't getting announced by ACPI except through the bus range
>    covered by mmconfig. At least the i7core-edac driver depends on these
>    devices getting detected.
>
>    Mauro, could you check whether with this change the Xeon 55xx hack in
>    that driver can go away altogether, and with it the bogus exporting of
>    pcibios_scan_specific_bus()?
>
>    Signed-off-by: Jan Beulich <jbeulich@novell.com>
>    Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
>    Cc: Aristeu Sergio <arozansk@redhat.com>
>    Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>
> diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
> index e282886..750c346 100644
> --- a/arch/x86/pci/mmconfig-shared.c
> +++ b/arch/x86/pci/mmconfig-shared.c
> @@ -606,6 +606,16 @@ static void __init __pci_mmcfg_init(int early)
>        if (list_empty(&pci_mmcfg_list))
>                return;
>
> +       if (pcibios_last_bus < 0) {
> +               const struct pci_mmcfg_region *cfg;
> +
> +               list_for_each_entry(cfg, &pci_mmcfg_list, list) {
> +                       if (cfg->segment)
> +                               break;
> +                       pcibios_last_bus = cfg->end_bus;
> +               }
> +       }
> +
>        if (pci_mmcfg_arch_init())
>                pci_probe = (pci_probe & ~PCI_PROBE_MASK) | PCI_PROBE_MMCONF;
>        else {

Re: [PATCH 16/23] PCI: add pci bus removal through /sys/.../pci_bus/.../remove

[Bjorn Helgaas]

[+cc Matthew Garrett, linux-acpi for ACPI hotplug]

On Wed, Mar 7, 2012 at 5:53 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Mar 7, 2012 at 4:03 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> it supports both pci root bus and pci bus under pci bridge.
>>>
>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>> Cc: Randy Dunlap <rdunlap@xenotime.net>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: linux-doc@vger.kernel.org
>>> ---
>>>  Documentation/ABI/testing/sysfs-bus-pci |    8 ++++++++
>>>  drivers/pci/pci-sysfs.c                 |   28 ++++++++++++++++++++++++++++
>>>  2 files changed, 36 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
>>> index 95f0f37..22392de 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-pci
>>> +++ b/Documentation/ABI/testing/sysfs-bus-pci
>>> @@ -92,6 +92,14 @@ Description:
>>>                hot-remove the PCI device and any of its children.
>>>                Depends on CONFIG_HOTPLUG.
>>>
>>> +What:          /sys/bus/pci/devices/.../pci_bus/.../remove
>>> +Date:          March 2012
>>> +Contact:       Linux PCI developers <linux-pci@vger.kernel.org>
>>> +Description:
>>> +               Writing a non-zero value to this attribute will
>>> +               hot-remove the PCI bus and any of its children.
>>
>> Is this the interface we want?  It seems like the ultimate goal is to
>> remove the *host bridge*, i.e., the PNP0A08 device on x86.  If that's
>> the case, the logical thing seems like a
>> /sys/bus/acpi/devices/PNP0A08:00/remove file.
>>
>> All the current "remove" attributes are for *devices*, e.g.,
>> /sys/devices/pci0000:00/0000:00:03.0/remove, not for *buses*.
>
> could add that later.

Of course, we can always add things later.  What I'm concerned about
is that once we add an interface like this to sysfs, it's difficult to
remove.  Therefore, I don't want to add things that are clearly not
what we really want.

> I am thinking to add some non-intrusive way to make notification work
> for root-bus hot removal/add.

The Way Things Are Supposed To Work is that ACPI notifies the OS that
a device (a PNP0A08 host bridge in this case) is being removed.  The
OS driver does its cleanup and lets go of the device.  There are
several things in the ACPI part of that path that are not implemented
yet, so I understand that we might need a sysfs knob to kick things
off.  But can't we make a knob that just calls the host bridge driver
.remove() method, i.e., acpi_pci_root_remove()?

> also in some case, cpu buses get scan during booting, could use that
> to remove those not needed
> root buses.

I don't see the value in this.  I think you're talking about removing
the devices we discovered by blindly probing, i.e., the ones the BIOS
didn't tell us about.  I do think we should stop blindly probing for
them, but I don't see what we gain by adding a sysfs knob to remove
them.

>> I'm not sure it makes sense to talk about removing a "bus" and leaving
>> the upstream bridge (either a host bridge or a P2P bridge).  I think
>> it'd make more sense to remove the bridge itself, which would of
>> course have the consequence of removing the secondary bus.
>
> for root bus, that remove pci_host_bridge and pci root bus.
>
> for pci bus under pci bridge, will remove that pci bus, but will still
> keep  that pci bridge.
> that should be ok. just like some pci bridge is there, and later can
> not create child bus for it.
>
> there is one case: during test busn_alloc, i need to remove all device
> on one bus, and
> use setpci to change bridge bus number register. then use rescan
> bridge to create new bus.
>
> with this one, I just need to remove that bus, instead of remove
> children devices one by one.

I don't think making it convenient for manual testing is an argument
for this interface.  For sysfs interfaces it is more important to make
something that fits well into the grand plan of how things Should
Work.  If you need internal helper functions for convenience, I'm OK
with that, because it's easier to change those than to change sysfs
interfaces.

Bjorn

Re: [PATCH 23/23] x86, PCI: add pcibios_root_rescan

[Jan Beulich]

>>> On 08.03.12 at 05:27, Bjorn Helgaas <bhelgaas@google.com> wrote:
> Personally, I think we should put a stake in the ground and say "For
> machines newer than 2013, Linux will not blindly probe for PCI
> devices.  If you want EDAC functionality, make sure your BIOS exposes
> the appropriate PCI host bridges."  If OEMs do care about EDAC, it's a
> simple BIOS change to do this.
> 
> I certainly don't think we need to add hotplug or bus rescan
> functionality to cover this case.  This sort of stuff makes
> maintenance MUCH harder because we have to worry about all these
> corner cases.

I'm certainly fine with such a position, as long as this is made visible as
a policy and the other hack in the i7core edac driver to find non-
exposed devices gets removed as well. It was really the existing hack
(which worked only on a certain subset of systems) that made me try
find a more generic solution.

Jan

Re: [PATCH 16/23] PCI: add pci bus removal through /sys/.../pci_bus/.../remove

[Greg Kroah-Hartman]

On Wed, Mar 07, 2012 at 09:45:18PM -0700, Bjorn Helgaas wrote:
> >> I'm not sure it makes sense to talk about removing a "bus" and leaving
> >> the upstream bridge (either a host bridge or a P2P bridge).  I think
> >> it'd make more sense to remove the bridge itself, which would of
> >> course have the consequence of removing the secondary bus.
> >
> > for root bus, that remove pci_host_bridge and pci root bus.
> >
> > for pci bus under pci bridge, will remove that pci bus, but will still
> > keep  that pci bridge.
> > that should be ok. just like some pci bridge is there, and later can
> > not create child bus for it.
> >
> > there is one case: during test busn_alloc, i need to remove all device
> > on one bus, and
> > use setpci to change bridge bus number register. then use rescan
> > bridge to create new bus.
> >
> > with this one, I just need to remove that bus, instead of remove
> > children devices one by one.
> 
> I don't think making it convenient for manual testing is an argument
> for this interface.  For sysfs interfaces it is more important to make
> something that fits well into the grand plan of how things Should
> Work.  If you need internal helper functions for convenience, I'm OK
> with that, because it's easier to change those than to change sysfs
> interfaces.

If it's "only" for testing, then put it in debugfs, not sysfs.

thanks,

greg k-h

Re: [PATCH 00/23] PCI, x86: pci root bus hotplug support

[Bjorn Helgaas]

On Tue, Mar 6, 2012 at 11:58 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Mar 6, 2012 at 8:44 PM, Bjorn Helgaas <bjorn.helgaas@gmail.com> wrote:
>> On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> Does this series support hot-add, too, or just removal?
>>
>> If it is intended to support hot-add, I didn't see any support for
>> _CBA, which I *think* is the way hotplug-capable host bridges are
>> required to report MMCONFIG areas.
>
> to make the whole hotplug working with intel new cpus with IIO,
> in addition to physical cpu and memory hotplug, will still need
> a. ioapic controller hotplug
> b. intr-remapping hotplug
> c. intel-iommu hotplug
> d. mmconf support.
> e. root bus hotplug
>
> this patch set is only involved with partitally root bus removal/add back.
> and assume ioapic, intr-remap, intel-iommu, and mmconf related are
> working from early initialization.
>
> that will help us to help to fix bugs like devices refcount leaking.

OK, so this series only supports host bridge removal.  There are some
situations where it allows a bridge to be added, but only if the other
things you mentioned (ioapic, intr-remapping, etc.) are already
present, probably because we didn't remove them when removing the host
bridge.

In the case of MMCONFIG in particular, I think this should be moved
into the host bridge add path, e.g., in acpi_pci_root_add().  The fact
that it's totally separate today is a mistake.  But that can be fixed
later.

Overall nits, because kernel development is obsessive by nature:
Please format your changelogs with textwidth=75, so that when you read
them with "git log" in an 80-column terminal, the normal text doesn't
wrap across a line ending.

Please make your subject lines lines consistent in style and
capitalization.  Sometimes you have "PCI", sometimes "pci".  Sometimes
the first word after "PCI: " is capitalized, sometimes not.  One of
your intel-iommu.c patches is "IOMMU:"; the other is "pci, dmar:" (and
these patches are not adjacent in the series, even though they could
be).  This is just sloppy and distracting.

In English, the first word of a sentence is capitalized.  Words in the
middle are not, unless they are acronyms or proper nouns.  It would be
helpful if you could follow these conventions in your changelogs,
because I expect to be reading them for years to come, and I'm easily
distracted.

The notes about "-v2, -v3, -v4, etc." are not really useful in the
commit changelogs.  They're helpful in the [00/] message, but not in
the changelogs.

I appreciate the effort you're making to write changelogs.  They're
definitely starting to contain some useful information.

Bjorn

Re: [PATCH 02/23] PCI, sysfs: create rescan_bridge under /sys/.../pci/devices/... for pci bridges

[Bjorn Helgaas]

On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> Current code will create rescan for every pci device under parent bus.
> that is not right. the device is already there, there is no reason to rescan it.
>
> We could have rescan for pci bridges. less confusing.

Yes, but now we have *three* rescan attributes for a single bridge/bus:

    /sys/devices/pci0000:00/0000:00:01.0/pci_bus/0000:01/rescan
    /sys/devices/pci0000:00/0000:00:01.0/rescan_bridge
    /sys/devices/pci0000:00/0000:00:01.0/rescan

and this for an endpoint:

    /sys/devices/pci0000:00/0000:00:05.0/rescan

I think endpoints should not have a "rescan" attribute at all, and
bridges should have only a "rescan" attribute (not "rescan" and
"rescan_bridge").

I'm not sure about "pci_bus/.../rescan".  I know you didn't add that,
but I'm not sure it makes sense to have both that and a "rescan" on
the bridge device.  If we have both, what's the difference between
them?

> Need to move rescan attr to pci dev bridge attribute group.
> And We should rescan bridge's secondary bus instead of primary bus.
>
> -v3: Use device_type for pci dev.
> -v4: Seperate pci device type change out
> -v5: add rescan_bridge for bridge type, and still keep the old rescan.
>        may remove the old rescan later.
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: Randy Dunlap <rdunlap@xenotime.net>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  Documentation/ABI/testing/sysfs-bus-pci |   10 ++++++++++
>  drivers/pci/pci-sysfs.c                 |   24 ++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index 34f5110..95f0f37 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -111,6 +111,16 @@ Description:
>                from this part of the device tree.
>                Depends on CONFIG_HOTPLUG.
>
> +What:          /sys/bus/pci/devices/.../rescan_bridge
> +Date:          February 2012
> +Contact:       Linux PCI developers <linux-pci@vger.kernel.org>
> +Description:
> +               Writing a non-zero value to this attribute will
> +               force a rescan of the bridge and all child buses, and
> +               re-discover devices removed earlier from this part of
> +               the device tree.
> +               Depends on CONFIG_HOTPLUG.
> +
>  What:          /sys/bus/pci/devices/.../reset
>  Date:          July 2009
>  Contact:       Michael S. Tsirkin <mst@redhat.com>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 39d15e6..d5c8ffb 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -325,6 +325,27 @@ dev_rescan_store(struct device *dev, struct device_attribute *attr,
>        return count;
>  }
>
> +static ssize_t
> +dev_bridge_rescan_store(struct device *dev, struct device_attribute *attr,
> +                const char *buf, size_t count)
> +{
> +       unsigned long val;
> +       struct pci_dev *pdev = to_pci_dev(dev);
> +
> +       if (kstrtoul(buf, 0, &val) < 0)
> +               return -EINVAL;
> +
> +       if (val) {
> +               mutex_lock(&pci_remove_rescan_mutex);
> +               pci_rescan_bus(pdev->subordinate);
> +               mutex_unlock(&pci_remove_rescan_mutex);
> +       }
> +       return count;
> +}
> +
> +static struct device_attribute pci_dev_bridge_rescan_attr =
> +       __ATTR(rescan_bridge, (S_IWUSR|S_IWGRP), NULL, dev_bridge_rescan_store);
> +
>  static void remove_callback(struct device *dev)
>  {
>        struct pci_dev *pdev = to_pci_dev(dev);
> @@ -1337,6 +1358,9 @@ static int __init pci_sysfs_init(void)
>  late_initcall(pci_sysfs_init);
>
>  static struct attribute *pci_dev_bridge_attrs[] = {
> +#ifdef CONFIG_HOTPLUG
> +       &pci_dev_bridge_rescan_attr.attr,
> +#endif
>        NULL,
>  };
>
> --
> 1.7.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/23] PCI: rescan with bus or bridge using callback method too

[Bjorn Helgaas]

On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> Just like removal.
>
> Because We could add new bus under the bridges...
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/pci-sysfs.c |   43 +++++++++++++++++++++++++++++++------------
>  1 files changed, 31 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 2049b2f..1794508 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -325,21 +325,31 @@ dev_rescan_store(struct device *dev, struct device_attribute *attr,
>        return count;
>  }
>
> +static void bridge_rescan_callback(struct device *dev)
> +{
> +       struct pci_dev *pdev = to_pci_dev(dev);
> +
> +       mutex_lock(&pci_remove_rescan_mutex);
> +       pci_rescan_bus_bridge_resize(pdev);
> +       mutex_unlock(&pci_remove_rescan_mutex);
> +}
> +
>  static ssize_t
>  dev_bridge_rescan_store(struct device *dev, struct device_attribute *attr,
>                 const char *buf, size_t count)
>  {
> +       int ret = 0;
>        unsigned long val;
> -       struct pci_dev *pdev = to_pci_dev(dev);
>
>        if (kstrtoul(buf, 0, &val) < 0)
>                return -EINVAL;
>
> -       if (val) {
> -               mutex_lock(&pci_remove_rescan_mutex);
> -               pci_rescan_bus_bridge_resize(pdev);
> -               mutex_unlock(&pci_remove_rescan_mutex);
> -       }
> +       if (val)
> +               ret = device_schedule_callback(dev, bridge_rescan_callback);
> +
> +       if (ret)
> +               count = ret;
> +
>        return count;

I see that you copied this style from remove_store(), but it's ugly
and hard to follow.  I think this would be more readable:

    if (!val)
        return count;

    err = device_schedule_callback(dev, bridge_rescan_callback);
    if (err)
        return err;

    return count;

>  }
>
> @@ -375,21 +385,30 @@ remove_store(struct device *dev, struct device_attribute *dummy,
>        return count;
>  }
>
> +static void bus_rescan_callback(struct device *dev)
> +{
> +       struct pci_bus *bus = to_pci_bus(dev);
> +
> +       mutex_lock(&pci_remove_rescan_mutex);
> +       pci_rescan_bus(bus);
> +       mutex_unlock(&pci_remove_rescan_mutex);
> +}
>  static ssize_t
>  dev_bus_rescan_store(struct device *dev, struct device_attribute *attr,
>                 const char *buf, size_t count)
>  {
> +       int ret = 0;
>        unsigned long val;
> -       struct pci_bus *bus = to_pci_bus(dev);
>
>        if (strict_strtoul(buf, 0, &val) < 0)
>                return -EINVAL;
>
> -       if (val) {
> -               mutex_lock(&pci_remove_rescan_mutex);
> -               pci_rescan_bus(bus);
> -               mutex_unlock(&pci_remove_rescan_mutex);
> -       }
> +       if (val)
> +               ret = device_schedule_callback(dev, bus_rescan_callback);
> +
> +       if (ret)
> +               count = ret;
> +
>        return count;
>  }
>
> --
> 1.7.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 07/23] pci, dmar: Update dmar units devices list during hotplug

[Bjorn Helgaas]

On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> When do pci remove/rescan on system that have more iommus, got
>
> [  894.089745] Set context mapping for c4:00.0
> [  894.110890] mpt2sas3: Allocated physical memory: size(4293 kB)
> [  894.112556] mpt2sas3: Current Controller Queue Depth(1883), Max Controller Queue Depth(2144)
> [  894.127278] mpt2sas3: Scatter Gather Elements per IO(128)
> [  894.361295] DRHD: handling fault status reg 2
> [  894.364053] DMAR:[DMA Read] Request device [c4:00.0] fault addr fffbe000
> [  894.364056] DMAR:[fault reason 02] Present bit in context entry is cl
>
> it turns out when remove/rescan, pci dev will be freed and will get another new dev.
> but drhd units still keep old one... so dmar_find_matched_drhd_unit will
> return wrong drhd and iommu for the device that is not on first iommu.
>
> So need to update devices in drhd_units during pci remove/rescan.
>
> Could save domain/bus/device/function aside in the list and according that info
> restore new dev to drhd_units later.
> Then dmar_find_matched_drdh_unit and device_to_iommu could return right drhd and iommu.
>
> Add remove_dev_from_drhd/restore_dev_to_drhd functions to do the real work.
> call them in device ADD_DEVICE and UNBOUND_DRIVER
>
> Need to do the samething to atsr.  (expose dmar_atsr_units and add atsru->segment)
>
> After patch, will right iommu for the new dev and will not get DMAR error any more.
>
> -v2: add pci_dev_put/pci_dev_get to make refcount consistent.
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Vinod Koul <vinod.koul@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: iommu@lists.linux-foundation.org
> ---
>  drivers/iommu/intel-iommu.c |  187 ++++++++++++++++++++++++++++++++++++++++---
>  include/linux/dmar.h        |    4 +
>  2 files changed, 181 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index c9c6053..39ef2ce 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -665,6 +665,159 @@ static struct intel_iommu *device_to_iommu(int segment, u8 bus, u8 devfn)
>        return NULL;
>  }
>
> +#ifdef CONFIG_HOTPLUG
> +struct dev_dmaru {
> +       struct list_head list;
> +       void *dmaru;
> +       int index;
> +       int segment;
> +       unsigned char bus;
> +       unsigned int devfn;
> +};
> +
> +static int
> +save_dev_dmaru(int segment, unsigned char bus, unsigned int devfn,
> +                void *dmaru, int index, struct list_head *lh)

Follow the indentation style used elsewhere in the file: as much of
the declaration on one line as will fit (here and below).

I think the callers would read more naturally if you passed in the
struct pci_dev * to save_dev_dmaru() and extracted the
segment/bus/devfn here rather than in the callers.

> +{
> +       struct dev_dmaru *m;
> +
> +       m = kzalloc(sizeof(*m), GFP_KERNEL);
> +       if (!m)
> +               return -ENOMEM;
> +
> +       m->segment = segment;
> +       m->bus     = bus;
> +       m->devfn   = devfn;
> +       m->dmaru   = dmaru;
> +       m->index   = index;
> +
> +       list_add(&m->list, lh);
> +
> +       return 0;
> +}
> +
> +static void
> +*get_dev_dmaru(int segment, unsigned char bus, unsigned int devfn,
> +               int *index, struct list_head *lh)
> +{
> +       struct dev_dmaru *m;
> +       void *dmaru = NULL;
> +
> +       list_for_each_entry(m, lh, list) {
> +               if (m->segment == segment &&
> +                   m->bus == bus && m->devfn == devfn) {
> +                       *index = m->index;
> +                       dmaru  = m->dmaru;
> +                       list_del(&m->list);
> +                       kfree(m);
> +                       break;
> +               }
> +       }
> +
> +       return dmaru;
> +}
> +
> +static LIST_HEAD(saved_dev_drhd_list);
> +
> +static void remove_dev_from_drhd(struct pci_dev *dev)
> +{
> +       struct dmar_drhd_unit *drhd = NULL;
> +       int segment = pci_domain_nr(dev->bus);
> +       int i;
> +
> +       for_each_drhd_unit(drhd) {
> +               if (drhd->ignored)
> +                       continue;
> +               if (segment != drhd->segment)
> +                       continue;
> +
> +               for (i = 0; i < drhd->devices_cnt; i++) {
> +                       if (drhd->devices[i] == dev) {
> +                               /* save it at first if it is in drhd */
> +                               save_dev_dmaru(segment, dev->bus->number,
> +                                               dev->devfn, drhd, i,
> +                                               &saved_dev_drhd_list);
> +                               /* always remove it */
> +                               pci_dev_put(dev);
> +                               drhd->devices[i] = NULL;
> +                               return;
> +                       }
> +               }
> +       }
> +}
> +
> +static void restore_dev_to_drhd(struct pci_dev *dev)
> +{
> +       struct dmar_drhd_unit *drhd = NULL;
> +       int i;
> +
> +       /* find the stored drhd */
> +       drhd = get_dev_dmaru(pci_domain_nr(dev->bus), dev->bus->number,
> +                                dev->devfn, &i, &saved_dev_drhd_list);
> +       /* restore that into drhd */
> +       if (drhd)
> +               drhd->devices[i] = pci_dev_get(dev);
> +}
> +#else
> +static void remove_dev_from_drhd(struct pci_dev *dev)
> +{
> +}
> +
> +static void restore_dev_to_drhd(struct pci_dev *dev)
> +{
> +}
> +#endif
> +
> +#if defined(CONFIG_DMAR) && defined(CONFIG_HOTPLUG)
> +static LIST_HEAD(saved_dev_atsr_list);
> +
> +static void remove_dev_from_atsr(struct pci_dev *dev)
> +{
> +       struct dmar_atsr_unit *atsr = NULL;
> +       int segment = pci_domain_nr(dev->bus);
> +       int i;
> +
> +       for_each_atsr_unit(atsr) {
> +               if (segment != atsr->segment)
> +                       continue;
> +
> +               for (i = 0; i < atsr->devices_cnt; i++) {
> +                       if (atsr->devices[i] == dev) {
> +                               /* save it at first if it is in drhd */
> +                               save_dev_dmaru(segment, dev->bus->number,
> +                                               dev->devfn, atsr, i,
> +                                               &saved_dev_atsr_list);
> +                               /* always remove it */
> +                               pci_dev_put(dev);
> +                               atsr->devices[i] = NULL;
> +                               return;
> +                       }
> +               }
> +       }
> +}
> +
> +static void restore_dev_to_atsr(struct pci_dev *dev)
> +{
> +       struct dmar_atsr_unit *atsr = NULL;
> +       int i;
> +
> +       /* find the stored atsr */
> +       atsr = get_dev_dmaru(pci_domain_nr(dev->bus), dev->bus->number,
> +                                dev->devfn, &i, &saved_dev_atsr_list);
> +       /* restore that into atsr */
> +       if (atsr)
> +               atsr->devices[i] = pci_dev_get(dev);
> +}
> +#else
> +static void remove_dev_from_atsr(struct pci_dev *dev)
> +{
> +}
> +
> +static void restore_dev_to_atsr(struct pci_dev *dev)
> +{
> +}
> +#endif
> +
>  static void domain_flush_cache(struct dmar_domain *domain,
>                               void *addr, int size)
>  {
> @@ -3474,6 +3627,7 @@ int __init dmar_parse_one_atsr(struct acpi_dmar_header *hdr)
>
>        atsru->hdr = hdr;
>        atsru->include_all = atsr->flags & 0x1;
> +       atsru->segment = atsr->segment;
>
>        list_add(&atsru->list, &dmar_atsr_units);
>
> @@ -3505,16 +3659,13 @@ int dmar_find_matched_atsr_unit(struct pci_dev *dev)
>  {
>        int i;
>        struct pci_bus *bus;
> -       struct acpi_dmar_atsr *atsr;
>        struct dmar_atsr_unit *atsru;
>
>        dev = pci_physfn(dev);
>
> -       list_for_each_entry(atsru, &dmar_atsr_units, list) {
> -               atsr = container_of(atsru->hdr, struct acpi_dmar_atsr, header);
> -               if (atsr->segment == pci_domain_nr(dev->bus))
> +       list_for_each_entry(atsru, &dmar_atsr_units, list)
> +               if (atsru->segment == pci_domain_nr(dev->bus))
>                        goto found;
> -       }
>
>        return 0;
>
> @@ -3574,20 +3725,36 @@ static int device_notifier(struct notifier_block *nb,
>        struct pci_dev *pdev = to_pci_dev(dev);
>        struct dmar_domain *domain;
>
> -       if (iommu_no_mapping(dev))
> +       if (unlikely(dev->bus != &pci_bus_type))
>                return 0;
>
> -       domain = find_domain(pdev);
> -       if (!domain)
> -               return 0;
> +       switch (action) {
> +       case BUS_NOTIFY_UNBOUND_DRIVER:
> +               if (iommu_no_mapping(dev))
> +                       goto out;
> +
> +               if (iommu_pass_through)
> +                       goto out;
> +
> +               domain = find_domain(pdev);
> +               if (!domain)
> +                       goto out;
>
> -       if (action == BUS_NOTIFY_UNBOUND_DRIVER && !iommu_pass_through) {
>                domain_remove_one_dev_info(domain, pdev);
>
>                if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) &&
>                    !(domain->flags & DOMAIN_FLAG_STATIC_IDENTITY) &&
>                    list_empty(&domain->devices))
>                        domain_exit(domain);
> +out:
> +               remove_dev_from_drhd(pdev);
> +               remove_dev_from_atsr(pdev);
> +
> +               break;
> +       case BUS_NOTIFY_ADD_DEVICE:
> +               restore_dev_to_drhd(pdev);
> +               restore_dev_to_atsr(pdev);
> +               break;
>        }
>
>        return 0;
> diff --git a/include/linux/dmar.h b/include/linux/dmar.h
> index 731a609..57e1375 100644
> --- a/include/linux/dmar.h
> +++ b/include/linux/dmar.h
> @@ -236,9 +236,13 @@ struct dmar_atsr_unit {
>        struct acpi_dmar_header *hdr;   /* ACPI header */
>        struct pci_dev **devices;       /* target devices */
>        int devices_cnt;                /* target device count */
> +       u16 segment;                    /* PCI domain */
>        u8 include_all:1;               /* include all ports */
>  };
>
> +#define for_each_atsr_unit(atsr) \
> +       list_for_each_entry(atsr, &dmar_atsr_units, list)
> +
>  int dmar_parse_rmrr_atsr_dev(void);
>  extern int dmar_parse_one_rmrr(struct acpi_dmar_header *header);
>  extern int dmar_parse_one_atsr(struct acpi_dmar_header *header);
> --
> 1.7.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/23] PCI: separate out pci_assign_unassigned_bus_resources()

[Bjorn Helgaas]

On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> it is main portion of pci_rescan_bus().
>
> Separate it out and will use it later for pci root bus rescan.
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/setup-bus.c |   34 ++++++++++++++++++++--------------
>  include/linux/pci.h     |    1 +
>  2 files changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 8fa2d4b..6a66139 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1488,25 +1488,12 @@ enable_all:
>  }
>  EXPORT_SYMBOL_GPL(pci_assign_unassigned_bridge_resources);
>
> -#ifdef CONFIG_HOTPLUG
> -/**
> - * pci_rescan_bus - scan a PCI bus for devices.
> - * @bus: PCI bus to scan
> - *
> - * Scan a PCI bus and child buses for new devices, adds them,
> - * and enables them.
> - *
> - * Returns the max number of subordinate bus discovered.
> - */
> -unsigned int __ref pci_rescan_bus(struct pci_bus *bus)
> +void pci_assign_unassigned_bus_resources(struct pci_bus *bus)
>  {
> -       unsigned int max;
>        struct pci_dev *dev;
>        LIST_HEAD(add_list); /* list of resources that
>                                        want additional resources */
>
> -       max = pci_scan_child_bus(bus);
> -
>        down_read(&pci_bus_sem);
>        list_for_each_entry(dev, &bus->devices, bus_list)
>                if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||
> @@ -1519,6 +1506,25 @@ unsigned int __ref pci_rescan_bus(struct pci_bus *bus)
>        BUG_ON(!list_empty(&add_list));
>
>        pci_enable_bridges(bus);
> +}
> +#ifdef CONFIG_HOTPLUG
> +/**
> + * pci_rescan_bus - scan a PCI bus for devices.
> + * @bus: PCI bus to scan
> + *
> + * Scan a PCI bus and child buses for new devices, adds them,
> + * and enables them.
> + *
> + * Returns the max number of subordinate bus discovered.
> + */
> +unsigned int __ref pci_rescan_bus(struct pci_bus *bus)
> +{
> +       unsigned int max;
> +
> +       max = pci_scan_child_bus(bus);
> +
> +       pci_assign_unassigned_bus_resources(bus);
> +
>        pci_bus_add_devices(bus);
>
>        return max;

We don't need all the blank lines here.

> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 28556cb..4f30cd1 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -910,6 +910,7 @@ void pci_bus_size_bridges(struct pci_bus *bus);
>  int pci_claim_resource(struct pci_dev *, int);
>  void pci_assign_unassigned_resources(void);
>  void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge);
> +void pci_assign_unassigned_bus_resources(struct pci_bus *bus);
>  void pdev_enable_device(struct pci_dev *);
>  int pci_enable_resources(struct pci_dev *, int mask);
>  void pci_fixup_irqs(u8 (*)(struct pci_dev *, u8 *),
> --
> 1.7.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 14/23] PCI: add __pci_remove_bus_devices()

[Bjorn Helgaas]

On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> will use it with pci_stop_and_remove_bus later.
>
> also remove __pci_remove_behind_bridge and pci_stop_behind_bridge.
>
> they are same except one take bridge and one take bus.
>
> and we already have pci_stop_bus_devices()
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/remove.c |   28 +++++++++++-----------------
>  1 files changed, 11 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index 243d59b..62c348c 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -78,7 +78,7 @@ void pci_remove_bus(struct pci_bus *pci_bus)
>  }
>  EXPORT_SYMBOL(pci_remove_bus);
>
> -static void __pci_remove_behind_bridge(struct pci_dev *dev);
> +static void __pci_remove_bus_devices(struct pci_bus *bus);
>  /**
>  * pci_stop_and_remove_bus_device - remove a PCI device and any children
>  * @dev: the device to remove
> @@ -96,7 +96,7 @@ void __pci_remove_bus_device(struct pci_dev *dev)
>        if (dev->subordinate) {
>                struct pci_bus *b = dev->subordinate;
>
> -               __pci_remove_behind_bridge(dev);
> +               __pci_remove_bus_devices(b);
>                pci_remove_bus(b);
>                dev->subordinate = NULL;
>        }
> @@ -111,22 +111,12 @@ void pci_stop_and_remove_bus_device(struct pci_dev *dev)
>        __pci_remove_bus_device(dev);
>  }
>
> -static void __pci_remove_behind_bridge(struct pci_dev *dev)
> +static void __pci_remove_bus_devices(struct pci_bus *bus)
>  {
>        struct list_head *l, *n;
>
> -       if (dev->subordinate)
> -               list_for_each_safe(l, n, &dev->subordinate->devices)
> -                       __pci_remove_bus_device(pci_dev_b(l));
> -}
> -
> -static void pci_stop_behind_bridge(struct pci_dev *dev)
> -{
> -       struct list_head *l, *n;
> -
> -       if (dev->subordinate)
> -               list_for_each_safe(l, n, &dev->subordinate->devices)
> -                       pci_stop_bus_device(pci_dev_b(l));
> +       list_for_each_safe(l, n, &bus->devices)
> +               __pci_remove_bus_device(pci_dev_b(l));

Use list_for_each_entry_safe() so you don't need pci_dev_b().

>  }
>
>  static void pci_stop_bus_devices(struct pci_bus *bus)
> @@ -158,8 +148,12 @@ static void pci_stop_bus_devices(struct pci_bus *bus)
>  */
>  void pci_stop_and_remove_behind_bridge(struct pci_dev *dev)
>  {
> -       pci_stop_behind_bridge(dev);
> -       __pci_remove_behind_bridge(dev);
> +       struct pci_bus *bus = dev->subordinate;
> +
> +       if (bus) {

Don't check "bus" here.  If the caller screws up and passes a
non-bridge pointer, I want to learn about it rather than ignore it.

> +               pci_stop_bus_devices(bus);
> +               __pci_remove_bus_devices(bus);
> +       }
>  }
>
>  /**
> --
> 1.7.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/23] PCI, sysfs: create rescan_bridge under /sys/.../pci/devices/... for pci bridges

[Yinghai Lu]

On Thu, Mar 8, 2012 at 4:52 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> Current code will create rescan for every pci device under parent bus.
>> that is not right. the device is already there, there is no reason to rescan it.
>>
>> We could have rescan for pci bridges. less confusing.
>
> Yes, but now we have *three* rescan attributes for a single bridge/bus:
>
>    /sys/devices/pci0000:00/0000:00:01.0/pci_bus/0000:01/rescan

no, that one is not there, and should not be added.

But I do suggest to add
/sys/devices/pci0000:00/0000:00:01.0/pci_bus/0000:01/remove

>    /sys/devices/pci0000:00/0000:00:01.0/rescan_bridge
>    /sys/devices/pci0000:00/0000:00:01.0/rescan
>
> and this for an endpoint:
>
>    /sys/devices/pci0000:00/0000:00:05.0/rescan
>
> I think endpoints should not have a "rescan" attribute at all, and
> bridges should have only a "rescan" attribute (not "rescan" and
> "rescan_bridge").

agreed,  that rescan actually is doing rescan of it's parent bus.

I have suggested to remove it. but Alex said some HP internal
application will need
that, so it can not be removed.

at last I have to use rescan_bridge for bridge device.

>
> I'm not sure about "pci_bus/.../rescan".  I know you didn't add that,
> but I'm not sure it makes sense to have both that and a "rescan" on
> the bridge device.  If we have both, what's the difference between
> them?

rescan : for device parent bus, that is really weird.
rescan_bridge: will only show up that device is bridge, and rescan that bridge.

Yinghai

Re: [PATCH 06/23] PCI: rescan with bus or bridge using callback method too

[Yinghai Lu]

On Thu, Mar 8, 2012 at 4:56 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> Just like removal.
>>
>> Because We could add new bus under the bridges...
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> ---
>>  drivers/pci/pci-sysfs.c |   43 +++++++++++++++++++++++++++++++------------
>>  1 files changed, 31 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> index 2049b2f..1794508 100644
>> --- a/drivers/pci/pci-sysfs.c
>> +++ b/drivers/pci/pci-sysfs.c
>> @@ -325,21 +325,31 @@ dev_rescan_store(struct device *dev, struct device_attribute *attr,
>>        return count;
>>  }
>>
>> +static void bridge_rescan_callback(struct device *dev)
>> +{
>> +       struct pci_dev *pdev = to_pci_dev(dev);
>> +
>> +       mutex_lock(&pci_remove_rescan_mutex);
>> +       pci_rescan_bus_bridge_resize(pdev);
>> +       mutex_unlock(&pci_remove_rescan_mutex);
>> +}
>> +
>>  static ssize_t
>>  dev_bridge_rescan_store(struct device *dev, struct device_attribute *attr,
>>                 const char *buf, size_t count)
>>  {
>> +       int ret = 0;
>>        unsigned long val;
>> -       struct pci_dev *pdev = to_pci_dev(dev);
>>
>>        if (kstrtoul(buf, 0, &val) < 0)
>>                return -EINVAL;
>>
>> -       if (val) {
>> -               mutex_lock(&pci_remove_rescan_mutex);
>> -               pci_rescan_bus_bridge_resize(pdev);
>> -               mutex_unlock(&pci_remove_rescan_mutex);
>> -       }
>> +       if (val)
>> +               ret = device_schedule_callback(dev, bridge_rescan_callback);
>> +
>> +       if (ret)
>> +               count = ret;
>> +
>>        return count;
>
> I see that you copied this style from remove_store(), but it's ugly
> and hard to follow.  I think this would be more readable:
>
>    if (!val)
>        return count;
>
>    err = device_schedule_callback(dev, bridge_rescan_callback);
>    if (err)
>        return err;
>
>    return count;
>

like

        if (!val)
                return count;

        ret = device_schedule_callback(dev, bus_rescan_callback);

        if (ret)
                return ret;

        return count;

that will have three return instead of one.

both should be ok.

Yinghai

Re: [PATCH 07/23] pci, dmar: Update dmar units devices list during hotplug

[Yinghai Lu]

On Thu, Mar 8, 2012 at 5:06 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> When do pci remove/rescan on system that have more iommus, got
>>
>> [  894.089745] Set context mapping for c4:00.0
>> [  894.110890] mpt2sas3: Allocated physical memory: size(4293 kB)
>> [  894.112556] mpt2sas3: Current Controller Queue Depth(1883), Max Controller Queue Depth(2144)
>> [  894.127278] mpt2sas3: Scatter Gather Elements per IO(128)
>> [  894.361295] DRHD: handling fault status reg 2
>> [  894.364053] DMAR:[DMA Read] Request device [c4:00.0] fault addr fffbe000
>> [  894.364056] DMAR:[fault reason 02] Present bit in context entry is cl
>>
>> it turns out when remove/rescan, pci dev will be freed and will get another new dev.
>> but drhd units still keep old one... so dmar_find_matched_drhd_unit will
>> return wrong drhd and iommu for the device that is not on first iommu.
>>
>> So need to update devices in drhd_units during pci remove/rescan.
>>
>> Could save domain/bus/device/function aside in the list and according that info
>> restore new dev to drhd_units later.
>> Then dmar_find_matched_drdh_unit and device_to_iommu could return right drhd and iommu.
>>
>> Add remove_dev_from_drhd/restore_dev_to_drhd functions to do the real work.
>> call them in device ADD_DEVICE and UNBOUND_DRIVER
>>
>> Need to do the samething to atsr.  (expose dmar_atsr_units and add atsru->segment)
>>
>> After patch, will right iommu for the new dev and will not get DMAR error any more.
>>
>> -v2: add pci_dev_put/pci_dev_get to make refcount consistent.
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> Cc: David Woodhouse <dwmw2@infradead.org>
>> Cc: Vinod Koul <vinod.koul@intel.com>
>> Cc: Dan Williams <dan.j.williams@intel.com>
>> Cc: iommu@lists.linux-foundation.org
>> ---
>>  drivers/iommu/intel-iommu.c |  187 ++++++++++++++++++++++++++++++++++++++++---
>>  include/linux/dmar.h        |    4 +
>>  2 files changed, 181 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>> index c9c6053..39ef2ce 100644
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -665,6 +665,159 @@ static struct intel_iommu *device_to_iommu(int segment, u8 bus, u8 devfn)
>>        return NULL;
>>  }
>>
>> +#ifdef CONFIG_HOTPLUG
>> +struct dev_dmaru {
>> +       struct list_head list;
>> +       void *dmaru;
>> +       int index;
>> +       int segment;
>> +       unsigned char bus;
>> +       unsigned int devfn;
>> +};
>> +
>> +static int
>> +save_dev_dmaru(int segment, unsigned char bus, unsigned int devfn,
>> +                void *dmaru, int index, struct list_head *lh)
>
> Follow the indentation style used elsewhere in the file: as much of
> the declaration on one line as will fit (here and below).
>
> I think the callers would read more naturally if you passed in the
> struct pci_dev * to save_dev_dmaru() and extracted the
> segment/bus/devfn here rather than in the callers.

Just want to keep save_dev_dmaru more consistent to get_dev_dmaru...

Anyway, will change to pass struct pci_dev *dev instead.

Yinghai

Re: [PATCH 14/23] PCI: add __pci_remove_bus_devices()

[Yinghai Lu]

On Thu, Mar 8, 2012 at 5:11 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> will use it with pci_stop_and_remove_bus later.
>>
>> also remove __pci_remove_behind_bridge and pci_stop_behind_bridge.
>>
>> they are same except one take bridge and one take bus.
>>
>> and we already have pci_stop_bus_devices()
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> ---
>>  drivers/pci/remove.c |   28 +++++++++++-----------------
>>  1 files changed, 11 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
>> index 243d59b..62c348c 100644
>> --- a/drivers/pci/remove.c
>> +++ b/drivers/pci/remove.c
>> @@ -78,7 +78,7 @@ void pci_remove_bus(struct pci_bus *pci_bus)
>>  }
>>  EXPORT_SYMBOL(pci_remove_bus);
>>
>> -static void __pci_remove_behind_bridge(struct pci_dev *dev);
>> +static void __pci_remove_bus_devices(struct pci_bus *bus);
>>  /**
>>  * pci_stop_and_remove_bus_device - remove a PCI device and any children
>>  * @dev: the device to remove
>> @@ -96,7 +96,7 @@ void __pci_remove_bus_device(struct pci_dev *dev)
>>        if (dev->subordinate) {
>>                struct pci_bus *b = dev->subordinate;
>>
>> -               __pci_remove_behind_bridge(dev);
>> +               __pci_remove_bus_devices(b);
>>                pci_remove_bus(b);
>>                dev->subordinate = NULL;
>>        }
>> @@ -111,22 +111,12 @@ void pci_stop_and_remove_bus_device(struct pci_dev *dev)
>>        __pci_remove_bus_device(dev);
>>  }
>>
>> -static void __pci_remove_behind_bridge(struct pci_dev *dev)
>> +static void __pci_remove_bus_devices(struct pci_bus *bus)
>>  {
>>        struct list_head *l, *n;
>>
>> -       if (dev->subordinate)
>> -               list_for_each_safe(l, n, &dev->subordinate->devices)
>> -                       __pci_remove_bus_device(pci_dev_b(l));
>> -}
>> -
>> -static void pci_stop_behind_bridge(struct pci_dev *dev)
>> -{
>> -       struct list_head *l, *n;
>> -
>> -       if (dev->subordinate)
>> -               list_for_each_safe(l, n, &dev->subordinate->devices)
>> -                       pci_stop_bus_device(pci_dev_b(l));
>> +       list_for_each_safe(l, n, &bus->devices)
>> +               __pci_remove_bus_device(pci_dev_b(l));
>
> Use list_for_each_entry_safe() so you don't need pci_dev_b().

just want to keep the patch to simple, and looks like just name renaming.

also use list_for_each_safe instead of list_for_each_entry_safe

could have less conversion.

>
>>  }
>>
>>  static void pci_stop_bus_devices(struct pci_bus *bus)
>> @@ -158,8 +148,12 @@ static void pci_stop_bus_devices(struct pci_bus *bus)
>>  */
>>  void pci_stop_and_remove_behind_bridge(struct pci_dev *dev)
>>  {
>> -       pci_stop_behind_bridge(dev);
>> -       __pci_remove_behind_bridge(dev);
>> +       struct pci_bus *bus = dev->subordinate;
>> +
>> +       if (bus) {
>
> Don't check "bus" here.  If the caller screws up and passes a
> non-bridge pointer, I want to learn about it rather than ignore it.

old code have that
           if (dev->subordinate)

checking.

Yinghai

Re: [PATCH 00/23] PCI, x86: pci root bus hotplug support

[Yinghai Lu]

On Thu, Mar 8, 2012 at 4:43 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Mar 6, 2012 at 11:58 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Tue, Mar 6, 2012 at 8:44 PM, Bjorn Helgaas <bjorn.helgaas@gmail.com> wrote:
>>> On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> Does this series support hot-add, too, or just removal?
>>>
>>> If it is intended to support hot-add, I didn't see any support for
>>> _CBA, which I *think* is the way hotplug-capable host bridges are
>>> required to report MMCONFIG areas.
>>
>> to make the whole hotplug working with intel new cpus with IIO,
>> in addition to physical cpu and memory hotplug, will still need
>> a. ioapic controller hotplug
>> b. intr-remapping hotplug
>> c. intel-iommu hotplug
>> d. mmconf support.
>> e. root bus hotplug
>>
>> this patch set is only involved with partitally root bus removal/add back.
>> and assume ioapic, intr-remap, intel-iommu, and mmconf related are
>> working from early initialization.
>>
>> that will help us to help to fix bugs like devices refcount leaking.
>
> OK, so this series only supports host bridge removal.  There are some
> situations where it allows a bridge to be added, but only if the other
> things you mentioned (ioapic, intr-remapping, etc.) are already
> present, probably because we didn't remove them when removing the host
> bridge.

yes.

pci based ioapic support is there for x86....

still need to look at acpi based: ioapic, iommu. etc.

>
> In the case of MMCONFIG in particular, I think this should be moved
> into the host bridge add path, e.g., in acpi_pci_root_add().  The fact
> that it's totally separate today is a mistake.  But that can be fixed
> later.

could have different pci_ops to make it simple.

but need to kill direct using of raw_pci_read/writing at first.

>
> Overall nits, because kernel development is obsessive by nature:
> Please format your changelogs with textwidth=75, so that when you read
> them with "git log" in an 80-column terminal, the normal text doesn't
> wrap across a line ending.
>
> Please make your subject lines lines consistent in style and
> capitalization.  Sometimes you have "PCI", sometimes "pci".  Sometimes
> the first word after "PCI: " is capitalized, sometimes not.  One of
> your intel-iommu.c patches is "IOMMU:"; the other is "pci, dmar:" (and
> these patches are not adjacent in the series, even though they could
> be).  This is just sloppy and distracting.

Sorry, that patch sit on local directly for some time,  and forget to update
subject this time.

>
> In English, the first word of a sentence is capitalized.  Words in the
> middle are not, unless they are acronyms or proper nouns.  It would be
> helpful if you could follow these conventions in your changelogs,
> because I expect to be reading them for years to come, and I'm easily
> distracted.
...
>
> The notes about "-v2, -v3, -v4, etc." are not really useful in the
> commit changelogs.  They're helpful in the [00/] message, but not in
> the changelogs.

But Jesse may still need mark to figure out which one is latest?

>
> I appreciate the effort you're making to write changelogs.  They're
> definitely starting to contain some useful information.

Sure, will try my best on that part.

Thanks

Yinghai

Re: [PATCH 02/23] PCI, sysfs: create rescan_bridge under /sys/.../pci/devices/... for pci bridges

[Bjorn Helgaas]

On Thu, Mar 8, 2012 at 11:42 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Mar 8, 2012 at 4:52 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> Current code will create rescan for every pci device under parent bus.
>>> that is not right. the device is already there, there is no reason to rescan it.
>>>
>>> We could have rescan for pci bridges. less confusing.
>>
>> Yes, but now we have *three* rescan attributes for a single bridge/bus:
>>
>>    /sys/devices/pci0000:00/0000:00:01.0/pci_bus/0000:01/rescan
>
> no, that one is not there, and should not be added.

Yes, it is!  I pasted that directly from the output of "find /sys
-name rescan\*".  Your series didn't add it, but it definitely exists.

> But I do suggest to add
> /sys/devices/pci0000:00/0000:00:01.0/pci_bus/0000:01/remove

Yes, your "[16/23] PCI: add pci bus removal through
/sys/.../pci_bus/.../remove" patch adds this "remove" file.  I think
it is a mistake.

A bus is just a collection of wires that connect devices.  The
*devices* are the interesting things, not the wires.  You can ask the
upstream bridge to look for devices on its secondary bus, you can add
or remove devices on the secondary side, you can remove the bridge
itself, etc.

You're using "pci_bus/.../remove" as shorthand for "remove all the
devices on this bus and remove the bus itself."  But I think that's
nonsense.  The bridge still exists, so the secondary bus still exists.
 You can already remove all the secondary devices by using the
*device* remove files, e.g., ".../0000:00:00.0/remove".  And you can
remove the bridge the same way.

So I don't think "pci_bus/.../remove" adds useful new functionality.
I think it *does* add a way to remove the internal struct pci_bus, but
I don't think that's a good thing to expose to users.

>> I think endpoints should not have a "rescan" attribute at all, and
>> bridges should have only a "rescan" attribute (not "rescan" and
>> "rescan_bridge").
>
> agreed,  that rescan actually is doing rescan of it's parent bus.
>
> I have suggested to remove it. but Alex said some HP internal
> application will need
> that, so it can not be removed.
>
> at last I have to use rescan_bridge for bridge device.

Here's that previous discussion, for reference:
http://www.spinics.net/lists/linux-pci/msg10825.html

The way I read that discussion is that it's fine to mark endpoint
"rescan" as deprecated and to use "rescan" for bridges only.
Unfortunately, you didn't follow through with the deprecation process.
 If you had, we could be removing endpoint "rescan" now, and we'd be
all set.

I think deprecating endpoint "rescan" is still the right thing to do.

Bjorn

Re: [PATCH 06/23] PCI: rescan with bus or bridge using callback method too

[Bjorn Helgaas]

On Thu, Mar 8, 2012 at 11:53 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Mar 8, 2012 at 4:56 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> Just like removal.
>>>
>>> Because We could add new bus under the bridges...
>>>
>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>> ---
>>>  drivers/pci/pci-sysfs.c |   43 +++++++++++++++++++++++++++++++------------
>>>  1 files changed, 31 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>>> index 2049b2f..1794508 100644
>>> --- a/drivers/pci/pci-sysfs.c
>>> +++ b/drivers/pci/pci-sysfs.c
>>> @@ -325,21 +325,31 @@ dev_rescan_store(struct device *dev, struct device_attribute *attr,
>>>        return count;
>>>  }
>>>
>>> +static void bridge_rescan_callback(struct device *dev)
>>> +{
>>> +       struct pci_dev *pdev = to_pci_dev(dev);
>>> +
>>> +       mutex_lock(&pci_remove_rescan_mutex);
>>> +       pci_rescan_bus_bridge_resize(pdev);
>>> +       mutex_unlock(&pci_remove_rescan_mutex);
>>> +}
>>> +
>>>  static ssize_t
>>>  dev_bridge_rescan_store(struct device *dev, struct device_attribute *attr,
>>>                 const char *buf, size_t count)
>>>  {
>>> +       int ret = 0;
>>>        unsigned long val;
>>> -       struct pci_dev *pdev = to_pci_dev(dev);
>>>
>>>        if (kstrtoul(buf, 0, &val) < 0)
>>>                return -EINVAL;
>>>
>>> -       if (val) {
>>> -               mutex_lock(&pci_remove_rescan_mutex);
>>> -               pci_rescan_bus_bridge_resize(pdev);
>>> -               mutex_unlock(&pci_remove_rescan_mutex);
>>> -       }
>>> +       if (val)
>>> +               ret = device_schedule_callback(dev, bridge_rescan_callback);
>>> +
>>> +       if (ret)
>>> +               count = ret;
>>> +
>>>        return count;
>>
>> I see that you copied this style from remove_store(), but it's ugly
>> and hard to follow.  I think this would be more readable:
>>
>>    if (!val)
>>        return count;
>>
>>    err = device_schedule_callback(dev, bridge_rescan_callback);
>>    if (err)
>>        return err;
>>
>>    return count;
>>
>
> like
>
>        if (!val)
>                return count;
>
>        ret = device_schedule_callback(dev, bus_rescan_callback);
>
>        if (ret)
>                return ret;
>
>        return count;
>
> that will have three return instead of one.
>
> both should be ok.

Why did you just repeat my proposal, changing "err" to "ret"?  I
missed the point you're trying to make.  The name "err" gives an
important clue that device_schedule_callback() returns 0 or errno.
That's essential to understanding the code.

Your original patch has one "return."  Mine proposal has three.  All
that means is that the reader of your patch has to manage more stuff
in her head to understand the code.  That's not a benefit!

Bjorn

Re: [PATCH 07/23] pci, dmar: Update dmar units devices list during hotplug

[Bjorn Helgaas]

On Fri, Mar 9, 2012 at 12:06 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Mar 8, 2012 at 5:06 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> When do pci remove/rescan on system that have more iommus, got
>>>
>>> [  894.089745] Set context mapping for c4:00.0
>>> [  894.110890] mpt2sas3: Allocated physical memory: size(4293 kB)
>>> [  894.112556] mpt2sas3: Current Controller Queue Depth(1883), Max Controller Queue Depth(2144)
>>> [  894.127278] mpt2sas3: Scatter Gather Elements per IO(128)
>>> [  894.361295] DRHD: handling fault status reg 2
>>> [  894.364053] DMAR:[DMA Read] Request device [c4:00.0] fault addr fffbe000
>>> [  894.364056] DMAR:[fault reason 02] Present bit in context entry is cl
>>>
>>> it turns out when remove/rescan, pci dev will be freed and will get another new dev.
>>> but drhd units still keep old one... so dmar_find_matched_drhd_unit will
>>> return wrong drhd and iommu for the device that is not on first iommu.
>>>
>>> So need to update devices in drhd_units during pci remove/rescan.
>>>
>>> Could save domain/bus/device/function aside in the list and according that info
>>> restore new dev to drhd_units later.
>>> Then dmar_find_matched_drdh_unit and device_to_iommu could return right drhd and iommu.
>>>
>>> Add remove_dev_from_drhd/restore_dev_to_drhd functions to do the real work.
>>> call them in device ADD_DEVICE and UNBOUND_DRIVER
>>>
>>> Need to do the samething to atsr.  (expose dmar_atsr_units and add atsru->segment)
>>>
>>> After patch, will right iommu for the new dev and will not get DMAR error any more.
>>>
>>> -v2: add pci_dev_put/pci_dev_get to make refcount consistent.
>>>
>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>> Cc: David Woodhouse <dwmw2@infradead.org>
>>> Cc: Vinod Koul <vinod.koul@intel.com>
>>> Cc: Dan Williams <dan.j.williams@intel.com>
>>> Cc: iommu@lists.linux-foundation.org
>>> ---
>>>  drivers/iommu/intel-iommu.c |  187 ++++++++++++++++++++++++++++++++++++++++---
>>>  include/linux/dmar.h        |    4 +
>>>  2 files changed, 181 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>>> index c9c6053..39ef2ce 100644
>>> --- a/drivers/iommu/intel-iommu.c
>>> +++ b/drivers/iommu/intel-iommu.c
>>> @@ -665,6 +665,159 @@ static struct intel_iommu *device_to_iommu(int segment, u8 bus, u8 devfn)
>>>        return NULL;
>>>  }
>>>
>>> +#ifdef CONFIG_HOTPLUG
>>> +struct dev_dmaru {
>>> +       struct list_head list;
>>> +       void *dmaru;
>>> +       int index;
>>> +       int segment;
>>> +       unsigned char bus;
>>> +       unsigned int devfn;
>>> +};
>>> +
>>> +static int
>>> +save_dev_dmaru(int segment, unsigned char bus, unsigned int devfn,
>>> +                void *dmaru, int index, struct list_head *lh)
>>
>> Follow the indentation style used elsewhere in the file: as much of
>> the declaration on one line as will fit (here and below).
>>
>> I think the callers would read more naturally if you passed in the
>> struct pci_dev * to save_dev_dmaru() and extracted the
>> segment/bus/devfn here rather than in the callers.
>
> Just want to keep save_dev_dmaru more consistent to get_dev_dmaru...

Well, it looks like you can change both save_dev_dmaru() *and*
get_dev_dmaru() to take a struct pci_dev *.  I assumed that would be
obvious.

Bjorn

Re: [PATCH 14/23] PCI: add __pci_remove_bus_devices()

[Bjorn Helgaas]

On Fri, Mar 9, 2012 at 12:17 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Mar 8, 2012 at 5:11 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> will use it with pci_stop_and_remove_bus later.
>>>
>>> also remove __pci_remove_behind_bridge and pci_stop_behind_bridge.
>>>
>>> they are same except one take bridge and one take bus.
>>>
>>> and we already have pci_stop_bus_devices()
>>>
>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>> ---
>>>  drivers/pci/remove.c |   28 +++++++++++-----------------
>>>  1 files changed, 11 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
>>> index 243d59b..62c348c 100644
>>> --- a/drivers/pci/remove.c
>>> +++ b/drivers/pci/remove.c
>>> @@ -78,7 +78,7 @@ void pci_remove_bus(struct pci_bus *pci_bus)
>>>  }
>>>  EXPORT_SYMBOL(pci_remove_bus);
>>>
>>> -static void __pci_remove_behind_bridge(struct pci_dev *dev);
>>> +static void __pci_remove_bus_devices(struct pci_bus *bus);
>>>  /**
>>>  * pci_stop_and_remove_bus_device - remove a PCI device and any children
>>>  * @dev: the device to remove
>>> @@ -96,7 +96,7 @@ void __pci_remove_bus_device(struct pci_dev *dev)
>>>        if (dev->subordinate) {
>>>                struct pci_bus *b = dev->subordinate;
>>>
>>> -               __pci_remove_behind_bridge(dev);
>>> +               __pci_remove_bus_devices(b);
>>>                pci_remove_bus(b);
>>>                dev->subordinate = NULL;
>>>        }
>>> @@ -111,22 +111,12 @@ void pci_stop_and_remove_bus_device(struct pci_dev *dev)
>>>        __pci_remove_bus_device(dev);
>>>  }
>>>
>>> -static void __pci_remove_behind_bridge(struct pci_dev *dev)
>>> +static void __pci_remove_bus_devices(struct pci_bus *bus)
>>>  {
>>>        struct list_head *l, *n;
>>>
>>> -       if (dev->subordinate)
>>> -               list_for_each_safe(l, n, &dev->subordinate->devices)
>>> -                       __pci_remove_bus_device(pci_dev_b(l));
>>> -}
>>> -
>>> -static void pci_stop_behind_bridge(struct pci_dev *dev)
>>> -{
>>> -       struct list_head *l, *n;
>>> -
>>> -       if (dev->subordinate)
>>> -               list_for_each_safe(l, n, &dev->subordinate->devices)
>>> -                       pci_stop_bus_device(pci_dev_b(l));
>>> +       list_for_each_safe(l, n, &bus->devices)
>>> +               __pci_remove_bus_device(pci_dev_b(l));
>>
>> Use list_for_each_entry_safe() so you don't need pci_dev_b().
>
> just want to keep the patch to simple, and looks like just name renaming.
>
> also use list_for_each_safe instead of list_for_each_entry_safe
>
> could have less conversion.

Sorry, I didn't understand the above.

It is OK to improve code as you change it :)  list_for_each_entry() is
clearly an improvement over list_for_each() + some conversion macro.

>>>  }
>>>
>>>  static void pci_stop_bus_devices(struct pci_bus *bus)
>>> @@ -158,8 +148,12 @@ static void pci_stop_bus_devices(struct pci_bus *bus)
>>>  */
>>>  void pci_stop_and_remove_behind_bridge(struct pci_dev *dev)
>>>  {
>>> -       pci_stop_behind_bridge(dev);
>>> -       __pci_remove_behind_bridge(dev);
>>> +       struct pci_bus *bus = dev->subordinate;
>>> +
>>> +       if (bus) {
>>
>> Don't check "bus" here.  If the caller screws up and passes a
>> non-bridge pointer, I want to learn about it rather than ignore it.
>
> old code have that
>           if (dev->subordinate)
>
> checking.

Removing a test that could silently cover a programming error is also
an improvement.

Bjorn

Re: [PATCH 07/23] pci, dmar: Update dmar units devices list during hotplug

[Yinghai Lu]

On Fri, Mar 9, 2012 at 9:25 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> Well, it looks like you can change both save_dev_dmaru() *and*
> get_dev_dmaru() to take a struct pci_dev *.  I assumed that would be
> obvious.

no.

get_dev_dmaru() have to use bus/devfn to retrieve the dev pointer.

Yinghai

Re: [PATCH 00/23] PCI, x86: pci root bus hotplug support

[Bjorn Helgaas]

On Fri, Mar 9, 2012 at 1:19 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Mar 8, 2012 at 4:43 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:

>> The notes about "-v2, -v3, -v4, etc." are not really useful in the
>> commit changelogs.  They're helpful in the [00/] message, but not in
>> the changelogs.
>
> But Jesse may still need mark to figure out which one is latest?

The way I do that is with "stg mail -v v3", which puts "[PATCH v3
01/10]" in the subject.  IMO, that's much more useful because when you
post a 23-patch series, I don't want to be bothered with v1, v2, v3 on
a per-patch basis.

It's true that when you post v2, many of those 23 patches will be
unchanged from v1, and you do have to describe that in the [v2 00/23]
mail.  But at least it's easy to identify the entire set of v2
patches.

Bjorn

Re: [PATCH 07/23] pci, dmar: Update dmar units devices list during hotplug

[Bjorn Helgaas]

On Fri, Mar 9, 2012 at 10:32 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Fri, Mar 9, 2012 at 9:25 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>> Well, it looks like you can change both save_dev_dmaru() *and*
>> get_dev_dmaru() to take a struct pci_dev *.  I assumed that would be
>> obvious.
>
> no.
>
> get_dev_dmaru() have to use bus/devfn to retrieve the dev pointer.

I expected something like that, but all I see in the patch is this:

+static void restore_dev_to_drhd(struct pci_dev *dev)
+       drhd = get_dev_dmaru(pci_domain_nr(dev->bus), dev->bus->number,
+                                dev->devfn, &i, &saved_dev_drhd_list);

+static void restore_dev_to_atsr(struct pci_dev *dev)
+       atsr = get_dev_dmaru(pci_domain_nr(dev->bus), dev->bus->number,
+                                dev->devfn, &i, &saved_dev_atsr_list);

In both those cases, you have the struct pci_dev *.

Re: [PATCH 07/23] pci, dmar: Update dmar units devices list during hotplug

[Yinghai Lu]

On Fri, Mar 9, 2012 at 9:37 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, Mar 9, 2012 at 10:32 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Fri, Mar 9, 2012 at 9:25 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>>
>>> Well, it looks like you can change both save_dev_dmaru() *and*
>>> get_dev_dmaru() to take a struct pci_dev *.  I assumed that would be
>>> obvious.
>>
>> no.
>>
>> get_dev_dmaru() have to use bus/devfn to retrieve the dev pointer.
>
> I expected something like that, but all I see in the patch is this:
>
> +static void restore_dev_to_drhd(struct pci_dev *dev)
> +       drhd = get_dev_dmaru(pci_domain_nr(dev->bus), dev->bus->number,
> +                                dev->devfn, &i, &saved_dev_drhd_list);
>
> +static void restore_dev_to_atsr(struct pci_dev *dev)
> +       atsr = get_dev_dmaru(pci_domain_nr(dev->bus), dev->bus->number,
> +                                dev->devfn, &i, &saved_dev_atsr_list);
>
> In both those cases, you have the struct pci_dev *.

OK, i missed that. will update that accordingly.

Thanks

Yinghai

Re: [PATCH 00/23] PCI, x86: pci root bus hotplug support

[Yinghai Lu]

On Fri, Mar 9, 2012 at 9:34 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, Mar 9, 2012 at 1:19 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Thu, Mar 8, 2012 at 4:43 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
>>> The notes about "-v2, -v3, -v4, etc." are not really useful in the
>>> commit changelogs.  They're helpful in the [00/] message, but not in
>>> the changelogs.
>>
>> But Jesse may still need mark to figure out which one is latest?
>
> The way I do that is with "stg mail -v v3", which puts "[PATCH v3
> 01/10]" in the subject.  IMO, that's much more useful because when you
> post a 23-patch series, I don't want to be bothered with v1, v2, v3 on
> a per-patch basis.

For that, I could just sed to replace PATCH to PATCH v3

>
> It's true that when you post v2, many of those 23 patches will be
> unchanged from v1, and you do have to describe that in the [v2 00/23]
> mail.  But at least it's easy to identify the entire set of v2
> patches.

Version number of the entire set could be different from individual patch.
That could be more confusing.

Yinghai

Re: [PATCH 14/23] PCI: add __pci_remove_bus_devices()

[Yinghai Lu]

On Fri, Mar 9, 2012 at 9:28 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, Mar 9, 2012 at 12:17 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Thu, Mar 8, 2012 at 5:11 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>>> will use it with pci_stop_and_remove_bus later.
>>>>
>>>> also remove __pci_remove_behind_bridge and pci_stop_behind_bridge.
>>>>
>>>> they are same except one take bridge and one take bus.
>>>>
>>>> and we already have pci_stop_bus_devices()
>>>>
>>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>>> ---
>>>>  drivers/pci/remove.c |   28 +++++++++++-----------------
>>>>  1 files changed, 11 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
>>>> index 243d59b..62c348c 100644
>>>> --- a/drivers/pci/remove.c
>>>> +++ b/drivers/pci/remove.c
>>>> @@ -78,7 +78,7 @@ void pci_remove_bus(struct pci_bus *pci_bus)
>>>>  }
>>>>  EXPORT_SYMBOL(pci_remove_bus);
>>>>
>>>> -static void __pci_remove_behind_bridge(struct pci_dev *dev);
>>>> +static void __pci_remove_bus_devices(struct pci_bus *bus);
>>>>  /**
>>>>  * pci_stop_and_remove_bus_device - remove a PCI device and any children
>>>>  * @dev: the device to remove
>>>> @@ -96,7 +96,7 @@ void __pci_remove_bus_device(struct pci_dev *dev)
>>>>        if (dev->subordinate) {
>>>>                struct pci_bus *b = dev->subordinate;
>>>>
>>>> -               __pci_remove_behind_bridge(dev);
>>>> +               __pci_remove_bus_devices(b);
>>>>                pci_remove_bus(b);
>>>>                dev->subordinate = NULL;
>>>>        }
>>>> @@ -111,22 +111,12 @@ void pci_stop_and_remove_bus_device(struct pci_dev *dev)
>>>>        __pci_remove_bus_device(dev);
>>>>  }
>>>>
>>>> -static void __pci_remove_behind_bridge(struct pci_dev *dev)
>>>> +static void __pci_remove_bus_devices(struct pci_bus *bus)
>>>>  {
>>>>        struct list_head *l, *n;
>>>>
>>>> -       if (dev->subordinate)
>>>> -               list_for_each_safe(l, n, &dev->subordinate->devices)
>>>> -                       __pci_remove_bus_device(pci_dev_b(l));
>>>> -}
>>>> -
>>>> -static void pci_stop_behind_bridge(struct pci_dev *dev)
>>>> -{
>>>> -       struct list_head *l, *n;
>>>> -
>>>> -       if (dev->subordinate)
>>>> -               list_for_each_safe(l, n, &dev->subordinate->devices)
>>>> -                       pci_stop_bus_device(pci_dev_b(l));
>>>> +       list_for_each_safe(l, n, &bus->devices)
>>>> +               __pci_remove_bus_device(pci_dev_b(l));
>>>
>>> Use list_for_each_entry_safe() so you don't need pci_dev_b().
>>
>> just want to keep the patch to simple, and looks like just name renaming.
>>
>> also use list_for_each_safe instead of list_for_each_entry_safe
>>
>> could have less conversion.
>
> Sorry, I didn't understand the above.
>
> It is OK to improve code as you change it :)  list_for_each_entry() is
> clearly an improvement over list_for_each() + some conversion macro.

ok, will change that.

>
>>>>  }
>>>>
>>>>  static void pci_stop_bus_devices(struct pci_bus *bus)
>>>> @@ -158,8 +148,12 @@ static void pci_stop_bus_devices(struct pci_bus *bus)
>>>>  */
>>>>  void pci_stop_and_remove_behind_bridge(struct pci_dev *dev)
>>>>  {
>>>> -       pci_stop_behind_bridge(dev);
>>>> -       __pci_remove_behind_bridge(dev);
>>>> +       struct pci_bus *bus = dev->subordinate;
>>>> +
>>>> +       if (bus) {
>>>
>>> Don't check "bus" here.  If the caller screws up and passes a
>>> non-bridge pointer, I want to learn about it rather than ignore it.
>>
>> old code have that
>>           if (dev->subordinate)
>>
>> checking.
>
> Removing a test that could silently cover a programming error is also
> an improvement.

just want to cause regression.

also it is legal that some one call it for a bridge without subordinate.

Yinghai

Re: [PATCH 06/23] PCI: rescan with bus or bridge using callback method too

[Yinghai Lu]

On Fri, Mar 9, 2012 at 9:22 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> Why did you just repeat my proposal, changing "err" to "ret"?  I
> missed the point you're trying to make.  The name "err" gives an
> important clue that device_schedule_callback() returns 0 or errno.
> That's essential to understanding the code.
>
> Your original patch has one "return."  Mine proposal has three.  All
> that means is that the reader of your patch has to manage more stuff
> in her head to understand the code.  That's not a benefit!

ok, can we have separated patch to fix them all?

Yinghai

Re: [PATCH 00/23] PCI, x86: pci root bus hotplug support

[Bjorn Helgaas]

On Fri, Mar 9, 2012 at 11:55 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Fri, Mar 9, 2012 at 9:34 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Fri, Mar 9, 2012 at 1:19 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> On Thu, Mar 8, 2012 at 4:43 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>>>> The notes about "-v2, -v3, -v4, etc." are not really useful in the
>>>> commit changelogs.  They're helpful in the [00/] message, but not in
>>>> the changelogs.
>>>
>>> But Jesse may still need mark to figure out which one is latest?
>>
>> The way I do that is with "stg mail -v v3", which puts "[PATCH v3
>> 01/10]" in the subject.  IMO, that's much more useful because when you
>> post a 23-patch series, I don't want to be bothered with v1, v2, v3 on
>> a per-patch basis.
>
> For that, I could just sed to replace PATCH to PATCH v3
>
>>
>> It's true that when you post v2, many of those 23 patches will be
>> unchanged from v1, and you do have to describe that in the [v2 00/23]
>> mail.  But at least it's easy to identify the entire set of v2
>> patches.
>
> Version number of the entire set could be different from individual patch.
> That could be more confusing.

You already post the entire series several times, so you have series
v1, series v2, etc.  Trying to talk about series v2 and patch v3
within that series is too complicated.  The version number of the
entire series is enough, in my opinion.

Bjorn

Re: [PATCH 06/23] PCI: rescan with bus or bridge using callback method too

[Bjorn Helgaas]

On Fri, Mar 9, 2012 at 12:05 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Fri, Mar 9, 2012 at 9:22 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>> Why did you just repeat my proposal, changing "err" to "ret"?  I
>> missed the point you're trying to make.  The name "err" gives an
>> important clue that device_schedule_callback() returns 0 or errno.
>> That's essential to understanding the code.
>>
>> Your original patch has one "return."  Mine proposal has three.  All
>> that means is that the reader of your patch has to manage more stuff
>> in her head to understand the code.  That's not a benefit!
>
> ok, can we have separated patch to fix them all?

Sure.

Re: [PATCH 00/23] PCI, x86: pci root bus hotplug support

[Yinghai Lu]

On Fri, Mar 9, 2012 at 11:10 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, Mar 9, 2012 at 11:55 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>
> You already post the entire series several times, so you have series
> v1, series v2, etc.  Trying to talk about series v2 and patch v3
> within that series is too complicated.  The version number of the
> entire series is enough, in my opinion.

I like to keep the history about patch reviewing in the change log.

Yinghai