Patchwork [2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

login
register
mail settings
Submitter Yinghai Lu
Date June 23, 2012, 7:42 a.m.
Message ID <1340437325-29282-3-git-send-email-yinghai@kernel.org>
Download mbox | patch
Permalink /patch/166732/
State Rejected
Headers show

Comments

Yinghai Lu - June 23, 2012, 7:42 a.m.
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 <yinghai@kernel.org>
---
 drivers/pci/hotplug/pciehp_hpc.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
Kenji Kaneshige - June 26, 2012, 1:26 a.m.
> -----Original Message-----
> From: linux-pci-owner@vger.kernel.org
> [mailto:linux-pci-owner@vger.kernel.org] On Behalf Of Yinghai Lu
> Sent: Saturday, June 23, 2012 4:42 PM
> To: Bjorn Helgaas
> Cc: linux-pci@vger.kernel.org; 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.

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 <kaneshige.kenji@jp.fujitsu.com>

> 
> 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.

Regards,
Kenji Kaneshige


> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/hotplug/pciehp_hpc.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c
> b/drivers/pci/hotplug/pciehp_hpc.c
> index a960fae..d4fa705 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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
Yinghai Lu - June 26, 2012, 6:03 p.m.
On Mon, Jun 25, 2012 at 6:26 PM, Kaneshige, Kenji
<kaneshige.kenji@jp.fujitsu.com> wrote:
>> -----Original Message-----
>> From: linux-pci-owner@vger.kernel.org
>> [mailto:linux-pci-owner@vger.kernel.org] On Behalf Of Yinghai Lu
>> Sent: Saturday, June 23, 2012 4:42 PM
>> To: Bjorn Helgaas
>> Cc: linux-pci@vger.kernel.org; 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.
>
> 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 <kaneshige.kenji@jp.fujitsu.com>

Thanks

>
>>
>> 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.

could be fixed from HW change and CPLD change.

Thanks

Yinghai
--
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
Bjorn Helgaas - July 10, 2012, 10:56 p.m.
On Sat, Jun 23, 2012 at 1:42 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> 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().

> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/hotplug/pciehp_hpc.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index a960fae..d4fa705 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu - July 10, 2012, 11:12 p.m.
On Tue, Jul 10, 2012 at 3:56 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Sat, Jun 23, 2012 at 1:42 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> 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.

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.

>
> 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.

Thanks

Yinghai
--
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
Bjorn Helgaas - July 10, 2012, 11:28 p.m.
On Tue, Jul 10, 2012 at 5:12 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Jul 10, 2012 at 3:56 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Sat, Jun 23, 2012 at 1:42 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> 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.
>
> 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.

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.

>> 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.

Dropped.
--
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
Yinghai Lu - July 10, 2012, 11:40 p.m.
On Tue, Jul 10, 2012 at 4:28 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Jul 10, 2012 at 5:12 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> 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.
>
> 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.

yes.
--
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

Patch

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index a960fae..d4fa705 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -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))