On Thu, Aug 03, 2017 at 03:10:34PM -0500, Bjorn Helgaas wrote: > [+cc Yinghai, Rajat] > > On Wed, Aug 02, 2017 at 11:59:24PM -0400, Keith Busch wrote: > > > > The part of the specification that suggests PDCE is tied to the hotplug > > capable bit is the "Slot Control Register" section in 7.8.10 of PCIe > > Base spec rev4. Specifically: > > > > "If the Hot-Plug Capable bit in the Slot Capabilities register is 0b, > > this bit is permitted to be read-only with a value of 0b" > > > > So this control doesn't do anything if hotplug capable is not set. > > I thought that might be the part of the spec you were referring to, > but that's not how I read it :) I read sec 7.8.10 as saying: > > - Presence Detect Changed Enable is a read/write bit, > - However, if Hot-Plug Capable (a HwInit/read-only bit) is 0, > PDCE may be a read-only 0 (OR it may be read/write) > > So I think it's slightly misleading to suggest that PDCE is "tied" to > HPC. I think the spec allows PDCE to be read/write and to have some > effect, even if HPC is 0. > > I don't know what it would *mean* to have a slot that said "I don't > support hot-plug operations, but my Presence Detect State might > change". We've seen some creative uses of pciehp, so I wouldn't be > surprised if somebody dreamed up a way to use it. Indeed, in my limited experience I also observe slightly different behavior that could all be interpreted as spec compliant. Coding the kernel for one unfortunately risks breaking another. :( > > The only reference I find on why the code is currently done this way is > > from this thread: > > > > https://marc.info/?l=linux-kernel&m=138688828930718&w=2 > > > > It seems the current behavior was done this way as a hunch more than > > anything emperical. > > Yeah, this is the sort of thing that niggles at me, but I don't know > how to resolve it other than to just apply your patch and see if > anything breaks. There is this hint that presence detect flaps > sometimes: > > http://www.spinics.net/lists/hotplug/msg05812.html > > But there's nothing there we can really act on, unless Yinghai or > Rajat can dredge up something more concrete. > > > The problem I'm trying to solve, though, is with a real platform > > that supports hot-add. The reason it is currently broken with Linux > > is because this platform advertises "Attention Button Present" when > > in fact no physical button exists on the platform. Since the kernel > > doesn't enable presence detect change events when attention button > > present is set, we don't get hot-add events. We've been working around > > this by using pciehp_poll_mode=1, but we'd prefer to see this working > > without kernel parameters. > > I agree, using pciehp_poll_mode stinks, and I like your patch. > > Can we mention the platform name here, just in case this oddity > (advertising Attention Button without having a button) is of interest > in the future? > > I propose the following changelog (and I'll add the platform name if > you can supply it): This one I'm helping with is based on a COM Express Type 6. The interesting part is probably the carrier board, which has a PEX8733 bridge. I think its safe to assume not all the available features are being used, which should be fine, but is why advertised capabilities are missing their human interfaces. > PCI: pciehp: Always enable Presence Detect notifications for hotplug slots > > Previously we only enabled notification of Presence Detect change events if > the slot did not advertise an Attention Button. > > But there are platforms that support hotplug and advertise "Attention > Button Present" when in fact there is no physical button. On these > platforms, we enabled Attention Button notifications, but never got any. > Presence Detect events occurred, but we left Presence Detect notification > disabled, so we never noticed them. > > Always enable Presence Detect notifications for hotplug slots, even if they > advertise "Attention Button Present", so we can detect hotplug events. > > Signed-off-by: Keith Busch <keith.busch@xxxxxxxxx> > [bhelgaas: changelog] > Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> This sounds great to me. Let me consider Rajat's reply before pulling the trigger on this one. I initially thought this patch was safe, but I don't want to break anything.
