| Message ID | 1340437325-29282-3-git-send-email-yinghai@kernel.org |
|---|---|
| State | Rejected |
| Headers | show |
> -----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
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
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
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
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
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
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))
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(-)