On Wed, May 02, 2018 at 08:41:48AM -0500, Bjorn Helgaas wrote:
> On Wed, May 02, 2018 at 02:55:09PM +0300, Mika Westerberg wrote:
> > On Tue, May 01, 2018 at 04:52:11PM -0500, Bjorn Helgaas wrote:
> > > Hi Mika,
> > >
> > > On Mon, Apr 16, 2018 at 01:34:47PM +0300, Mika Westerberg wrote:
> > > > After suspend/resume cycle the Presence Detect or Data Link Layer Status
> > > > Changed bits might be set and if we don't clear them those events will
> > > > not fire anymore and nothing happens for instance when a device is now
> > > > hot-unplugged.
> > > >
> > > > Fix this by clearing those bits in a newly introduced function
> > > > pcie_reenable_notification(). This should be fine because immediately
> > > > after we check if the adapter is still present by reading directly from
> > > > the status register.
> > > >
> > > > Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > > > Cc: stable@xxxxxxxxxxxxxxx
> > >
> > > I want to understand why we need to handle this differently between
> > > the boot-time probe path and the resume path.
> > >
> > > This patch clears PCI_EXP_SLTSTA_PDC and PCI_EXP_SLTSTA_DLLSC in the
> > > resume path:
> > >
> > > pciehp_resume
> > > pcie_reenable_notification
> > > # clear PDC DLLSC
> > > pcie_enable_notification
> > > # set DLLSCE ABPE/PDCE HPIE CCIE
> > > pciehp_get_adapter_status
> > > # read PCI_EXP_SLTSTA
> > >
> > > We used to clear PCI_EXP_SLTSTA_PDC and PCI_EXP_SLTSTA_DLLSC in the
> > > probe path:
> > >
> > > pciehp_probe
> > > pcie_init
> > > # clear PDC ABP PFD MRLSC CC DLSCC
> > > pcie_init_notification
> > > pciehp_request_irq
> > > pcie_enable_notification
> > > # set DLLSCE ABPE/PDCE HPIE CCIE
> > > pciehp_get_adapter_status
> > > # read PCI_EXP_SLTSTA
> > > pciehp_get_power_status
> > >
> > > db63d40017a5 ("PCI: pciehp: Do not clear Presence Detect Changed
> > > during initialization") changed the probe path so we don't clear
> > > PCI_EXP_SLTSTA_PDC there anymore. This was so we wouldn't miss a PDC
> > > interrupt that happened before probe.
> > >
> > > Why can't we handle probe and resume the same way? They look quite
> > > similar.
> > >
> > > You say this patch should be fine because we read SLTSTA immediately
> > > after clearing PDC and DLLSC. But we also read SLTSTA in the probe
> > > path, so I'm not sure why we need to clear PDC and DLLSC for resume
> > > but not for probe.
> >
> > On probe path we read status but here is what it does:
> >
> > pcie_init_notification()
> > ...
> > pciehp_get_adapter_status(slot, &occupied);
> > ...
> > if (occupied && pciehp_force) {
> > mutex_lock(&slot->hotplug_lock);
> > pciehp_enable_slot(slot);
> > mutex_unlock(&slot->hotplug_lock);
> > }
> >
> > If you don't have "pciehp.pciehp_force=1" in your kernel command line
> > you miss the fact that the card is already there. Obviously you can't
> > expect ordinary users to pass random command line options to get their
> > already connected device detected by Linux.
>
> Yeah, definitely not, that's really ugly.
>
> > So the reason why in probe we don't clear PDC is that once the interrupt
> > is unmasked, you get an interrupt and the card gets detected properly.
> >
> > On resume path we already check whether the card is there or not and
> > handle it accordingly. However, if we don't clear PDC and DLLSC bits we
> > will never get hotplug interrupt again.
> >
> > Now, you ask why can't we handle probe and resume the same way? I think
> > we can if we could get rid of that pciehp_force thing but it seems to
> > fix an issue on some HP hardware. See 9e5858244926 ("pciehp: don't
> > enable slot unless forced").
>
> Ugh. That's really ancient history. I would *love* to get rid of
> pciehp.pciehp_force. There's not much detail in 9e5858244926, but
> maybe with some digging we can figure out something.
>
> I'd rather do the digging now and try to simplify this area instead of
> adding another tweak.
I did some digging but unfortunately it is still not clear what issue
9e5858244926 is fixing. There is no bugzilla link and I was not able to
find any discussion around this either.
However, I think currently pciehp_force=1 does not actually do what it
was intended to do. Reason is that portdrv core already asks BIOS
whether native PCIe is allowed and if not, it does not set
PCIE_PORT_SERVICE_HP and that prevents the whole device to be created.
So even if you specify pciehp_force=1 in the command line, it does not
bypass the BIOS setting.
Furthermore we have had similar check in pciehp_resume() but it was
removed with 87683e22c646 ("PCI: pciehp: Always implement resume,
regardless of pciehp_force param") because it broke resume.
If I understand correctly you want me to change the driver so that I
remove pciehp_force check from probe. Then I can revert db63d40017a5
("PCI: pciehp: Do not clear Presence Detect Changed during
initialization") and that makes both probe path and resume path similar
(when this patch is included). Is that correct? I can do that in the
next version of the patch series.
