| Message ID | 20180416103453.46232-4-mika.westerberg@linux.intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Fixes and cleanups for native PCIe and ACPI hotplug | expand |
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@linux.intel.com> > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: stable@vger.kernel.org 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. > --- > drivers/pci/hotplug/pciehp.h | 2 +- > drivers/pci/hotplug/pciehp_core.c | 2 +- > drivers/pci/hotplug/pciehp_hpc.c | 13 ++++++++++++- > 3 files changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h > index 88e917c9120f..5f892065585e 100644 > --- a/drivers/pci/hotplug/pciehp.h > +++ b/drivers/pci/hotplug/pciehp.h > @@ -121,7 +121,7 @@ struct controller *pcie_init(struct pcie_device *dev); > int pcie_init_notification(struct controller *ctrl); > int pciehp_enable_slot(struct slot *p_slot); > int pciehp_disable_slot(struct slot *p_slot); > -void pcie_enable_notification(struct controller *ctrl); > +void pcie_reenable_notification(struct controller *ctrl); > int pciehp_power_on_slot(struct slot *slot); > void pciehp_power_off_slot(struct slot *slot); > void pciehp_get_power_status(struct slot *slot, u8 *status); > diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c > index 332b723ff9e6..44a6a63802d5 100644 > --- a/drivers/pci/hotplug/pciehp_core.c > +++ b/drivers/pci/hotplug/pciehp_core.c > @@ -283,7 +283,7 @@ static int pciehp_resume(struct pcie_device *dev) > ctrl = get_service_data(dev); > > /* reinitialize the chipset's event detection logic */ > - pcie_enable_notification(ctrl); > + pcie_reenable_notification(ctrl); > > slot = ctrl->slot; > > diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c > index 18a42f8f5dc5..98ea75aa32c7 100644 > --- a/drivers/pci/hotplug/pciehp_hpc.c > +++ b/drivers/pci/hotplug/pciehp_hpc.c > @@ -659,7 +659,7 @@ static irqreturn_t pcie_isr(int irq, void *dev_id) > return handled; > } > > -void pcie_enable_notification(struct controller *ctrl) > +static void pcie_enable_notification(struct controller *ctrl) > { > u16 cmd, mask; > > @@ -697,6 +697,17 @@ void pcie_enable_notification(struct controller *ctrl) > pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, cmd); > } > > +void pcie_reenable_notification(struct controller *ctrl) > +{ > + /* > + * Clear both Presence and Data Link Layer Changed to make sure > + * those events still fire after we have re-enabled them. > + */ > + pcie_capability_write_word(ctrl->pcie->port, PCI_EXP_SLTSTA, > + PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC); > + pcie_enable_notification(ctrl); > +} > + > static void pcie_disable_notification(struct controller *ctrl) > { > u16 mask; > -- > 2.16.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
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@linux.intel.com> > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Cc: stable@vger.kernel.org > > 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. 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").
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@linux.intel.com> > > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > Cc: stable@vger.kernel.org > > > > 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. Bjorn
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@linux.intel.com> > > > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > Cc: stable@vger.kernel.org > > > > > > 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.
On Thu, May 03, 2018 at 01:42:50PM +0300, Mika Westerberg wrote: > 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@linux.intel.com> > > > > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > > Cc: stable@vger.kernel.org > > > > > > > > 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. If you think we can remove pciehp_force, that would be awesome. This should be a separate patch all by itself, of course, and include your reasoning above. I would also love to revert db63d40017a5 ("PCI: pciehp: Do not clear Presence Detect Changed during initialization") because I'm not convinced that trying to handle interrupts that happened before binding the driver makes sense. It *would* make sense to me to enable interrupts, clear the "changed" status bits so future changes will cause interrupts, and check the "state" status bits and act on them. Bjorn
On Tue, May 01, 2018 at 04:52:11PM -0500, Bjorn Helgaas wrote: > 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. > > 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 The Slot Control register is already written during the ->resume_noirq phase via: pci_pm_resume_noirq pci_pm_default_resume_early pci_restore_state pci_restore_pcie_state From that point on, slot events are enabled, but they're not received because interrupts are disabled until the ->resume_early phase commences. My guess is that if the hotplug port uses edge-triggered MSI/MSI-X, an interrupt may have already been sent during ->resume_noirq, but was lost because interrupts were disabled. Clearing the Slot Status register thus acknowledges any lost interrupts. If this theory is correct, it should probably be included in the commit message and/or a code comment so that it's clear what's going on. Thanks, Lukas
On Thu, May 03, 2018 at 06:01:08PM -0500, Bjorn Helgaas wrote: > If you think we can remove pciehp_force, that would be awesome. This > should be a separate patch all by itself, of course, and include your > reasoning above. > > I would also love to revert db63d40017a5 ("PCI: pciehp: Do not clear > Presence Detect Changed during initialization") because I'm not > convinced that trying to handle interrupts that happened before > binding the driver makes sense. It *would* make sense to me to enable > interrupts, clear the "changed" status bits so future changes will > cause interrupts, and check the "state" status bits and act on them. OK, I'll make that a separate patch series completely. Regarding the current patch set, do you have any additional comments / converns? I would like to send out v6 next week.
On Fri, May 04, 2018 at 09:18:27AM +0200, Lukas Wunner wrote: > On Tue, May 01, 2018 at 04:52:11PM -0500, Bjorn Helgaas wrote: > > 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. > > > > 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 > > The Slot Control register is already written during the ->resume_noirq > phase via: > > pci_pm_resume_noirq > pci_pm_default_resume_early > pci_restore_state > pci_restore_pcie_state > > >From that point on, slot events are enabled, but they're not received > because interrupts are disabled until the ->resume_early phase commences. > > My guess is that if the hotplug port uses edge-triggered MSI/MSI-X, > an interrupt may have already been sent during ->resume_noirq, but > was lost because interrupts were disabled. Clearing the Slot Status > register thus acknowledges any lost interrupts. > > If this theory is correct, it should probably be included in the > commit message and/or a code comment so that it's clear what's > going on. I think I can check this by printing out the Slot status register in pci_restore_pcie_state(). If it turns out that the interrupt happens after we have restored Slot control register, I'll update the changelog accordingly.
On Thu, May 03, 2018 at 06:01:08PM -0500, Bjorn Helgaas wrote: > On Thu, May 03, 2018 at 01:42:50PM +0300, Mika Westerberg wrote: > > 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: > > > > > 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. > > > > > > > > > > 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. > > If you think we can remove pciehp_force, that would be awesome. This > should be a separate patch all by itself, of course, and include your > reasoning above. > > I would also love to revert db63d40017a5 ("PCI: pciehp: Do not clear > Presence Detect Changed during initialization") because I'm not > convinced that trying to handle interrupts that happened before > binding the driver makes sense. It *would* make sense to me to enable > interrupts, clear the "changed" status bits so future changes will > cause interrupts, and check the "state" status bits and act on them. JFYI, I've added the below patch to my upcoming series which reworks pciehp event handling and adds runtime PM support: https://github.com/l1k/linux/commits/pciehp_runpm_v2 The patch removes pciehp_force and reverts db63d40017a5 as requested. I needed this for the series to work flawlessly. Note, the patch depends on the preceding patches in the series. It cannot be applied as is to pci.git branches. The series has been in development for months but is now converging, I hope to post it sometime during or after the merge window. Comments/testing welcome. Thanks, Lukas -- >8 -- Subject: [PATCH] PCI: pciehp: Always enable occupied slot on probe Per PCIe r4.0, sec 6.7.3.4, a "port may optionally send an MSI when there are hot-plug events that occur while interrupt generation is disabled, and interrupt generation is subsequently enabled." On probe, we currently clear all event bits in the Slot Status register with the notable exception of the Presence Detect Changed bit. Thereby we seek to receive an interrupt for an already occupied slot once event notification is enabled. But because the interrupt is optional, users may have to specify the pciehp_force parameter on the command line, which is inconvenient. Moreover, now that pciehp's event handling has become resilient to missed events, a Presence Detect Changed interrupt for a slot which is powered on is interpreted as removal of the card. If the slot has already been brought up by the BIOS, receiving such an interrupt on probe causes the slot to be powered off and immediately back on, which is likewise undesirable. Avoid both issues by making the behavior of pciehp_force the default and clearing the Presence Detect Changed bit on probe. Note that the stated purpose of pciehp_force per the MODULE_PARM_DESC ("Force pciehp, even if OSHP is missing") seems nonsensical because the OSHP control method is only relevant for SHCP slots according to the PCI Firmware specification r3.0, sec 4.8. Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> Signed-off-by: Lukas Wunner <lukas@wunner.de> --- drivers/pci/hotplug/pciehp_core.c | 12 ++++-------- drivers/pci/hotplug/pciehp_hpc.c | 9 ++------- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c index 6c77b6d7445f..64903f6fc8ef 100644 --- a/drivers/pci/hotplug/pciehp_core.c +++ b/drivers/pci/hotplug/pciehp_core.c @@ -30,7 +30,6 @@ bool pciehp_debug; bool pciehp_poll_mode; int pciehp_poll_time; -static bool pciehp_force; /* * not really modular, but the easiest way to keep compat with existing @@ -39,11 +38,9 @@ static bool pciehp_force; module_param(pciehp_debug, bool, 0644); module_param(pciehp_poll_mode, bool, 0644); module_param(pciehp_poll_time, int, 0644); -module_param(pciehp_force, bool, 0644); MODULE_PARM_DESC(pciehp_debug, "Debugging mode enabled or not"); MODULE_PARM_DESC(pciehp_poll_mode, "Using polling mechanism for hot-plug events or not"); MODULE_PARM_DESC(pciehp_poll_time, "Polling mechanism frequency, in seconds"); -MODULE_PARM_DESC(pciehp_force, "Force pciehp, even if OSHP is missing"); #define PCIE_MODULE_NAME "pciehp" @@ -243,11 +240,10 @@ static int pciehp_probe(struct pcie_device *dev) mutex_lock(&slot->lock); pciehp_get_adapter_status(slot, &occupied); pciehp_get_power_status(slot, &poweron); - if (pciehp_force && - ((occupied && (slot->state == OFF_STATE || - slot->state == BLINKINGON_STATE)) || - (!occupied && (slot->state == ON_STATE || - slot->state == BLINKINGOFF_STATE)))) { + if ((occupied && (slot->state == OFF_STATE || + slot->state == BLINKINGON_STATE)) || + (!occupied && (slot->state == ON_STATE || + slot->state == BLINKINGOFF_STATE))) { atomic_or(PCI_EXP_SLTSTA_PDC, &ctrl->pending_events); irq_wake_thread(ctrl->pcie->irq, ctrl); } diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index 3d402bd998df..8cde5c14f4e6 100644 --- a/drivers/pci/hotplug/pciehp_hpc.c +++ b/drivers/pci/hotplug/pciehp_hpc.c @@ -835,16 +835,11 @@ struct controller *pcie_init(struct pcie_device *dev) if (link_cap & PCI_EXP_LNKCAP_DLLLARC) ctrl->link_active_reporting = 1; - /* - * Clear all remaining event bits in Slot Status register except - * Presence Detect Changed. We want to make sure possible - * hotplug event is triggered when the interrupt is unmasked so - * that we don't lose that event. - */ + /* Clear all remaining event bits in Slot Status register. */ pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD | PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_CC | - PCI_EXP_SLTSTA_DLLSC); + PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC); ctrl_info(ctrl, "Slot #%d AttnBtn%c PwrCtrl%c MRL%c AttnInd%c PwrInd%c HotPlug%c Surprise%c Interlock%c NoCompl%c LLActRep%c\n", (slot_cap & PCI_EXP_SLTCAP_PSN) >> 19,
On Wed, May 30, 2018 at 12:40:50PM +0200, Lukas Wunner wrote: > JFYI, I've added the below patch to my upcoming series which reworks > pciehp event handling and adds runtime PM support: > https://github.com/l1k/linux/commits/pciehp_runpm_v2 > > The patch removes pciehp_force and reverts db63d40017a5 as requested. Thanks for taking care if it! I can remove it from my todo list now :-)
diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h index 88e917c9120f..5f892065585e 100644 --- a/drivers/pci/hotplug/pciehp.h +++ b/drivers/pci/hotplug/pciehp.h @@ -121,7 +121,7 @@ struct controller *pcie_init(struct pcie_device *dev); int pcie_init_notification(struct controller *ctrl); int pciehp_enable_slot(struct slot *p_slot); int pciehp_disable_slot(struct slot *p_slot); -void pcie_enable_notification(struct controller *ctrl); +void pcie_reenable_notification(struct controller *ctrl); int pciehp_power_on_slot(struct slot *slot); void pciehp_power_off_slot(struct slot *slot); void pciehp_get_power_status(struct slot *slot, u8 *status); diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c index 332b723ff9e6..44a6a63802d5 100644 --- a/drivers/pci/hotplug/pciehp_core.c +++ b/drivers/pci/hotplug/pciehp_core.c @@ -283,7 +283,7 @@ static int pciehp_resume(struct pcie_device *dev) ctrl = get_service_data(dev); /* reinitialize the chipset's event detection logic */ - pcie_enable_notification(ctrl); + pcie_reenable_notification(ctrl); slot = ctrl->slot; diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index 18a42f8f5dc5..98ea75aa32c7 100644 --- a/drivers/pci/hotplug/pciehp_hpc.c +++ b/drivers/pci/hotplug/pciehp_hpc.c @@ -659,7 +659,7 @@ static irqreturn_t pcie_isr(int irq, void *dev_id) return handled; } -void pcie_enable_notification(struct controller *ctrl) +static void pcie_enable_notification(struct controller *ctrl) { u16 cmd, mask; @@ -697,6 +697,17 @@ void pcie_enable_notification(struct controller *ctrl) pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, cmd); } +void pcie_reenable_notification(struct controller *ctrl) +{ + /* + * Clear both Presence and Data Link Layer Changed to make sure + * those events still fire after we have re-enabled them. + */ + pcie_capability_write_word(ctrl->pcie->port, PCI_EXP_SLTSTA, + PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC); + pcie_enable_notification(ctrl); +} + static void pcie_disable_notification(struct controller *ctrl) { u16 mask;