On Thu, Jul 12, 2012 at 1:20 PM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote: > On Wed, Jul 11, 2012 at 6:05 PM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote: >> 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: > > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
