On Wed, Jul 11, 2012 at 3:24 PM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
