On Thu, Jul 12, 2012 at 6:19 PM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote: > 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"); 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
