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

[Yinghai Lu <yinghai@kernel.org>]

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