[PATCH part1 0/4] PCI: pci root bus hotplug support - part1

[PATCH part1 0/4] PCI: pci root bus hotplug support - part1

[Yinghai Lu]

It is based on pci/next

Add back some functions for pci remove.

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

Yinghai Lu (4):
  PCI, pciehp: Turn on link again after power off the slot
  PCI, acpiphp: Add is_hotplug_bridge detection
  PCI: Add root bus children dev's res to fail list
  PCI: Split out stop_bus_device and remove_bus_dev again.

 drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++-
 drivers/pci/hotplug/pciehp_hpc.c   |    2 +
 drivers/pci/remove.c               |   77 ++++++++++++++++++++++++++++-------
 drivers/pci/setup-bus.c            |    2 +-
 include/linux/pci.h                |    2 +
 5 files changed, 92 insertions(+), 18 deletions(-)

-- 
1.7.7

[PATCH part1 1/4] PCI, pciehp: Turn on link again after power off the slot

[Yinghai Lu]

During debugging hotplug surpise support, found slot status reg does not report
present status anymore after removing the card. That present bit
does not get cleared even that card is removed. So no interrupt is generated
later after reinsert the card.

That problem is caused by commit:
| commit 2debd9289997fc5d1c0043b41201a8b40d5e11d0
|
|    PCI: pciehp: Disable/enable link during slot power off/on

Try turn on link after power off.
With the fix present bit will report correctly, also still avoid the aer
during the power off.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
Cc: stable@vger.kernel.org
---
 drivers/pci/hotplug/pciehp_hpc.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 13b2eaf..7cee8db 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -631,6 +631,8 @@ int pciehp_power_off_slot(struct slot * slot)
 	slot_cmd = POWER_OFF;
 	cmd_mask = PCI_EXP_SLTCTL_PCC;
 	retval = pcie_write_cmd(ctrl, slot_cmd, cmd_mask);
+	/* need to enable link again for present bit report */
+	pciehp_link_enable(ctrl);
 	if (retval) {
 		ctrl_err(ctrl, "Write command failed!\n");
 		return retval;
-- 
1.7.7

[PATCH part1 2/4] PCI, acpiphp: Add is_hotplug_bridge detection

[Yinghai Lu]

When system support hotplug bridge with children hotplug slots, we need to make sure
that parent bridge get preallocated resource so later when device is plugged into
children slot, those children devices will get resource allocated.

We do not meet this problem, because for pcie hotplug card, when acpiphp is used,
pci_scan_bridge will set that for us when detect hotplug bit in slot cap.

Reported-and-tested-by: Jason Baron <jbaron@redhat.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Acked-by: Jason Baron <jbaron@redhat.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 7be4ca5..085eec7 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -783,6 +783,29 @@ static void acpiphp_set_acpi_region(struct acpiphp_slot *slot)
 	}
 }
 
+static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev)
+{
+	struct acpiphp_func *func;
+
+	if (!dev->subordinate)
+		return;
+
+	/* quirk, or pcie could set it already */
+	if (dev->is_hotplug_bridge)
+		return;
+
+	if (PCI_SLOT(dev->devfn) != slot->device)
+		return;
+
+	list_for_each_entry(func, &slot->funcs, sibling) {
+		if (PCI_FUNC(dev->devfn) == func->function) {
+			/* check if this bridge has ejectable slots */
+			if ((detect_ejectable_slots(func->handle) > 0))
+				dev->is_hotplug_bridge = 1;
+			break;
+		}
+	}
+}
 /**
  * enable_device - enable, configure a slot
  * @slot: slot to be enabled
@@ -817,8 +840,10 @@ static int __ref enable_device(struct acpiphp_slot *slot)
 			if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||
 			    dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) {
 				max = pci_scan_bridge(bus, dev, max, pass);
-				if (pass && dev->subordinate)
+				if (pass && dev->subordinate) {
+					check_hotplug_bridge(slot, dev);
 					pci_bus_size_bridges(dev->subordinate);
+				}
 			}
 		}
 	}
-- 
1.7.7

[PATCH part1 3/4] PCI: Add root bus children dev's res to fail list

[Yinghai Lu]

We can stop according to try number now and do not need to use root_bus checking
as stop sign anymore.

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

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index fb50613..f0e959c 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -283,7 +283,7 @@ static void assign_requested_resources_sorted(struct list_head *head,
 		idx = res - &dev_res->dev->resource[0];
 		if (resource_size(res) &&
 		    pci_assign_resource(dev_res->dev, idx)) {
-			if (fail_head && !pci_is_root_bus(dev_res->dev->bus)) {
+			if (fail_head) {
 				/*
 				 * if the failed res is for ROM BAR, and it will
 				 * be enabled later, don't add it to the list
-- 
1.7.7

[PATCH part1 4/4] PCI: Split out stop_bus_device and remove_bus_dev again.

[Yinghai Lu]

So add back some functions that are needed for pci root bus hotplug support.

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

diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 4f9ca91..aaae16e 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -56,25 +56,13 @@ void pci_remove_bus(struct pci_bus *bus)
 }
 EXPORT_SYMBOL(pci_remove_bus);
 
-/**
- * pci_stop_and_remove_bus_device - remove a PCI device and any children
- * @dev: the device to remove
- *
- * Remove a PCI device from the device lists, informing the drivers
- * that the device has been removed.  We also remove any subordinate
- * buses and children in a depth-first manner.
- *
- * For each device we remove, delete the device structure from the
- * device lists, remove the /proc entry, and notify userspace
- * (/sbin/hotplug).
- */
-void pci_stop_and_remove_bus_device(struct pci_dev *dev)
+static void pci_stop_bus_device(struct pci_dev *dev)
 {
 	struct pci_bus *bus = dev->subordinate;
 	struct pci_dev *child, *tmp;
 
 	/*
-	 * Removing an SR-IOV PF device removes all the associated VFs,
+	 * Stopping an SR-IOV PF device removes all the associated VFs,
 	 * which will update the bus->devices list and confuse the
 	 * iterator.  Therefore, iterate in reverse so we remove the VFs
 	 * first, then the PF.
@@ -82,13 +70,70 @@ void pci_stop_and_remove_bus_device(struct pci_dev *dev)
 	if (bus) {
 		list_for_each_entry_safe_reverse(child, tmp,
 						 &bus->devices, bus_list)
-			pci_stop_and_remove_bus_device(child);
+			pci_stop_bus_device(child);
+	}
+
+	pci_stop_dev(dev);
+}
+
+static void pci_remove_bus_device(struct pci_dev *dev)
+{
+	struct pci_bus *bus = dev->subordinate;
+	struct pci_dev *child, *tmp;
+
+	if (bus) {
+		list_for_each_entry_safe(child, tmp,
+					 &bus->devices, bus_list)
+			pci_remove_bus_device(child);
 
 		pci_remove_bus(bus);
 		dev->subordinate = NULL;
 	}
 
-	pci_stop_dev(dev);
 	pci_destroy_dev(dev);
 }
+
+/**
+ * pci_stop_and_remove_bus_device - remove a PCI device and any children
+ * @dev: the device to remove
+ *
+ * Remove a PCI device from the device lists, informing the drivers
+ * that the device has been removed.  We also remove any subordinate
+ * buses and children in a depth-first manner.
+ *
+ * For each device we remove, delete the device structure from the
+ * device lists, remove the /proc entry, and notify userspace
+ * (/sbin/hotplug).
+ */
+void pci_stop_and_remove_bus_device(struct pci_dev *dev)
+{
+	pci_stop_bus_device(dev);
+	pci_remove_bus_device(dev);
+}
 EXPORT_SYMBOL(pci_stop_and_remove_bus_device);
+
+void pci_stop_bus_devices(struct pci_bus *bus)
+{
+	struct pci_dev *child, *tmp;
+
+	list_for_each_entry_safe_reverse(child, tmp,
+					 &bus->devices, bus_list)
+		pci_stop_bus_device(child);
+}
+
+static void pci_remove_bus_devices(struct pci_bus *bus)
+{
+	struct pci_dev *child, *tmp;
+
+	list_for_each_entry_safe(child, tmp,
+				 &bus->devices, bus_list)
+		pci_remove_bus_device(child);
+}
+
+void pci_stop_and_remove_behind_bridge(struct pci_dev *dev)
+{
+	pci_stop_bus_device(dev);
+
+	if (dev->subordinate)
+		pci_remove_bus_devices(dev->subordinate);
+}
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 0d87189..1b460e1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -734,6 +734,8 @@ extern struct pci_dev *pci_dev_get(struct pci_dev *dev);
 extern void pci_dev_put(struct pci_dev *dev);
 extern void pci_remove_bus(struct pci_bus *b);
 extern void pci_stop_and_remove_bus_device(struct pci_dev *dev);
+void pci_stop_bus_devices(struct pci_bus *bus);
+void pci_stop_and_remove_behind_bridge(struct pci_dev *dev);
 void pci_setup_cardbus(struct pci_bus *bus);
 extern void pci_sort_breadthfirst(void);
 #define dev_is_pci(d) ((d)->bus == &pci_bus_type)
-- 
1.7.7

Re: [PATCH part1 1/4] PCI, pciehp: Turn on link again after power off the slot

[Prarit Bhargava]

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
Cc: stable@vger.kernel.org

>diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
>index 13b2eaf..7cee8db 100644
>--- a/drivers/pci/hotplug/pciehp_hpc.c
>+++ b/drivers/pci/hotplug/pciehp_hpc.c
>@@ -631,6 +631,8 @@ int pciehp_power_off_slot(struct slot * slot)
> 	slot_cmd = POWER_OFF;
> 	cmd_mask = PCI_EXP_SLTCTL_PCC;
> 	retval = pcie_write_cmd(ctrl, slot_cmd, cmd_mask);
>+	/* need to enable link again for present bit report */
>+	pciehp_link_enable(ctrl);

You're not checking the return value of pciehp_link_enable().  I wonder if
enabling the link is the only way to get the present bit to report correctly...

Also, should the pciehp_link_enable() be called in pciehp_power_on_slot() if
we already reenable it on the power off?

P.

Re: [PATCH part1 1/4] PCI, pciehp: Turn on link again after power off the slot

[Yinghai Lu]

On Thu, Sep 6, 2012 at 7:34 AM, Prarit Bhargava <prarit@redhat.com> wrote:
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
> Cc: stable@vger.kernel.org
>
>>diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
>>index 13b2eaf..7cee8db 100644
>>--- a/drivers/pci/hotplug/pciehp_hpc.c
>>+++ b/drivers/pci/hotplug/pciehp_hpc.c
>>@@ -631,6 +631,8 @@ int pciehp_power_off_slot(struct slot * slot)
>>       slot_cmd = POWER_OFF;
>>       cmd_mask = PCI_EXP_SLTCTL_PCC;
>>       retval = pcie_write_cmd(ctrl, slot_cmd, cmd_mask);
>>+      /* need to enable link again for present bit report */
>>+      pciehp_link_enable(ctrl);
>
> You're not checking the return value of pciehp_link_enable().  I wonder if
> enabling the link is the only way to get the present bit to report correctly...

that is silicon problem. and no one noticed that before.

>
> Also, should the pciehp_link_enable() be called in pciehp_power_on_slot() if
> we already reenable it on the power off?

we call that in pciehp_power_on_slot for another case:
BIOS could have link disable on pcie slot if there is no card plugged
in that slot.

Thanks

Yinghai

Re: [PATCH part1 1/4] PCI, pciehp: Turn on link again after power off the slot

[Bjorn Helgaas]

On Thu, Sep 6, 2012 at 11:30 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Sep 6, 2012 at 7:34 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> Cc: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
>> Cc: stable@vger.kernel.org
>>
>>>diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
>>>index 13b2eaf..7cee8db 100644
>>>--- a/drivers/pci/hotplug/pciehp_hpc.c
>>>+++ b/drivers/pci/hotplug/pciehp_hpc.c
>>>@@ -631,6 +631,8 @@ int pciehp_power_off_slot(struct slot * slot)
>>>       slot_cmd = POWER_OFF;
>>>       cmd_mask = PCI_EXP_SLTCTL_PCC;
>>>       retval = pcie_write_cmd(ctrl, slot_cmd, cmd_mask);
>>>+      /* need to enable link again for present bit report */
>>>+      pciehp_link_enable(ctrl);
>>
>> You're not checking the return value of pciehp_link_enable().  I wonder if
>> enabling the link is the only way to get the present bit to report correctly...
>
> that is silicon problem. and no one noticed that before.
>
>>
>> Also, should the pciehp_link_enable() be called in pciehp_power_on_slot() if
>> we already reenable it on the power off?
>
> we call that in pciehp_power_on_slot for another case:
> BIOS could have link disable on pcie slot if there is no card plugged
> in that slot.

So now we have two situations that should be the same, but they're not:

  1) Slot is empty at boot, and BIOS leaves link disabled.  Link
remains disabled until pciehp_power_on_slot() enables it.
  2) We hot-remove a card, so the slot is now empty, but
pciehp_power_off_slot() leaves the link enabled.  Link stays *enabled*
until another disable/enable sequence in pciehp_power_off_slot().

Why should these be different?  In both cases the slot is empty, but
in case 1 the link is disabled, while in case 2 it is enabled.  If we
can detect a hot-add in case 1, why can't we detect it after a
hot-remove even if the link is disabled?

Or maybe case 1 is broken on this box, and hot-add to that slot
doesn't work correctly either?

Re: [PATCH part1 1/4] PCI, pciehp: Turn on link again after power off the slot

[Yinghai Lu]

On Mon, Sep 17, 2012 at 4:06 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> So now we have two situations that should be the same, but they're not:
>
>   1) Slot is empty at boot, and BIOS leaves link disabled.  Link
> remains disabled until pciehp_power_on_slot() enables it.
>   2) We hot-remove a card, so the slot is now empty, but
> pciehp_power_off_slot() leaves the link enabled.  Link stays *enabled*
> until another disable/enable sequence in pciehp_power_off_slot().
>
> Why should these be different?  In both cases the slot is empty, but
> in case 1 the link is disabled, while in case 2 it is enabled.  If we
> can detect a hot-add in case 1, why can't we detect it after a
> hot-remove even if the link is disabled?

silicon problem. assume that case is not well tested.

we filed the bug to vendor, and they can not explain that so far.
system hotplug fpga report correct present status to the chipset.
looks like the silicon get confused somehow.

>
> Or maybe case 1 is broken on this box, and hot-add to that slot
> doesn't work correctly either?

works. with and without this patch.

Yinghai

Re: [PATCH part1 1/4] PCI, pciehp: Turn on link again after power off the slot

[Bjorn Helgaas]

On Mon, Sep 17, 2012 at 5:25 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Mon, Sep 17, 2012 at 4:06 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>> So now we have two situations that should be the same, but they're not:
>>
>>   1) Slot is empty at boot, and BIOS leaves link disabled.  Link
>> remains disabled until pciehp_power_on_slot() enables it.
>>   2) We hot-remove a card, so the slot is now empty, but
>> pciehp_power_off_slot() leaves the link enabled.  Link stays *enabled*
>> until another disable/enable sequence in pciehp_power_off_slot().
>>
>> Why should these be different?  In both cases the slot is empty, but
>> in case 1 the link is disabled, while in case 2 it is enabled.  If we
>> can detect a hot-add in case 1, why can't we detect it after a
>> hot-remove even if the link is disabled?
>
> silicon problem. assume that case is not well tested.
>
> we filed the bug to vendor, and they can not explain that so far.
> system hotplug fpga report correct present status to the chipset.
> looks like the silicon get confused somehow.
>
>>
>> Or maybe case 1 is broken on this box, and hot-add to that slot
>> doesn't work correctly either?
>
> works. with and without this patch.

This patch is just black magic then.  I don't want to intentionally
treat a slot that's empty at boot differently than one that is empty
after a hot-remove.

Re: [PATCH part1 1/4] PCI, pciehp: Turn on link again after power off the slot

[Yinghai Lu]

On Mon, Sep 17, 2012 at 4:52 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> This patch is just black magic then.  I don't want to intentionally
> treat a slot that's empty at boot differently than one that is empty
> after a hot-remove.

sure. Please drop this one.

Thanks

Yinghai