PCI: hotplug related misc patches

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Yinghai Lu
Sent: Saturday, June 23, 2012 4:42 PM
To: Bjorn Helgaas
Cc: [email protected]; Yinghai Lu
Subject: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

If surprise removal is not supported, that event get dropped later. So there is no point to enable that.

Also some sick chipset have those bit flip around when the card is not present.
and make log full of useless warning.

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/pci/hotplug/pciehp_hpc.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

drivers/pci/hotplug/pciehp_hpc.c +3 -3

index a960fae..d4fa705 100644
@@ -808,7 +808,7 @@ int pciehp_get_cur_lnk_width(struct slot *slot,
 
 int pcie_enable_notification(struct controller *ctrl)
 {
-	u16 cmd, mask;
+	u16 cmd = 0, mask;
 
 	/*
 	 * TBD: Power fault detected software notification support.

@@ -820,7 +820,8 @@ int pcie_enable_notification(struct controller *ctrl)
 	 * when it is cleared in the interrupt service routine, and
 	 * next power fault detected interrupt was notified again.
 	 */
-	cmd = PCI_EXP_SLTCTL_PDCE;
+	if (HP_SUPR_RM(ctrl))
+		cmd |= PCI_EXP_SLTCTL_PDCE;
 	if (ATTN_BUTTN(ctrl))
 		cmd |= PCI_EXP_SLTCTL_ABPE;
 	if (MRL_SENS(ctrl))
--

1.7.7

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Yinghai Lu
Sent: Saturday, June 23, 2012 4:42 PM
To: Bjorn Helgaas
Cc: [email protected]; Yinghai Lu
Subject: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

Quoted 2 lines

If surprise removal is not supported, that event get dropped later. So there is no point to enable that.

Presence changed event is currently just for printing a kernel message when surprise removal is not supported, and I think the message is not so important. So I agree this change.

Acked-by Kenji Kaneshige <[email protected]>

Quoted 5 lines

Also some sick chipset have those bit flip around when the card is not present.
and make log full of useless warning.

Is there any other way to avoid this issue other than disabling presence changed notification? If we need enabling presence changed event in the future, this will happen again. I'm worrying about this.

If surprise removal is not supported, that event get dropped later. So there is no point to enable that.

Also some sick chipset have those bit flip around when the card is not present. and make log full of useless warning.

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/pci/hotplug/pciehp_hpc.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

drivers/pci/hotplug/pciehp_hpc.c +3 -3

index a960fae..d4fa705 100644
@@ -808,7 +808,7 @@ int pciehp_get_cur_lnk_width(struct slot *slot,
 
 int pcie_enable_notification(struct controller *ctrl)
 {
-       u16 cmd, mask;
+       u16 cmd = 0, mask;
 
        /*
         * TBD: Power fault detected software notification support.

@@ -820,7 +820,8 @@ int pcie_enable_notification(struct controller *ctrl)
         * when it is cleared in the interrupt service routine, and
         * next power fault detected interrupt was notified again.
         */
-       cmd = PCI_EXP_SLTCTL_PDCE;
+       if (HP_SUPR_RM(ctrl))
+               cmd |= PCI_EXP_SLTCTL_PDCE;
        if (ATTN_BUTTN(ctrl))
                cmd |= PCI_EXP_SLTCTL_ABPE;
        if (MRL_SENS(ctrl))
--

1.7.7

Quoted 5 lines · Yinghai Lu

If surprise removal is not supported, that event get dropped later. So there is no point to enable that.

Also some sick chipset have those bit flip around when the card is not present. and make log full of useless warning.

HP_SUPR_RM tests the Slot Capabilities "Hot-Plug Surprise" bit, which indicates that an adapter might be *removed* without prior notification (sec 7.8.9).

PCI_EXP_SLTCTL_PDCE is the Slot Control "Presence Detect Changed Enable" bit, which enables interrupts for any change in the Slot Status "Presence Detect State", i.e., we may get interrupts for either add or remove events.

In interrupt_event_handler(), we drop both add and remove events if !HP_SUPR_RM(). Specifically, we drop *add* events if surprise *removal* isn't supported. That seems strange -- just from reading the spec, it seems that a surprise *add* could occur even if the "Hot-Plug Surprise" bit is not set.

So I'm not convinced that we should even bother looking at the "Hot-Plug Surprise" bit... Maybe we'd be better off if we just removed that HP_SUPR_RM() test in interrupt_event_handler() and always called handle_surprise_event().

Quoted 16 lines · Bjorn Helgaas

Quoted 5 lines · Yinghai Lu

If surprise removal is not supported, that event get dropped later. So there is no point to enable that.

Also some sick chipset have those bit flip around when the card is not present. and make log full of useless warning.

HP_SUPR_RM tests the Slot Capabilities "Hot-Plug Surprise" bit, which indicates that an adapter might be *removed* without prior notification (sec 7.8.9).

PCI_EXP_SLTCTL_PDCE is the Slot Control "Presence Detect Changed Enable" bit, which enables interrupts for any change in the Slot Status "Presence Detect State", i.e., we may get interrupts for either add or remove events.

Quoted 5 lines

In interrupt_event_handler(), we drop both add and remove events if !HP_SUPR_RM(). Specifically, we drop *add* events if surprise *removal* isn't supported. That seems strange -- just from reading the spec, it seems that a surprise *add* could occur even if the "Hot-Plug Surprise" bit is not set.

Interesting, that means after card is plugged in, we don't need to press attention button to bring
it online.

And other oses like windows and solaris do need to press the button like current linux doing.

Quoted 4 lines

So I'm not convinced that we should even bother looking at the "Hot-Plug Surprise" bit... Maybe we'd be better off if we just removed that HP_SUPR_RM() test in interrupt_event_handler() and always called handle_surprise_event().

Ah, that means some system with chip problem will keep feeding pciehp_wq with pciehp_disable_slot and pciehp_enable_slot.

Anyway please drop this patch now.

Quoted 4 lines · Yinghai Lu

Interesting, that means after card is plugged in, we don't need to press attention button to bring
it online.

Quoted 2 lines

And other oses like windows and solaris do need to press the button like current linux doing.

The attention button is apparently optional, depending on the form factor.

I would guess that on a platform with no attention button and Hot-Plug Surprise == 0, Linux would be unable to do a hot-add.

Quoted 5 lines · Yinghai Lu

If surprise removal is not supported, that event get dropped later. So there is no point to enable that.

Also some sick chipset have those bit flip around when the card is not present. and make log full of useless warning.

HP_SUPR_RM tests the Slot Capabilities "Hot-Plug Surprise" bit, which indicates that an adapter might be *removed* without prior notification (sec 7.8.9).

PCI_EXP_SLTCTL_PDCE is the Slot Control "Presence Detect Changed Enable" bit, which enables interrupts for any change in the Slot Status "Presence Detect State", i.e., we may get interrupts for either add or remove events.

In interrupt_event_handler(), we drop both add and remove events if !HP_SUPR_RM(). Specifically, we drop *add* events if surprise *removal* isn't supported. That seems strange -- just from reading the spec, it seems that a surprise *add* could occur even if the "Hot-Plug Surprise" bit is not set.

So I'm not convinced that we should even bother looking at the "Hot-Plug Surprise" bit... Maybe we'd be better off if we just removed that HP_SUPR_RM() test in interrupt_event_handler() and always called handle_surprise_event().

On Tue, Jul 10, 2012 at 04:56:15PM -0600, Bjorn Helgaas wrote:

What bad things would happen if we did the following?

I have no way to test this, but I don't understand what benefit there is in testing HP_SUPR_RM() here.

drivers/pci/hotplug/pciehp_ctrl.c +0 -0

index 27f4429..4bbe257 100644
@@ -463,9 +463,8 @@ static void interrupt_event_handler(struct work_struct *work)
                break;

Quoted 13 lines · Bjorn Helgaas

On Tue, Jul 10, 2012 at 04:56:15PM -0600, Bjorn Helgaas wrote:

What bad things would happen if we did the following?

I have no way to test this, but I don't understand what benefit there is in testing HP_SUPR_RM() here.

drivers/pci/hotplug/pciehp_ctrl.c +0 -0

index 27f4429..4bbe257 100644
@@ -463,9 +463,8 @@ static void interrupt_event_handler(struct work_struct *work)
                break;

drivers/pci/hotplug/pciehp_ctrl.c static void interrupt_event_handler(struct work_struct *work) · line 470

Quoted 10 lines

drivers/pci/hotplug/pciehp_ctrl.c +2 -3

        case INT_PRESENCE_ON:
        case INT_PRESENCE_OFF:
-               if (!HP_SUPR_RM(ctrl))
-                       break;
-               ctrl_dbg(ctrl, "Surprise Removal\n");
+               ctrl_dbg(ctrl, "Presence Detect changed (now %spresent)\n",
+                        info->event_type == INT_PRESENCE_OFF ? "not " : "");
                handle_surprise_event(p_slot);
                break;
        default:

some chipsets (from cpu vendor) have some problem, when you remove the card, the present bit
will keep flip around.
Because that present bit is "OR" with inband (pcie link) and outband (input from VPP) detection.
and silicon is trying to retrain the link to see if there is any device there. That means handle_surprise_event would get called frequently.

What's the connection with HP_SUPR_RM()? Is it just a coincidence that chipsets that set the "Hot-Plug Surprise" bit don't have this problem with the Presence Detect State bit?

Using HP_SUPR_RM() seems like a totally bogus way to work around a presence detect issue.

Quoted 7 lines · Bjorn Helgaas

What's the connection with HP_SUPR_RM()? Is it just a coincidence that chipsets that set the "Hot-Plug Surprise" bit don't have this problem with the Presence Detect State bit?

Using HP_SUPR_RM() seems like a totally bogus way to work around a presence detect issue.

then we should blame the spec.

and if you do the above changing, when plug the card into system, kernel will bring that card online automatically without press attention button.
that will be big change.

Quoted 10 lines · Yinghai Lu

Quoted 7 lines · Bjorn Helgaas

What's the connection with HP_SUPR_RM()? Is it just a coincidence that chipsets that set the "Hot-Plug Surprise" bit don't have this problem with the Presence Detect State bit?

Using HP_SUPR_RM() seems like a totally bogus way to work around a presence detect issue.

then we should blame the spec.

What specifically are you referring to? I see this Presence Detect State text:

Presence Detect State – This bit indicates the presence of an adapter in the slot, reflected by the logical “OR” of the Physical Layer in-band presence detect mechanism and, if present, any out-of-band presence detect mechanism defined for the slot’s corresponding form factor. Note that the in-band presence detect mechanism requires that power be applied to an adapter for its presence to be detected. Consequently, form factors that require a power controller for hot-plug must implement a physical pin presence detect mechanism.

But I don't yet see the connection with the Hot-Plug Surprise bit.

Quoted 5 lines

and if you do the above changing, when plug the card into system, kernel will bring that card online automatically without press attention button.
that will be big change.

I don't want to make a fundamental change in behavior like that. I'm just trying to understand why we should handle Presence Detect differently based on the Hot-Plug Surprise bit.

The attention button is optional. What happens today when you plug a card into a slot with no attention button?

Quoted 24 lines · Bjorn Helgaas

Quoted 10 lines · Yinghai Lu

Quoted 7 lines · Bjorn Helgaas

What's the connection with HP_SUPR_RM()? Is it just a coincidence that chipsets that set the "Hot-Plug Surprise" bit don't have this problem with the Presence Detect State bit?

Using HP_SUPR_RM() seems like a totally bogus way to work around a presence detect issue.

then we should blame the spec.

What specifically are you referring to? I see this Presence Detect State text:

Presence Detect State – This bit indicates the presence of an adapter in the slot, reflected by the logical “OR” of the Physical Layer in-band presence detect mechanism and, if present, any out-of-band presence detect mechanism defined for the slot’s corresponding form factor. Note that the in-band presence detect mechanism requires that power be applied to an adapter for its presence to be detected. Consequently, form factors that require a power controller for hot-plug must implement a physical pin presence detect mechanism.

Quoted 1 line

But I don't yet see the connection with the Hot-Plug Surprise bit.

Spec does not mention about surprise add clearly.

Quoted 5 lines

Quoted 4 lines

and if you do the above changing, when plug the card into system, kernel will bring that card online automatically without press attention button.
that will be big change.

Quoted 3 lines

I don't want to make a fundamental change in behavior like that. I'm just trying to understand why we should handle Presence Detect differently based on the Hot-Plug Surprise bit.

When hotplug surprise is supported, attention button may not there. So have to use present bit to trigger the sequence online work, and offline clean up work.

When hotplug surprise is not there, why should we handle Presence Bit change?

Quoted 3 lines

The attention button is optional. What happens today when you plug a card into a slot with no attention button?

if the slot support hotplug surprise, that card will be online automatically. if the slot does not support hotplug surprise, but that slot have power control, then could use need sw interface /sys/..../power to turn on the power and bring that card online.

Yinghai

Quoted 4 lines

When hotplug surprise is supported, attention button may not there. So have to use present bit to trigger the sequence online work, and offline clean up work.

Well, there is an "Attention Button Present" bit. Why wouldn't we use that instead of trying to infer the button's presence from Hot-Plug Surprise?

Quoted 8 lines · Bjorn Helgaas

Quoted 4 lines

When hotplug surprise is supported, attention button may not there. So have to use present bit to trigger the sequence online work, and offline clean up work.

Well, there is an "Attention Button Present" bit. Why wouldn't we use that instead of trying to infer the button's presence from Hot-Plug Surprise?

so you want this ?

drivers/pci/hotplug/pciehp_ctrl.c +1 -1

index 27f4429..f103a4ca 100644
@@ -463,7 +463,7 @@ static void interrupt_event_handler(struct
 work_struct *work)
                break;
        case INT_PRESENCE_ON:
        case INT_PRESENCE_OFF:
-               if (!HP_SUPR_RM(ctrl))
+               if (ATTN_BUTTN(ctrl))
                        break;
                ctrl_dbg(ctrl, "Surprise Removal\n");

                handle_surprise_event(p_slot);

that should keep current user expected behavior that kernel only bring the card online after press the button.

If we *do* want to change something there, I don't like the proposal above any better. It's still basically saying "presence detect is only reliable when X is set" when X is not clearly related to presence detect.

I think it's better to disable the presence detect interrupt completely if it's not reliable, as your original patch did. My complaint with that is that HP_SUPR_RM() doesn't seem like the right test for "the interrupt is not reliable."

Having a "Presence Detect State" bit and an interrupt that tells you when it changed is only meaningful if that bit gives you useful information. If hardware supplies that bit but it toggles all the time when the slot is empty because it's hooked up to link training attempts, that just means the hardware screwed up. The hardware *should* have included some logic to filter out the attempts and toggle the bit only when a card is actually added or removed. I believe the functionality of "Presence Detect State" is logically independent of "Hot-Plug Surprise" and "Attention Button Present."

So if we want to disable the "Presence Detect Changed" interrupt, that's fine, but I think we should do it based on a quirk or blacklist, or based on the fact that we have no need for it. One reason to want the interrupt is if "Hot-Plug Surprise" is set, indicating that an adapter might be removed without notice, and if that's the only reason, we could use your original patch.

But if we
do, I think we should change interrupt_event_handler() to look something like this:

case INT_PRESENCE_ON:
if (!ATTN_BUTTN(ctrl))
handle_surprise_event(p_slot); /* omit this if you don't
think it's useful */

        break;
    case INT_PRESENCE_OFF:
        handle_surprise_event(p_slot);
        break;

If you did make a change like this, I propose (as a separate patch) passing info->event_type into handle_surprise_event(). We've already read the "Presence Detect State" bit, so there's no need for handle_surprise_event() to do it again.

Quoted 9 lines · Bjorn Helgaas

If we *do* want to change something there, I don't like the proposal above any better. It's still basically saying "presence detect is only reliable when X is set" when X is not clearly related to presence detect.

I think it's better to disable the presence detect interrupt completely if it's not reliable, as your original patch did. My complaint with that is that HP_SUPR_RM() doesn't seem like the right test for "the interrupt is not reliable."

ok.

Quoted 10 lines

Having a "Presence Detect State" bit and an interrupt that tells you when it changed is only meaningful if that bit gives you useful information. If hardware supplies that bit but it toggles all the time when the slot is empty because it's hooked up to link training attempts, that just means the hardware screwed up. The hardware *should* have included some logic to filter out the attempts and toggle the bit only when a card is actually added or removed. I believe the functionality of "Presence Detect State" is logically independent of "Hot-Plug Surprise" and "Attention Button Present."

the cpu vendor already agreed that is out of spec for that.

Quoted 7 lines

So if we want to disable the "Presence Detect Changed" interrupt, that's fine, but I think we should do it based on a quirk or blacklist, or based on the fact that we have no need for it. One reason to want the interrupt is if "Hot-Plug Surprise" is set, indicating that an adapter might be removed without notice, and if that's the only reason, we could use your original patch.

no, with that patch, we will not get interrupt for present bit change for non-hotplug-surprise
case.

Quoted 5 lines

But if we
do, I think we should change interrupt_event_handler() to look something like this:

case INT_PRESENCE_ON:

Quoted 5 lines

if (!ATTN_BUTTN(ctrl))
handle_surprise_event(p_slot); /* omit this if you don't
think it's useful */

        break;
    case INT_PRESENCE_OFF:

Quoted 2 lines

        handle_surprise_event(p_slot);
        break;

yes, this one should be good. and it is enhancement.

Quoted 4 lines

If you did make a change like this, I propose (as a separate patch) passing info->event_type into handle_surprise_event(). We've already read the "Presence Detect State" bit, so there's no need for handle_surprise_event() to do it again.

ok. will prepare patches for that.

Yinghai

Quoted 35 lines · Yinghai Lu

Quoted 9 lines · Bjorn Helgaas

If we *do* want to change something there, I don't like the proposal above any better. It's still basically saying "presence detect is only reliable when X is set" when X is not clearly related to presence detect.

I think it's better to disable the presence detect interrupt completely if it's not reliable, as your original patch did. My complaint with that is that HP_SUPR_RM() doesn't seem like the right test for "the interrupt is not reliable."

ok.

Quoted 10 lines

Having a "Presence Detect State" bit and an interrupt that tells you when it changed is only meaningful if that bit gives you useful information. If hardware supplies that bit but it toggles all the time when the slot is empty because it's hooked up to link training attempts, that just means the hardware screwed up. The hardware *should* have included some logic to filter out the attempts and toggle the bit only when a card is actually added or removed. I believe the functionality of "Presence Detect State" is logically independent of "Hot-Plug Surprise" and "Attention Button Present."

the cpu vendor already agreed that is out of spec for that.

Quoted 7 lines

So if we want to disable the "Presence Detect Changed" interrupt, that's fine, but I think we should do it based on a quirk or blacklist, or based on the fact that we have no need for it. One reason to want the interrupt is if "Hot-Plug Surprise" is set, indicating that an adapter might be removed without notice, and if that's the only reason, we could use your original patch.

Quoted 9 lines

no, with that patch, we will not get interrupt for present bit change for non-hotplug-surprise
case.

Quoted 5 lines

But if we
do, I think we should change interrupt_event_handler() to look something like this:

case INT_PRESENCE_ON:

        ctrl_info(ctrl, "adapter now present\n");

Quoted 5 lines

Quoted 5 lines

if (!ATTN_BUTTN(ctrl))
handle_surprise_event(p_slot); /* omit this if you don't
think it's useful */

        break;
    case INT_PRESENCE_OFF:

        ctrl_warn(ctrl, "adapter removed unexpectedly\n");

Quoted 4 lines

Quoted 3 lines

        handle_surprise_event(p_slot);
        break;

Quoted 1 line

yes, this one should be good. and it is enhancement.

When we don't have an attention button, I don't know that it's a good idea to automatically power up a new card. I was thinking about things like ExpressCard, where the standard usage model probably *is* "plug in the card and it automatically starts working, no button press or software UI required." But my guess is that ExpressCard would not be handled by pciehp, would it?

The pciehp flow seems to be basically the same as SHPC, and the Standard Hot-Plug Controller spec, rev 1.0, sec 2.5, has a flow of hot-add without an attention button. It shows:

1) User selects empty, disabled slot and opens MRL
2) Adapter insertion:
2a) User inserts add-in-card
2b) User closes MRL
2c) User attaches cables
3) User requests via software UI that slot be enabled

We'll get the Presence Detect interrupt at step 2a, before the user can close the MRL or attach cables. I don't know if it's a good thing to power-on the slot and attach the driver while this is happening. It certainly doesn't seem to follow the intent of the spec.

Quoted 54 lines · Bjorn Helgaas

Quoted 35 lines · Yinghai Lu

Quoted 9 lines · Bjorn Helgaas

If we *do* want to change something there, I don't like the proposal above any better. It's still basically saying "presence detect is only reliable when X is set" when X is not clearly related to presence detect.

I think it's better to disable the presence detect interrupt completely if it's not reliable, as your original patch did. My complaint with that is that HP_SUPR_RM() doesn't seem like the right test for "the interrupt is not reliable."

ok.

Quoted 10 lines

Having a "Presence Detect State" bit and an interrupt that tells you when it changed is only meaningful if that bit gives you useful information. If hardware supplies that bit but it toggles all the time when the slot is empty because it's hooked up to link training attempts, that just means the hardware screwed up. The hardware *should* have included some logic to filter out the attempts and toggle the bit only when a card is actually added or removed. I believe the functionality of "Presence Detect State" is logically independent of "Hot-Plug Surprise" and "Attention Button Present."

the cpu vendor already agreed that is out of spec for that.

Quoted 7 lines

So if we want to disable the "Presence Detect Changed" interrupt, that's fine, but I think we should do it based on a quirk or blacklist, or based on the fact that we have no need for it. One reason to want the interrupt is if "Hot-Plug Surprise" is set, indicating that an adapter might be removed without notice, and if that's the only reason, we could use your original patch.

Quoted 9 lines

no, with that patch, we will not get interrupt for present bit change for non-hotplug-surprise
case.

Quoted 5 lines

But if we
do, I think we should change interrupt_event_handler() to look something like this:

case INT_PRESENCE_ON:

        ctrl_info(ctrl, "adapter now present\n");

Quoted 5 lines

Quoted 5 lines

if (!ATTN_BUTTN(ctrl))
handle_surprise_event(p_slot); /* omit this if you don't
think it's useful */

        break;
    case INT_PRESENCE_OFF:

Quoted 1 line

        ctrl_warn(ctrl, "adapter removed unexpectedly\n");

or keep the old

ctrl_dbg(ctrl, "Surprise Removal\n");

Quoted 11 lines

Quoted 4 lines

Quoted 2 lines

        handle_surprise_event(p_slot);
        break;

yes, this one should be good. and it is enhancement.

When we don't have an attention button, I don't know that it's a good idea to automatically power up a new card. I was thinking about things like ExpressCard, where the standard usage model probably *is* "plug in the card and it automatically starts working, no button press or software UI required." But my guess is that ExpressCard would not be handled by pciehp, would it?

some are by pciehp.

but my T420 BIOS _OSC doesn't give away pcie cap access, so pciehp can not be loaded.
Try to file BIOS bug to the vendor, find no where to do that.

Quoted 12 lines

The pciehp flow seems to be basically the same as SHPC, and the Standard Hot-Plug Controller spec, rev 1.0, sec 2.5, has a flow of hot-add without an attention button. It shows:

1) User selects empty, disabled slot and opens MRL
2) Adapter insertion:
2a) User inserts add-in-card
2b) User closes MRL
2c) User attaches cables
3) User requests via software UI that slot be enabled

Quoted 5 lines

We'll get the Presence Detect interrupt at step 2a, before the user can close the MRL or attach cables. I don't know if it's a good thing to power-on the slot and attach the driver while this is happening. It certainly doesn't seem to follow the intent of the spec.

I think we should just keep old behavior like user need to press the attention button.
and other os do that same way.

On Thu, Jul 12, 2012 at 6:19 PM, Yinghai Lu <[email protected]> wrote:

The question is what to do when a new card appears and there's no attention button. In general I think we should follow the SHPC flow and wait for some software equivalent of a button push, e.g., a sysfs poke.

But if pciehp handles ExpressCards, that doesn't sound like the desired user experience -- if we insert an ExpressCard in a laptop, don't we expect it to just start working, without having to poke around in software?

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 <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>

Acked-by Jason Baron <[email protected]>

---
 drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

drivers/pci/hotplug/acpiphp_glue.c +23 -0

index ad6fd66..0f2b72d 100644
@@ -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

drivers/pci/hotplug/acpiphp_glue.c +1 -1

                                        pci_bus_size_bridges(dev->subordinate);
+                               }
                        }
                }
        }
--

1.7.7

Quoted 49 lines · 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 <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>

Acked-by Jason Baron <[email protected]>

---
 drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

drivers/pci/hotplug/acpiphp_glue.c +23 -0

index ad6fd66..0f2b72d 100644
@@ -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

drivers/pci/hotplug/acpiphp_glue.c static int __ref enable_device(struct acpiphp_slot *slot) · line 844

Quoted 6 lines

drivers/pci/hotplug/acpiphp_glue.c +2 -1

@@ -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);

I don't like this patch because it increases the differences between the hotplug drivers, rather than decreasing them.

For PCI Express devices, we set dev->is_hotplug_bridge in the pci_setup_device() path (in set_pcie_hotplug_bridge()). I think it would make sense to try to expand that path to also handle SHPC and ACPI hotplug as well. ACPI is harder because it's not PCI-specified, so we'd need some sort of pcibios or other optional hook.

I don't have a clear picture of how this works -- if I understand correctly, the situation is that we have a bridge managed by acpiphp. That part makes sense because the bridge is on the motherboard and can have a DSDT device. Now we plug something into the slot below the bridge. I *think* this patch handles the case where this new hot-added thing is also a bridge managed by acpiphp. But where does the ACPI device for this hot-added bridge come from? It's an arbitrary device the BIOS knows nothing about, so it can't be in the DSDT.

Quoted 66 lines · Bjorn Helgaas

Quoted 49 lines · 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 <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>

Acked-by Jason Baron <[email protected]>

---
 drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

drivers/pci/hotplug/acpiphp_glue.c +23 -0

index ad6fd66..0f2b72d 100644
@@ -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

drivers/pci/hotplug/acpiphp_glue.c static int __ref enable_device(struct acpiphp_slot *slot) · line 844

Quoted 6 lines

drivers/pci/hotplug/acpiphp_glue.c +2 -1

@@ -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);

I don't like this patch because it increases the differences between the hotplug drivers, rather than decreasing them.

For PCI Express devices, we set dev->is_hotplug_bridge in the pci_setup_device() path (in set_pcie_hotplug_bridge()). I think it would make sense to try to expand that path to also handle SHPC and ACPI hotplug as well. ACPI is harder because it's not PCI-specified, so we'd need some sort of pcibios or other optional hook.

Quoted 10 lines

I don't have a clear picture of how this works -- if I understand correctly, the situation is that we have a bridge managed by acpiphp. That part makes sense because the bridge is on the motherboard and can have a DSDT device. Now we plug something into the slot below the bridge. I *think* this patch handles the case where this new hot-added thing is also a bridge managed by acpiphp. But where does the ACPI device for this hot-added bridge come from? It's an arbitrary device the BIOS knows nothing about, so it can't be in the DSDT.

So this came up while I was developing pci bridge hotplug for qemu. Currently, there is a top level host bus (with ACPI device definitions), where devices can be hot-plugged. What I've done is added a second level of hotplug pci busses (again with ACPI device definitions). Thus, we can now hotplug a bridge into the top-level bus and then devices behind it. Effectively increasing the hot-plug space from n -> n^2.

Before the above pci patch, the devices behind the bridge would not configure their PCI BARs properly, since there were no i/o, mem resources assigned to the bridge. However, with the above patch in place things work as expected.

Using the same code base I was able to do acpi hotplug on Windows 7, which correctly configured the both the bridge window and devices behind it on hot-plug. So currently, the above usage pattern works on Windows 7, but not on Linux.

Quoted 4 lines · Jason Baron

Using the same code base I was able to do acpi hotplug on Windows 7, which correctly configured the both the bridge window and devices behind it on hot-plug. So currently, the above usage pattern works on Windows 7, but not on Linux.

Thanks, Jason. Do you have "lspci -v" output and the DSDT AML handy? I'd like to look in more detail at what we're missing.

Quoted 90 lines · Jason Baron

Quoted 66 lines · Bjorn Helgaas

Quoted 49 lines · 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 <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>

Acked-by Jason Baron <[email protected]>

---
 drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

drivers/pci/hotplug/acpiphp_glue.c +23 -0

index ad6fd66..0f2b72d 100644
@@ -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

drivers/pci/hotplug/acpiphp_glue.c static int __ref enable_device(struct acpiphp_slot *slot) · line 844

Quoted 6 lines

drivers/pci/hotplug/acpiphp_glue.c +2 -1

@@ -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);

I don't like this patch because it increases the differences between the hotplug drivers, rather than decreasing them.

For PCI Express devices, we set dev->is_hotplug_bridge in the pci_setup_device() path (in set_pcie_hotplug_bridge()). I think it would make sense to try to expand that path to also handle SHPC and ACPI hotplug as well. ACPI is harder because it's not PCI-specified, so we'd need some sort of pcibios or other optional hook.

Quoted 10 lines

I don't have a clear picture of how this works -- if I understand correctly, the situation is that we have a bridge managed by acpiphp. That part makes sense because the bridge is on the motherboard and can have a DSDT device. Now we plug something into the slot below the bridge. I *think* this patch handles the case where this new hot-added thing is also a bridge managed by acpiphp. But where does the ACPI device for this hot-added bridge come from? It's an arbitrary device the BIOS knows nothing about, so it can't be in the DSDT.

So this came up while I was developing pci bridge hotplug for qemu. Currently, there is a top level host bus (with ACPI device definitions), where devices can be hot-plugged. What I've done is added a second level of hotplug pci busses (again with ACPI device definitions). Thus, we can now hotplug a bridge into the top-level bus and then devices behind it. Effectively increasing the hot-plug space from n -> n^2.

Before the above pci patch, the devices behind the bridge would not configure their PCI BARs properly, since there were no i/o, mem resources assigned to the bridge. However, with the above patch in place things work as expected.

Quoted 4 lines

Using the same code base I was able to do acpi hotplug on Windows 7, which correctly configured the both the bridge window and devices behind it on hot-plug. So currently, the above usage pattern works on Windows 7, but not on Linux.

Thanks, Jason. Do you have "lspci -v" output and the DSDT AML handy? I'd like to look in more detail at what we're missing.

Quoted 91 lines · Bjorn Helgaas

Quoted 90 lines · Jason Baron

Quoted 66 lines · Bjorn Helgaas

Quoted 49 lines · 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 <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>

Acked-by Jason Baron <[email protected]>

---
 drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

drivers/pci/hotplug/acpiphp_glue.c +23 -0

index ad6fd66..0f2b72d 100644
@@ -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

drivers/pci/hotplug/acpiphp_glue.c static int __ref enable_device(struct acpiphp_slot *slot) · line 844

Quoted 6 lines

drivers/pci/hotplug/acpiphp_glue.c +2 -1

@@ -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);

I don't like this patch because it increases the differences between the hotplug drivers, rather than decreasing them.

For PCI Express devices, we set dev->is_hotplug_bridge in the pci_setup_device() path (in set_pcie_hotplug_bridge()). I think it would make sense to try to expand that path to also handle SHPC and ACPI hotplug as well. ACPI is harder because it's not PCI-specified, so we'd need some sort of pcibios or other optional hook.

Quoted 10 lines

I don't have a clear picture of how this works -- if I understand correctly, the situation is that we have a bridge managed by acpiphp. That part makes sense because the bridge is on the motherboard and can have a DSDT device. Now we plug something into the slot below the bridge. I *think* this patch handles the case where this new hot-added thing is also a bridge managed by acpiphp. But where does the ACPI device for this hot-added bridge come from? It's an arbitrary device the BIOS knows nothing about, so it can't be in the DSDT.

So this came up while I was developing pci bridge hotplug for qemu. Currently, there is a top level host bus (with ACPI device definitions), where devices can be hot-plugged. What I've done is added a second level of hotplug pci busses (again with ACPI device definitions). Thus, we can now hotplug a bridge into the top-level bus and then devices behind it. Effectively increasing the hot-plug space from n -> n^2.

Before the above pci patch, the devices behind the bridge would not configure their PCI BARs properly, since there were no i/o, mem resources assigned to the bridge. However, with the above patch in place things work as expected.

Quoted 7 lines

Quoted 4 lines

Using the same code base I was able to do acpi hotplug on Windows 7, which correctly configured the both the bridge window and devices behind it on hot-plug. So currently, the above usage pattern works on Windows 7, but not on Linux.

Thanks, Jason. Do you have "lspci -v" output and the DSDT AML handy? I'd like to look in more detail at what we're missing.

Hi,

Sorry for the delay...was on vacation.

Anyways, below is the patch I have to the seabios acpi table. However, there are other pieces for seabios and qemu required. I still need to clean them up and send them out. But this pci patch (or something similar) is a required dependency.

What 'lspci -v' output are you looking for?

Let me know what else I can provide.

Quoted 111 lines · Jason Baron

Quoted 91 lines · Bjorn Helgaas

Quoted 90 lines · Jason Baron

Quoted 66 lines · Bjorn Helgaas

Quoted 49 lines · 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 <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>

Acked-by Jason Baron <[email protected]>

---
 drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

drivers/pci/hotplug/acpiphp_glue.c +23 -0

index ad6fd66..0f2b72d 100644
@@ -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

drivers/pci/hotplug/acpiphp_glue.c static int __ref enable_device(struct acpiphp_slot *slot) · line 844

Quoted 6 lines

drivers/pci/hotplug/acpiphp_glue.c +2 -1

@@ -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);

I don't like this patch because it increases the differences between the hotplug drivers, rather than decreasing them.

For PCI Express devices, we set dev->is_hotplug_bridge in the pci_setup_device() path (in set_pcie_hotplug_bridge()). I think it would make sense to try to expand that path to also handle SHPC and ACPI hotplug as well. ACPI is harder because it's not PCI-specified, so we'd need some sort of pcibios or other optional hook.

Quoted 10 lines

I don't have a clear picture of how this works -- if I understand correctly, the situation is that we have a bridge managed by acpiphp. That part makes sense because the bridge is on the motherboard and can have a DSDT device. Now we plug something into the slot below the bridge. I *think* this patch handles the case where this new hot-added thing is also a bridge managed by acpiphp. But where does the ACPI device for this hot-added bridge come from? It's an arbitrary device the BIOS knows nothing about, so it can't be in the DSDT.

So this came up while I was developing pci bridge hotplug for qemu. Currently, there is a top level host bus (with ACPI device definitions), where devices can be hot-plugged. What I've done is added a second level of hotplug pci busses (again with ACPI device definitions). Thus, we can now hotplug a bridge into the top-level bus and then devices behind it. Effectively increasing the hot-plug space from n -> n^2.

Before the above pci patch, the devices behind the bridge would not configure their PCI BARs properly, since there were no i/o, mem resources assigned to the bridge. However, with the above patch in place things work as expected.

Quoted 7 lines

Quoted 4 lines

Using the same code base I was able to do acpi hotplug on Windows 7, which correctly configured the both the bridge window and devices behind it on hot-plug. So currently, the above usage pattern works on Windows 7, but not on Linux.

Thanks, Jason. Do you have "lspci -v" output and the DSDT AML handy? I'd like to look in more detail at what we're missing.

Hi,

Sorry for the delay...was on vacation.

Anyways, below is the patch I have to the seabios acpi table. However, there are other pieces for seabios and qemu required. I still need to clean them up and send them out. But this pci patch (or something similar) is a required dependency.

What 'lspci -v' output are you looking for?

Quoted 1 line

Let me know what else I can provide.

Now it's my turn to be sorry for dropping this for so long :)

I was thinking of the "lspci -v" output from your system (qemu guest) and the disassembled DSDT extracted from the same system. Then we could talk about concrete devices and point to the corresponding entries in lspci output and the ACPI namespace.

Quoted 112 lines · Bjorn Helgaas

Quoted 111 lines · Jason Baron

Quoted 91 lines · Bjorn Helgaas

Quoted 90 lines · Jason Baron

Quoted 66 lines · Bjorn Helgaas

Quoted 49 lines · 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 <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>

Acked-by Jason Baron <[email protected]>

---
 drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

drivers/pci/hotplug/acpiphp_glue.c +23 -0

index ad6fd66..0f2b72d 100644
@@ -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

drivers/pci/hotplug/acpiphp_glue.c static int __ref enable_device(struct acpiphp_slot *slot) · line 844

Quoted 6 lines

drivers/pci/hotplug/acpiphp_glue.c +2 -1

@@ -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);

I don't like this patch because it increases the differences between the hotplug drivers, rather than decreasing them.

For PCI Express devices, we set dev->is_hotplug_bridge in the pci_setup_device() path (in set_pcie_hotplug_bridge()). I think it would make sense to try to expand that path to also handle SHPC and ACPI hotplug as well. ACPI is harder because it's not PCI-specified, so we'd need some sort of pcibios or other optional hook.

Quoted 10 lines

I don't have a clear picture of how this works -- if I understand correctly, the situation is that we have a bridge managed by acpiphp. That part makes sense because the bridge is on the motherboard and can have a DSDT device. Now we plug something into the slot below the bridge. I *think* this patch handles the case where this new hot-added thing is also a bridge managed by acpiphp. But where does the ACPI device for this hot-added bridge come from? It's an arbitrary device the BIOS knows nothing about, so it can't be in the DSDT.

So this came up while I was developing pci bridge hotplug for qemu. Currently, there is a top level host bus (with ACPI device definitions), where devices can be hot-plugged. What I've done is added a second level of hotplug pci busses (again with ACPI device definitions). Thus, we can now hotplug a bridge into the top-level bus and then devices behind it. Effectively increasing the hot-plug space from n -> n^2.

Before the above pci patch, the devices behind the bridge would not configure their PCI BARs properly, since there were no i/o, mem resources assigned to the bridge. However, with the above patch in place things work as expected.

Quoted 7 lines

Quoted 4 lines

Using the same code base I was able to do acpi hotplug on Windows 7, which correctly configured the both the bridge window and devices behind it on hot-plug. So currently, the above usage pattern works on Windows 7, but not on Linux.

Thanks, Jason. Do you have "lspci -v" output and the DSDT AML handy? I'd like to look in more detail at what we're missing.

Hi,

Sorry for the delay...was on vacation.

Anyways, below is the patch I have to the seabios acpi table. However, there are other pieces for seabios and qemu required. I still need to clean them up and send them out. But this pci patch (or something similar) is a required dependency.

What 'lspci -v' output are you looking for?

Quoted 8 lines

Quoted 1 line

Let me know what else I can provide.

Now it's my turn to be sorry for dropping this for so long :)

I was thinking of the "lspci -v" output from your system (qemu guest) and the disassembled DSDT extracted from the same system. Then we could talk about concrete devices and point to the corresponding entries in lspci output and the ACPI namespace.

Hi,

posted to: http://people.redhat.com/~jbaron/bridge-hotplug/

I've extracted the ssdt and dsdt tables.

Quoted 122 lines · Bjorn Helgaas

Quoted 112 lines · Jason Baron

Quoted 111 lines · Bjorn Helgaas

Quoted 91 lines · Jason Baron

Quoted 90 lines · Bjorn Helgaas

Quoted 66 lines · Yinghai Lu

Quoted 49 lines

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 <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>

Acked-by Jason Baron <[email protected]>

---
 drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

drivers/pci/hotplug/acpiphp_glue.c +23 -0

index ad6fd66..0f2b72d 100644
@@ -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

drivers/pci/hotplug/acpiphp_glue.c static int __ref enable_device(struct acpiphp_slot *slot) · line 844

Quoted 6 lines

drivers/pci/hotplug/acpiphp_glue.c +2 -1

@@ -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);

I don't like this patch because it increases the differences between the hotplug drivers, rather than decreasing them.

For PCI Express devices, we set dev->is_hotplug_bridge in the pci_setup_device() path (in set_pcie_hotplug_bridge()). I think it would make sense to try to expand that path to also handle SHPC and ACPI hotplug as well. ACPI is harder because it's not PCI-specified, so we'd need some sort of pcibios or other optional hook.

Quoted 10 lines

I don't have a clear picture of how this works -- if I understand correctly, the situation is that we have a bridge managed by acpiphp. That part makes sense because the bridge is on the motherboard and can have a DSDT device. Now we plug something into the slot below the bridge. I *think* this patch handles the case where this new hot-added thing is also a bridge managed by acpiphp. But where does the ACPI device for this hot-added bridge come from? It's an arbitrary device the BIOS knows nothing about, so it can't be in the DSDT.

So this came up while I was developing pci bridge hotplug for qemu. Currently, there is a top level host bus (with ACPI device definitions), where devices can be hot-plugged. What I've done is added a second level of hotplug pci busses (again with ACPI device definitions). Thus, we can now hotplug a bridge into the top-level bus and then devices behind it. Effectively increasing the hot-plug space from n -> n^2.

Before the above pci patch, the devices behind the bridge would not configure their PCI BARs properly, since there were no i/o, mem resources assigned to the bridge. However, with the above patch in place things work as expected.

Quoted 7 lines

Quoted 4 lines

Using the same code base I was able to do acpi hotplug on Windows 7, which correctly configured the both the bridge window and devices behind it on hot-plug. So currently, the above usage pattern works on Windows 7, but not on Linux.

Thanks, Jason. Do you have "lspci -v" output and the DSDT AML handy? I'd like to look in more detail at what we're missing.

Hi,

Sorry for the delay...was on vacation.

Anyways, below is the patch I have to the seabios acpi table. However, there are other pieces for seabios and qemu required. I still need to clean them up and send them out. But this pci patch (or something similar) is a required dependency.

What 'lspci -v' output are you looking for?

Quoted 8 lines

Quoted 1 line

Let me know what else I can provide.

Now it's my turn to be sorry for dropping this for so long :)

I was thinking of the "lspci -v" output from your system (qemu guest) and the disassembled DSDT extracted from the same system. Then we could talk about concrete devices and point to the corresponding entries in lspci output and the ACPI namespace.

Quoted 5 lines

Hi,

posted to: http://people.redhat.com/~jbaron/bridge-hotplug/

I've extracted the ssdt and dsdt tables.

Thanks, Jason.

I'd like to play with this on qemu myself so I don't have to ask so many stupid questions. Earlier you mentioned some other qemu and seabios patches. Are they usable yet? I see the src/ssdt-pcihp.dsl patch you posted Jul 17, but I think you were referring to some other ones.

Bjorn

minor changes that should be safe for 3.6

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

Yinghai Lu (5):
PCI, acpiphp: remove not used res_lock
pciehp: Don't enable presence notification while surprise removal is
not supported.
PCI, acpiphp: Merge acpiphp_debug and debug
PCI, acpiphp: add is_hotplug_bridge detection
PCI: add root bus children dev's res to fail list

 drivers/pci/hotplug/acpiphp.h      |    4 +---
 drivers/pci/hotplug/acpiphp_core.c |    7 ++-----
 drivers/pci/hotplug/acpiphp_glue.c |   30 ++++++++++++++++++++++++++----
 drivers/pci/hotplug/pciehp_hpc.c   |    5 +++--
 drivers/pci/setup-bus.c            |    2 +-
 5 files changed, 33 insertions(+), 15 deletions(-)

Quoted 8 lines · Yinghai Lu

minor changes that should be safe for 3.6

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

Yinghai Lu (5):
PCI, acpiphp: remove not used res_lock
PCI, acpiphp: Merge acpiphp_debug and debug

Quoted 4 lines

PCI, acpiphp: add is_hotplug_bridge detection
pciehp: Don't enable presence notification while surprise removal is
not supported.
PCI: add root bus children dev's res to fail list

Quoted 6 lines

 drivers/pci/hotplug/acpiphp.h      |    4 +---
 drivers/pci/hotplug/acpiphp_core.c |    7 ++-----
 drivers/pci/hotplug/acpiphp_glue.c |   30 ++++++++++++++++++++++++++----
 drivers/pci/hotplug/pciehp_hpc.c   |    5 +++--
 drivers/pci/setup-bus.c            |    2 +-
 5 files changed, 33 insertions(+), 15 deletions(-)

can you put those 5 patches into pci/next?

Those should be safe for v3.6 merging window.

Thanks

Yinghai