[PATCH] pci: Use hot-plug capable for testing presence on

[PATCH] pci: Use hot-plug capable for testing presence on

[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 <keith.busch@intel.com>
---
 drivers/pci/hotplug/pciehp.h      |    1 +
 drivers/pci/hotplug/pciehp_ctrl.c |    2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index b115219..b91eefb 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -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)
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index ff32e85..996c313 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -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

Re: [PATCH] pci: Use hot-plug capable for testing presence on

[Keith Busch]

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.

On Mon, 22 Dec 2014, Keith Busch wrote:
> 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 <keith.busch@intel.com>
> ---
> drivers/pci/hotplug/pciehp.h      |    1 +
> drivers/pci/hotplug/pciehp_ctrl.c |    2 +-
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index b115219..b91eefb 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -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)
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index ff32e85..996c313 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -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

Re: [PATCH] pci: Use hot-plug capable for testing presence on

[Bjorn Helgaas]

[+cc Rajat]

On Mon, Dec 22, 2014 at 12:07:03PM -0700, Keith Busch wrote:
> 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 <keith.busch@intel.com>

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.

> ---
>  drivers/pci/hotplug/pciehp.h      |    1 +
>  drivers/pci/hotplug/pciehp_ctrl.c |    2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index b115219..b91eefb 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -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)
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index ff32e85..996c313 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pci: Use hot-plug capable for testing presence on

[Rajat Jain]

Hello,

>>
>> Signed-off-by: Keith Busch <keith.busch@intel.com>
>
> This seems reasonable to me, and I applied it to pci/hotplug for v3.20.

Yes, I agree.

>
> 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>>
>> ---
>>  drivers/pci/hotplug/pciehp.h      |    1 +
>>  drivers/pci/hotplug/pciehp_ctrl.c |    2 +-
>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
>> index b115219..b91eefb 100644
>> --- a/drivers/pci/hotplug/pciehp.h
>> +++ b/drivers/pci/hotplug/pciehp.h
>> @@ -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)
>> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
>> index ff32e85..996c313 100644
>> --- a/drivers/pci/hotplug/pciehp_ctrl.c
>> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
>> @@ -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 majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pci: Use hot-plug capable for testing presence on

[Keith Busch]

On Fri, 16 Jan 2015, Rajat Jain wrote:
>> 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?

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.

Re: [PATCH] pci: Use hot-plug capable for testing presence on

[Bjorn Helgaas]

On Fri, Jan 16, 2015 at 2:50 PM, Rajat Jain <rajatxjain@gmail.com> wrote:
> Hello,
>
>>>
>>> Signed-off-by: Keith Busch <keith.busch@intel.com>
>>
>> This seems reasonable to me, and I applied it to pci/hotplug for v3.20.
>
> Yes, I agree.
>
>>
>> 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?

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

Re: [PATCH] pci: Use hot-plug capable for testing presence on

[Bjorn Helgaas]

On Fri, Jan 16, 2015 at 02:33:30PM -0600, Bjorn Helgaas wrote:
> [+cc Rajat]
> 
> On Mon, Dec 22, 2014 at 12:07:03PM -0700, Keith Busch wrote:
> > 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 <keith.busch@intel.com>
> 
> 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 <bhelgaas@google.com>
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()).
    
    Link: http://lkml.kernel.org/r/1419275223-14602-1-git-send-email-keith.busch@intel.com
    Based-on-patch-by: Keith Busch <keith.busch@intel.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index ff32e85e1de6..f052e951b23e 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -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;