diff mbox

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

Message ID 20120711162120.GA17988@google.com
State Not Applicable
Headers show

Commit Message

Bjorn Helgaas July 11, 2012, 4:21 p.m. UTC 
On Tue, Jul 10, 2012 at 04:56:15PM -0600, Bjorn Helgaas 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.
> 
> 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().

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.

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

Comments

Yinghai Lu July 11, 2012, 5:58 p.m. UTC | #1 
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
Bjorn Helgaas July 11, 2012, 6:15 p.m. UTC | #2 
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
Yinghai Lu July 11, 2012, 6:49 p.m. UTC | #3 
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
Bjorn Helgaas July 11, 2012, 7:56 p.m. UTC | #4 
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
Yinghai Lu July 11, 2012, 8:28 p.m. UTC | #5 
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
Bjorn Helgaas July 11, 2012, 8:48 p.m. UTC | #6 
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 mbox

Patch

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: