| Message ID | 20120711162120.GA17988@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Wed, Jul 11, 2012 at 9:21 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: > 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. > > diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c > index 27f4429..4bbe257 100644 > --- a/drivers/pci/hotplug/pciehp_ctrl.c > +++ b/drivers/pci/hotplug/pciehp_ctrl.c > @@ -463,9 +463,8 @@ static void interrupt_event_handler(struct work_struct *work) > break; > 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. 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 Wed, Jul 11, 2012 at 11:58 AM, Yinghai Lu <yinghai@kernel.org> wrote: > On Wed, Jul 11, 2012 at 9:21 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> 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. >> >> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c >> index 27f4429..4bbe257 100644 >> --- a/drivers/pci/hotplug/pciehp_ctrl.c >> +++ b/drivers/pci/hotplug/pciehp_ctrl.c >> @@ -463,9 +463,8 @@ static void interrupt_event_handler(struct work_struct *work) >> break; >> 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. -- 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 Wed, Jul 11, 2012 at 11:15 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: > > 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. 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 Wed, Jul 11, 2012 at 12:49 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Wed, Jul 11, 2012 at 11:15 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> >> 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. > 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? -- 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 Wed, Jul 11, 2012 at 12:56 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Wed, Jul 11, 2012 at 12:49 PM, Yinghai Lu <yinghai@kernel.org> wrote: >> On Wed, Jul 11, 2012 at 11:15 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: >>> >>> 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. Spec does not mention about surprise add clearly. > >> 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. 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? > > 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 -- 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 Wed, Jul 11, 2012 at 2:28 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Wed, Jul 11, 2012 at 12:56 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> On Wed, Jul 11, 2012 at 12:49 PM, Yinghai Lu <yinghai@kernel.org> wrote: >>> On Wed, Jul 11, 2012 at 11:15 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: >>>> >>>> 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. > > Spec does not mention about surprise add clearly. No, in fact the Hot-Plug Surprise description mentions *removal* twice and doesn't mention *add* at all. >>> 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. > > 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? > When hotplug surprise is not there, why should we handle Presence Bit change? > >> >> 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 -- 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_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c index 27f4429..4bbe257 100644 --- a/drivers/pci/hotplug/pciehp_ctrl.c +++ b/drivers/pci/hotplug/pciehp_ctrl.c @@ -463,9 +463,8 @@ static void interrupt_event_handler(struct work_struct *work) break; 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: