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

login
register
mail settings
Submitter Yinghai Lu
Date July 11, 2012, 8:56 p.m.
Message ID <CAE9FiQXTjEqLLGx=EzxoWNqKTPF+g4=PNMiiTLkdUyf0LX-JUg@mail.gmail.com>
Download mbox | patch
Permalink /patch/170512/
State Not Applicable
Headers show

Comments

Yinghai Lu - July 11, 2012, 8:56 p.m.
On Wed, Jul 11, 2012 at 1:48 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>> 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?

so you want this ?

                handle_surprise_event(p_slot);

that should keep current user expected behavior that kernel only bring
the card online after press the 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
Bjorn Helgaas - July 11, 2012, 10:24 p.m.
On Wed, Jul 11, 2012 at 2:56 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Jul 11, 2012 at 1:48 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>>
>>> 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?
>
> so you want this ?
>
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c
> b/drivers/pci/hotplug/pciehp_ctrl.c
> index 27f4429..f103a4ca 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -463,7 +463,7 @@ static void interrupt_event_handler(struct
> work_struct *work)
>                 break;
>         case INT_PRESENCE_ON:
>         case INT_PRESENCE_OFF:
> -               if (!HP_SUPR_RM(ctrl))
> +               if (ATTN_BUTTN(ctrl))
>                         break;
>                 ctrl_dbg(ctrl, "Surprise Removal\n");
>                 handle_surprise_event(p_slot);
>
> that should keep current user expected behavior that kernel only bring
> the card online after press the button.

Well, I was actually only interested in reviewing your original patch,
and this seemed like a possible bug.  If it's not a bug, I don't want
to change anything.

If we *do* want to change something there, I don't like the proposal
above any better.  It's still basically saying "presence detect is
only reliable when X is set" when X is not clearly related to presence
detect.

I think it's better to disable the presence detect interrupt
completely if it's not reliable, as your original patch did.  My
complaint with that is that HP_SUPR_RM() doesn't seem like the right
test for "the interrupt is not reliable."

Having a "Presence Detect State" bit and an interrupt that tells you
when it changed is only meaningful if that bit gives you useful
information.  If hardware supplies that bit but it toggles all the
time when the slot is empty because it's hooked up to link training
attempts, that just means the hardware screwed up.  The hardware
*should* have included some logic to filter out the attempts and
toggle the bit only when a card is actually added or removed.  I
believe the functionality of "Presence Detect State" is logically
independent of "Hot-Plug Surprise" and "Attention Button Present."

So if we want to disable the "Presence Detect Changed" interrupt,
that's fine, but I think we should do it based on a quirk or
blacklist, or based on the fact that we have no need for it.  One
reason to want the interrupt is if "Hot-Plug Surprise" is set,
indicating that an adapter might be removed without notice, and if
that's the only reason, we could use your original patch.  But if we
do, I think we should change interrupt_event_handler() to look
something like this:

    case INT_PRESENCE_ON:
        if (!ATTN_BUTTN(ctrl))
            handle_surprise_event(p_slot);  /* omit this if you don't
think it's useful */
        break;
    case INT_PRESENCE_OFF:
        handle_surprise_event(p_slot);
        break;

If you did make a change like this, I propose (as a separate patch)
passing info->event_type into handle_surprise_event().  We've already
read the "Presence Detect State" bit, so there's no need for
handle_surprise_event() to do it again.
--
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 12, 2012, 12:05 a.m.
On Wed, Jul 11, 2012 at 3:24 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:

> If we *do* want to change something there, I don't like the proposal
> above any better.  It's still basically saying "presence detect is
> only reliable when X is set" when X is not clearly related to presence
> detect.
>
> I think it's better to disable the presence detect interrupt
> completely if it's not reliable, as your original patch did.  My
> complaint with that is that HP_SUPR_RM() doesn't seem like the right
> test for "the interrupt is not reliable."

ok.

>
> Having a "Presence Detect State" bit and an interrupt that tells you
> when it changed is only meaningful if that bit gives you useful
> information.  If hardware supplies that bit but it toggles all the
> time when the slot is empty because it's hooked up to link training
> attempts, that just means the hardware screwed up.  The hardware
> *should* have included some logic to filter out the attempts and
> toggle the bit only when a card is actually added or removed.  I
> believe the functionality of "Presence Detect State" is logically
> independent of "Hot-Plug Surprise" and "Attention Button Present."

the cpu vendor already agreed that is out of spec for that.

>
> So if we want to disable the "Presence Detect Changed" interrupt,
> that's fine, but I think we should do it based on a quirk or
> blacklist, or based on the fact that we have no need for it.  One
> reason to want the interrupt is if "Hot-Plug Surprise" is set,
> indicating that an adapter might be removed without notice, and if
> that's the only reason, we could use your original patch.

no,  with that patch, we will not get interrupt for present bit change
for non-hotplug-surprise
case.

> But if we
> do, I think we should change interrupt_event_handler() to look
> something like this:
>
>     case INT_PRESENCE_ON:
>         if (!ATTN_BUTTN(ctrl))
>             handle_surprise_event(p_slot);  /* omit this if you don't
> think it's useful */
>         break;
>     case INT_PRESENCE_OFF:
>         handle_surprise_event(p_slot);
>         break;

yes, this one should be good. and it is enhancement.

>
> If you did make a change like this, I propose (as a separate patch)
> passing info->event_type into handle_surprise_event().  We've already
> read the "Presence Detect State" bit, so there's no need for
> handle_surprise_event() to do it again.

ok. will prepare patches for that.

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 12, 2012, 8:20 p.m.
On Wed, Jul 11, 2012 at 6:05 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Jul 11, 2012 at 3:24 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
>> If we *do* want to change something there, I don't like the proposal
>> above any better.  It's still basically saying "presence detect is
>> only reliable when X is set" when X is not clearly related to presence
>> detect.
>>
>> I think it's better to disable the presence detect interrupt
>> completely if it's not reliable, as your original patch did.  My
>> complaint with that is that HP_SUPR_RM() doesn't seem like the right
>> test for "the interrupt is not reliable."
>
> ok.
>
>>
>> Having a "Presence Detect State" bit and an interrupt that tells you
>> when it changed is only meaningful if that bit gives you useful
>> information.  If hardware supplies that bit but it toggles all the
>> time when the slot is empty because it's hooked up to link training
>> attempts, that just means the hardware screwed up.  The hardware
>> *should* have included some logic to filter out the attempts and
>> toggle the bit only when a card is actually added or removed.  I
>> believe the functionality of "Presence Detect State" is logically
>> independent of "Hot-Plug Surprise" and "Attention Button Present."
>
> the cpu vendor already agreed that is out of spec for that.
>
>>
>> So if we want to disable the "Presence Detect Changed" interrupt,
>> that's fine, but I think we should do it based on a quirk or
>> blacklist, or based on the fact that we have no need for it.  One
>> reason to want the interrupt is if "Hot-Plug Surprise" is set,
>> indicating that an adapter might be removed without notice, and if
>> that's the only reason, we could use your original patch.
>
> no,  with that patch, we will not get interrupt for present bit change
> for non-hotplug-surprise
> case.
>
>> But if we
>> do, I think we should change interrupt_event_handler() to look
>> something like this:
>>
>>     case INT_PRESENCE_ON:

        ctrl_info(ctrl, "adapter now present\n");

>>         if (!ATTN_BUTTN(ctrl))
>>             handle_surprise_event(p_slot);  /* omit this if you don't
>> think it's useful */
>>         break;
>>     case INT_PRESENCE_OFF:

        ctrl_warn(ctrl, "adapter removed unexpectedly\n");

>>         handle_surprise_event(p_slot);
>>         break;
>
> yes, this one should be good. and it is enhancement.

When we don't have an attention button, I don't know that it's a good
idea to automatically power up a new card.  I was thinking about
things like ExpressCard, where the standard usage model probably *is*
"plug in the card and it automatically starts working, no button press
or software UI required."  But my guess is that ExpressCard would not
be handled by pciehp, would it?

The pciehp flow seems to be basically the same as SHPC, and the
Standard Hot-Plug Controller spec, rev 1.0, sec 2.5, has a flow of
hot-add without an attention button.  It shows:

  1) User selects empty, disabled slot and opens MRL
  2) Adapter insertion:
      2a) User inserts add-in-card
      2b) User closes MRL
      2c) User attaches cables
  3) User requests via software UI that slot be enabled

We'll get the Presence Detect interrupt at step 2a, before the user
can close the MRL or attach cables.  I don't know if it's a good thing
to power-on the slot and attach the driver while this is happening.
It certainly doesn't seem to follow the intent of the spec.

>> If you did make a change like this, I propose (as a separate patch)
>> passing info->event_type into handle_surprise_event().  We've already
>> read the "Presence Detect State" bit, so there's no need for
>> handle_surprise_event() to do it again.
>
> ok. will prepare patches for that.
>
> 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
Yinghai Lu - July 13, 2012, 12:19 a.m.
On Thu, Jul 12, 2012 at 1:20 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, Jul 11, 2012 at 6:05 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Wed, Jul 11, 2012 at 3:24 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>>> If we *do* want to change something there, I don't like the proposal
>>> above any better.  It's still basically saying "presence detect is
>>> only reliable when X is set" when X is not clearly related to presence
>>> detect.
>>>
>>> I think it's better to disable the presence detect interrupt
>>> completely if it's not reliable, as your original patch did.  My
>>> complaint with that is that HP_SUPR_RM() doesn't seem like the right
>>> test for "the interrupt is not reliable."
>>
>> ok.
>>
>>>
>>> Having a "Presence Detect State" bit and an interrupt that tells you
>>> when it changed is only meaningful if that bit gives you useful
>>> information.  If hardware supplies that bit but it toggles all the
>>> time when the slot is empty because it's hooked up to link training
>>> attempts, that just means the hardware screwed up.  The hardware
>>> *should* have included some logic to filter out the attempts and
>>> toggle the bit only when a card is actually added or removed.  I
>>> believe the functionality of "Presence Detect State" is logically
>>> independent of "Hot-Plug Surprise" and "Attention Button Present."
>>
>> the cpu vendor already agreed that is out of spec for that.
>>
>>>
>>> So if we want to disable the "Presence Detect Changed" interrupt,
>>> that's fine, but I think we should do it based on a quirk or
>>> blacklist, or based on the fact that we have no need for it.  One
>>> reason to want the interrupt is if "Hot-Plug Surprise" is set,
>>> indicating that an adapter might be removed without notice, and if
>>> that's the only reason, we could use your original patch.
>>
>> no,  with that patch, we will not get interrupt for present bit change
>> for non-hotplug-surprise
>> case.
>>
>>> But if we
>>> do, I think we should change interrupt_event_handler() to look
>>> something like this:
>>>
>>>     case INT_PRESENCE_ON:
>
>         ctrl_info(ctrl, "adapter now present\n");
>
>>>         if (!ATTN_BUTTN(ctrl))
>>>             handle_surprise_event(p_slot);  /* omit this if you don't
>>> think it's useful */
>>>         break;
>>>     case INT_PRESENCE_OFF:
>
>         ctrl_warn(ctrl, "adapter removed unexpectedly\n");

or keep the old

ctrl_dbg(ctrl, "Surprise Removal\n");

?

>
>>>         handle_surprise_event(p_slot);
>>>         break;
>>
>> yes, this one should be good. and it is enhancement.
>
> When we don't have an attention button, I don't know that it's a good
> idea to automatically power up a new card.  I was thinking about
> things like ExpressCard, where the standard usage model probably *is*
> "plug in the card and it automatically starts working, no button press
> or software UI required."  But my guess is that ExpressCard would not
> be handled by pciehp, would it?

some are by pciehp.

but my T420 BIOS _OSC doesn't give away pcie cap access, so pciehp can
not be loaded.
Try to file BIOS bug to the vendor, find no where to do that.

>
> The pciehp flow seems to be basically the same as SHPC, and the
> Standard Hot-Plug Controller spec, rev 1.0, sec 2.5, has a flow of
> hot-add without an attention button.  It shows:
>
>   1) User selects empty, disabled slot and opens MRL
>   2) Adapter insertion:
>       2a) User inserts add-in-card
>       2b) User closes MRL
>       2c) User attaches cables
>   3) User requests via software UI that slot be enabled
>
> We'll get the Presence Detect interrupt at step 2a, before the user
> can close the MRL or attach cables.  I don't know if it's a good thing
> to power-on the slot and attach the driver while this is happening.
> It certainly doesn't seem to follow the intent of the spec.
>

I think we should just keep old behavior like user need to press the
attention button.
and other os do that same way.

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 13, 2012, 3:30 p.m.
On Thu, Jul 12, 2012 at 6:19 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Jul 12, 2012 at 1:20 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Wed, Jul 11, 2012 at 6:05 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> On Wed, Jul 11, 2012 at 3:24 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>>
>>>> If we *do* want to change something there, I don't like the proposal
>>>> above any better.  It's still basically saying "presence detect is
>>>> only reliable when X is set" when X is not clearly related to presence
>>>> detect.
>>>>
>>>> I think it's better to disable the presence detect interrupt
>>>> completely if it's not reliable, as your original patch did.  My
>>>> complaint with that is that HP_SUPR_RM() doesn't seem like the right
>>>> test for "the interrupt is not reliable."
>>>
>>> ok.
>>>
>>>>
>>>> Having a "Presence Detect State" bit and an interrupt that tells you
>>>> when it changed is only meaningful if that bit gives you useful
>>>> information.  If hardware supplies that bit but it toggles all the
>>>> time when the slot is empty because it's hooked up to link training
>>>> attempts, that just means the hardware screwed up.  The hardware
>>>> *should* have included some logic to filter out the attempts and
>>>> toggle the bit only when a card is actually added or removed.  I
>>>> believe the functionality of "Presence Detect State" is logically
>>>> independent of "Hot-Plug Surprise" and "Attention Button Present."
>>>
>>> the cpu vendor already agreed that is out of spec for that.
>>>
>>>>
>>>> So if we want to disable the "Presence Detect Changed" interrupt,
>>>> that's fine, but I think we should do it based on a quirk or
>>>> blacklist, or based on the fact that we have no need for it.  One
>>>> reason to want the interrupt is if "Hot-Plug Surprise" is set,
>>>> indicating that an adapter might be removed without notice, and if
>>>> that's the only reason, we could use your original patch.
>>>
>>> no,  with that patch, we will not get interrupt for present bit change
>>> for non-hotplug-surprise
>>> case.
>>>
>>>> But if we
>>>> do, I think we should change interrupt_event_handler() to look
>>>> something like this:
>>>>
>>>>     case INT_PRESENCE_ON:
>>
>>         ctrl_info(ctrl, "adapter now present\n");
>>
>>>>         if (!ATTN_BUTTN(ctrl))
>>>>             handle_surprise_event(p_slot);  /* omit this if you don't
>>>> think it's useful */
>>>>         break;
>>>>     case INT_PRESENCE_OFF:
>>
>>         ctrl_warn(ctrl, "adapter removed unexpectedly\n");
>
> or keep the old
>
> ctrl_dbg(ctrl, "Surprise Removal\n");

ctrl_dbg() only produces output if pciehp_debug is enabled, and I
thought the message was useful enough for debugging issues that it
should always appear.  And I thought it was useful to include the word
"adapter" to make the message more meaningful to an end-user.

>>>>         handle_surprise_event(p_slot);
>>>>         break;
>>>
>>> yes, this one should be good. and it is enhancement.
>>
>> When we don't have an attention button, I don't know that it's a good
>> idea to automatically power up a new card.  I was thinking about
>> things like ExpressCard, where the standard usage model probably *is*
>> "plug in the card and it automatically starts working, no button press
>> or software UI required."  But my guess is that ExpressCard would not
>> be handled by pciehp, would it?
>
> some are by pciehp.
>
> but my T420 BIOS _OSC doesn't give away pcie cap access, so pciehp can
> not be loaded.
> Try to file BIOS bug to the vendor, find no where to do that.
>
>>
>> The pciehp flow seems to be basically the same as SHPC, and the
>> Standard Hot-Plug Controller spec, rev 1.0, sec 2.5, has a flow of
>> hot-add without an attention button.  It shows:
>>
>>   1) User selects empty, disabled slot and opens MRL
>>   2) Adapter insertion:
>>       2a) User inserts add-in-card
>>       2b) User closes MRL
>>       2c) User attaches cables
>>   3) User requests via software UI that slot be enabled
>>
>> We'll get the Presence Detect interrupt at step 2a, before the user
>> can close the MRL or attach cables.  I don't know if it's a good thing
>> to power-on the slot and attach the driver while this is happening.
>> It certainly doesn't seem to follow the intent of the spec.
>>
>
> I think we should just keep old behavior like user need to press the
> attention button.
> and other os do that same way.

If there is an attention button, I think we should definitely wait
until the user presses it.

The question is what to do when a new card appears and there's no
attention button.  In general I think we should follow the SHPC flow
and wait for some software equivalent of a button push, e.g., a sysfs
poke.

But if pciehp handles ExpressCards, that doesn't sound like the
desired user experience -- if we insert an ExpressCard in a laptop,
don't we expect it to just start working, without having to poke
around in software?
--
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 13, 2012, 6:07 p.m.
On Fri, Jul 13, 2012 at 8:30 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Thu, Jul 12, 2012 at 6:19 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>
> The question is what to do when a new card appears and there's no
> attention button.  In general I think we should follow the SHPC flow
> and wait for some software equivalent of a button push, e.g., a sysfs
> poke.
>
> But if pciehp handles ExpressCards, that doesn't sound like the
> desired user experience -- if we insert an ExpressCard in a laptop,
> don't we expect it to just start working, without having to poke
> around in software?

ExpressCards does support surprise removal. so in current code, it
still get online
automatically.

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

Patch

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c
b/drivers/pci/hotplug/pciehp_ctrl.c
index 27f4429..f103a4ca 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -463,7 +463,7 @@  static void interrupt_event_handler(struct
work_struct *work)
                break;
        case INT_PRESENCE_ON:
        case INT_PRESENCE_OFF:
-               if (!HP_SUPR_RM(ctrl))
+               if (ATTN_BUTTN(ctrl))
                        break;
                ctrl_dbg(ctrl, "Surprise Removal\n");