pci: Use hot-plug capable for testing presence on
PATCH 1/1 pci: Use hot-plug capable for testing presence on
The PCI-e Base specifically says Hot-Plug Surprise capability is for removal only, but pciehp checked this to determine if it should handle a slot event on device add. Lots of platform's pcie slots don't advertise support for surprise removal, but are perfectly capable of handling hot-add, so checking surprise removal is not appropriate.
This patch checks the Hot-Plug Capable bit in the slot capabilities to determine if the detected presence on is a reliable event instead of Hot-Plug Surprise.
--- drivers/pci/hotplug/pciehp.h | 1 + drivers/pci/hotplug/pciehp_ctrl.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)
drivers/pci/hotplug/pciehp.h +1 -0
@@ -126,6 +126,7 @@ struct controller {#define ATTN_LED (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_AIP)#define PWR_LED (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_PIP)#define HP_SUPR_RM (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_HPS) +#define HP_CAP (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_HPC)#define EMI (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_EIP)#define NO_CMD_CMPL (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_NCCS)#define PSN (ctrl) (((ctrl)->slot_cap & PCI_EXP_SLTCAP_PSN) >> 19)
drivers/pci/hotplug/pciehp_ctrl.c +1 -1
@@ -532,7 +532,7 @@ static void interrupt_event_handler(struct work_struct *work)pciehp_green_led_off (p_slot);break ;case INT_PRESENCE_ON : -if (!HP_SUPR_RM (ctrl)) +if (!HP_CAP (ctrl))break ;ctrl_dbg (ctrl,"Surprise Insertion\n" );handle_surprise_event (p_slot);
Signature · 1 line
1.7.10.4
Any thoughts on this? I've read old threads on the pci lists regarding the HPS slot capability usage being wrong for hot-add, but it didn't seem like it was closed.
Right now there are customers reporting hot-add doesn't work on Linux for this reason, and I'm struggling to come up with a rebuttal to the other OS developers claiming superiority over Linux for PCI-e hotplug on popular platforms.
Quoted 43 lines · Keith Busch
The PCI-e Base specifically says Hot-Plug Surprise capability is for removal only, but pciehp checked this to determine if it should handle a slot event on device add. Lots of platform's pcie slots don't advertise support for surprise removal, but are perfectly capable of handling hot-add, so checking surprise removal is not appropriate.
This patch checks the Hot-Plug Capable bit in the slot capabilities to determine if the detected presence on is a reliable event instead of Hot-Plug Surprise.
Signed-off-by Keith Busch <[email protected]>---
drivers/pci/hotplug/pciehp.h | 1 + drivers/pci/hotplug/pciehp_ctrl.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)drivers/pci/hotplug/pciehp.h +1
-0@@ -126,6 +126,7 @@ struct controller {#define ATTN_LED (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_AIP)#define PWR_LED (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_PIP)#define HP_SUPR_RM (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_HPS) +#define HP_CAP (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_HPC)#define EMI (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_EIP)#define NO_CMD_CMPL (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_NCCS)#define PSN (ctrl) (((ctrl)->slot_cap & PCI_EXP_SLTCAP_PSN) >> 19)drivers/pci/hotplug/pciehp_ctrl.c +1
-1@@ -532,7 +532,7 @@ static void interrupt_event_handler(struct work_struct *work)pciehp_green_led_off (p_slot);break ;case INT_PRESENCE_ON : -if (!HP_SUPR_RM (ctrl)) +if (!HP_CAP (ctrl))break ;ctrl_dbg (ctrl,"Surprise Insertion\n" );handle_surprise_event (p_slot);Signature · 1 line
1.7.10.4
Bjorn Helgaas
maintainer[+cc Rajat]
Quoted 11 lines · Keith Busch
The PCI-e Base specifically says Hot-Plug Surprise capability is for removal only, but pciehp checked this to determine if it should handle a slot event on device add. Lots of platform's pcie slots don't advertise support for surprise removal, but are perfectly capable of handling hot-add, so checking surprise removal is not appropriate.
This patch checks the Hot-Plug Capable bit in the slot capabilities to determine if the detected presence on is a reliable event instead of Hot-Plug Surprise.
Signed-off-by Keith Busch <[email protected]>
This seems reasonable to me, and I applied it to pci/hotplug for v3.20.
Does this fix a bug you've observed? If so, I'll include details or a pointer in the changelog.
2b3940b60626 ("PCI: pciehp: Remove a non-existent card, regardless of "surprise" capability") made it so we handle unexpected loss of presence detect even if the slot doesn't advertise Hot-Plug Surprise. I suppose we could make a similar argument that if a card shows up, maybe we should do something with it regardless of what the slot advertises for Hot-Plug Capable.
Quoted 37 lines
--- drivers/pci/hotplug/pciehp.h | 1 + drivers/pci/hotplug/pciehp_ctrl.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)drivers/pci/hotplug/pciehp.h +1
-0@@ -126,6 +126,7 @@ struct controller {#define ATTN_LED (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_AIP)#define PWR_LED (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_PIP)#define HP_SUPR_RM (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_HPS) +#define HP_CAP (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_HPC)#define EMI (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_EIP)#define NO_CMD_CMPL (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_NCCS)#define PSN (ctrl) (((ctrl)->slot_cap & PCI_EXP_SLTCAP_PSN) >> 19)drivers/pci/hotplug/pciehp_ctrl.c +1
-1@@ -532,7 +532,7 @@ static void interrupt_event_handler(struct work_struct *work)pciehp_green_led_off (p_slot);break ;case INT_PRESENCE_ON : -if (!HP_SUPR_RM (ctrl)) +if (!HP_CAP (ctrl))break ;ctrl_dbg (ctrl,"Surprise Insertion\n" );handle_surprise_event (p_slot);Signature · 6 lines
1.7.10.4 -- 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
Hello,
Quoted 4 lines
Quoted 2 lines
Signed-off-by Keith Busch <[email protected]>This seems reasonable to me, and I applied it to pci/hotplug for v3.20.
Yes, I agree.
Quoted 10 lines
Does this fix a bug you've observed? If so, I'll include details or a pointer in the changelog.
2b3940b60626("PCI: pciehp: Remove a non-existent card, regardless of "surprise" capability") made it so we handle unexpected loss of presence detect even if the slot doesn't advertise Hot-Plug Surprise. I suppose we could make a similar argument that if a card shows up, maybe we should do something with it regardless of what the slot advertises for Hot-Plug Capable.
Ummm ... may be not. From The PCI Hotplug Spec ver 1.1. section 2.2.2 (Hot Insertion)
==================================================================
A slot must be powered down and isolated from the bus before an add-in card can be
inserted. The process of making a slot ready for insertion depends upon the particular
Platform and operating system. The following general sequence of steps is necessary to
insert an add-in card into a slot after it is powered down and ready for insertion:
1. The user inserts the new add-in card.
2. The user notifies the Hot-Plug Service to turn on the slot containing the new add-in
card.
3. The Hot-Plug Service issues a Hot-Plug Primitive to the Hot-Plug System Driver to
turn on the appropriate slot.
===============================================================
Also, I recall I read somewhere in the specs about the need to use the button to initiate hot-plug. This change would make the button useless, no?
I'm thinking the user may have reasons to have the card plugged-in, but not have it turned on. The card may be a back-up for example, to be turned on (to enable an alternate path) in case of some other card fails etc. Or may be other application reasons. Thoughts?
Thanks,
Rajat
<span class="sewoi9i5w58ly4q"><br></span>>
Quoted 37 lines
Quoted 37 lines
--- drivers/pci/hotplug/pciehp.h | 1 + drivers/pci/hotplug/pciehp_ctrl.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)drivers/pci/hotplug/pciehp.h +1
-0@@ -126,6 +126,7 @@ struct controller {#define ATTN_LED (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_AIP)#define PWR_LED (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_PIP)#define HP_SUPR_RM (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_HPS) +#define HP_CAP (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_HPC)#define EMI (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_EIP)#define NO_CMD_CMPL (ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_NCCS)#define PSN (ctrl) (((ctrl)->slot_cap & PCI_EXP_SLTCAP_PSN) >> 19)drivers/pci/hotplug/pciehp_ctrl.c +1
-2@@ -532,7 +532,7 @@ static void interrupt_event_handler(struct work_struct *work)pciehp_green_led_off (p_slot);break ;case INT_PRESENCE_ON : -if (!HP_SUPR_RM (ctrl)) +if (!HP_CAP (ctrl))break ;ctrl_dbg (ctrl,"Surprise Insertion\n" );handle_surprise_event (p_slot); --1.7.10.4
--
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 28 lines · Rajat Jain
Quoted 6 lines
2b3940b60626("PCI: pciehp: Remove a non-existent card, regardless of "surprise" capability") made it so we handle unexpected loss of presence detect even if the slot doesn't advertise Hot-Plug Surprise. I suppose we could make a similar argument that if a card shows up, maybe we should do something with it regardless of what the slot advertises for Hot-Plug Capable.Ummm ... may be not. From The PCI Hotplug Spec ver 1.1. section 2.2.2 (Hot Insertion)
==================================================================
A slot must be powered down and isolated from the bus before an add-in card can be
inserted. The process of making a slot ready for insertion depends upon the particular
Platform and operating system. The following general sequence of steps is necessary to
insert an add-in card into a slot after it is powered down and ready for insertion:1. The user inserts the new add-in card.
2. The user notifies the Hot-Plug Service to turn on the slot containing the new add-in
card.
3. The Hot-Plug Service issues a Hot-Plug Primitive to the Hot-Plug System Driver to
turn on the appropriate slot.
===============================================================
Quoted 8 lines
Also, I recall I read somewhere in the specs about the need to use the button to initiate hot-plug. This change would make the button useless, no?
I'm thinking the user may have reasons to have the card plugged-in, but not have it turned on. The card may be a back-up for example, to be turned on (to enable an alternate path) in case of some other card fails etc. Or may be other application reasons. Thoughts?
Maybe I need to try this on an add-in card. I'm using front-loading SFF devices which don't have attention buttons. I would have expected these slots to have the hot-plug surprise capability set, but there's enough platforms in existnace that don't have it set, but can handle add events just fine otherwise. So while this patch fixes that scenario, I don't want to break others.
Bjorn Helgaas
maintainerQuoted 41 lines · Rajat Jain
Hello,
Quoted 4 lines
Quoted 2 lines
Signed-off-by Keith Busch <[email protected]>This seems reasonable to me, and I applied it to pci/hotplug for v3.20.
Yes, I agree.
Quoted 10 lines
Does this fix a bug you've observed? If so, I'll include details or a pointer in the changelog.
2b3940b60626("PCI: pciehp: Remove a non-existent card, regardless of "surprise" capability") made it so we handle unexpected loss of presence detect even if the slot doesn't advertise Hot-Plug Surprise. I suppose we could make a similar argument that if a card shows up, maybe we should do something with it regardless of what the slot advertises for Hot-Plug Capable.Ummm ... may be not. From The PCI Hotplug Spec ver 1.1. section 2.2.2 (Hot Insertion)
==================================================================
A slot must be powered down and isolated from the bus before an add-in card can be
inserted. The process of making a slot ready for insertion depends upon the particular
Platform and operating system. The following general sequence of steps is necessary to
insert an add-in card into a slot after it is powered down and ready for insertion:1. The user inserts the new add-in card.
2. The user notifies the Hot-Plug Service to turn on the slot containing the new add-in
card.
3. The Hot-Plug Service issues a Hot-Plug Primitive to the Hot-Plug System Driver to
turn on the appropriate slot.
===============================================================
Quoted 8 lines
Also, I recall I read somewhere in the specs about the need to use the button to initiate hot-plug. This change would make the button useless, no?
I'm thinking the user may have reasons to have the card plugged-in, but not have it turned on. The card may be a back-up for example, to be turned on (to enable an alternate path) in case of some other card fails etc. Or may be other application reasons. Thoughts?
If there's an Attention Button, we definitely want to leave the card powered off until the user presses the button. With an Attention Button, I think the flow looks like this:
- Slot is initially powered off
- Slot hardware includes Attention Button
- pcie_enable_notification() enables Attention Button interrupt but
not Presence Detect Changed interrupt
- User inserts card
- Port may detect card presence (but pciehp hasn't enabled interrupt)
- User presses attention button
- Port asserts Attention Button interrupt
- OS turns on power to slot
If there's no Attention Button, e.g., for an ExpressCard slot or Keith's SFF devices, I think it looks like this:
- Slot is initially powered off
- Slot hardware does not include an Attention Button
- pcie_enable_notification() enables Presence Detect Changed
interrupt but not Attention Button interrupt
- User inserts card
- Port detects card presence and asserts Presence Detect Changed interrupt
- ** Here is Keith's change to check Hot-Plug Capable instead of
Hot-Plug Surprise **
- OS turns on power to slot
I *think* it's already superfluous to check Hot-Plug Capable, because get_port_device_capability() in portdrv_core.c already checks for that, and if it's not set, I don't think pciehp will claim the Port. If that's true, then I should drop Keith's patch, and we should just remove the HP_SUPR_RM test altogether.
Bjorn
Bjorn Helgaas
maintainerQuoted 9 lines · Bjorn Helgaas
[+cc Rajat]
Quoted 6 lines · Keith Busch
The PCI-e Base specifically says Hot-Plug Surprise capability is for removal only, but pciehp checked this to determine if it should handle a slot event on device add. Lots of platform's pcie slots don't advertise support for surprise removal, but are perfectly capable of handling hot-add, so checking surprise removal is not appropriate.
Quoted 7 lines
Quoted 5 lines
This patch checks the Hot-Plug Capable bit in the slot capabilities to determine if the detected presence on is a reliable event instead of Hot-Plug Surprise.
Signed-off-by Keith Busch <[email protected]>This seems reasonable to me, and I applied it to pci/hotplug for v3.20.
After our follow-up discussion, I replaced this patch with the following. If anybody objects, let me know.
commit dc0c25c0ee9f92ba1872fdda9c2a0014611cf722
Author: Bjorn Helgaas <[email protected]> Date: Wed Jan 21 10:28:07 2015 -0600
PCI: pciehp: Handle surprise add even if surprise removal isn't supported
The PCIe spec (r3.0, sec 7.8.9) says Hot-Plug Surprise indicates support for surprise *removal*, but pciehp checked this to determine if it should handle presence detect interrupts for device *addition*.
Allow surprise device addition even if the slot doesn't advertise support for surprise removal.
Keith has a platform with slots for front-loading SFF devices. The slots do not have attention buttons and do not support surprise removal, but they do have presence detect. In that case, we still want to use presence detect for device addition.
Keith's original patch handled surprise insertions only if Hot-Plug Capable is set. I think that test is superfluous because pciehp only claims slots that advertise Hot-Plug Capable (see get_port_device_capability()).
drivers/pci/hotplug/pciehp_ctrl.c +0 -2
@@ -532,8 +532,6 @@ static void interrupt_event_handler(struct work_struct *work)pciehp_green_led_off (p_slot);break ;case INT_PRESENCE_ON : -if (!HP_SUPR_RM (ctrl)) -break ;ctrl_dbg (ctrl,"Surprise Insertion\n" );handle_surprise_event (p_slot);break ;