[v5,3/9] PCI: pciehp: Clear Presence Detect and Data Link Layer Status Changed on resume

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
Related show

Commit Message

Mika Westerberg April 16, 2018, 10:34 a.m.
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
---
 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(-)

Comments

Bjorn Helgaas May 1, 2018, 9:52 p.m. | #1
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
Mika Westerberg May 2, 2018, 11:55 a.m. | #2
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").
Bjorn Helgaas May 2, 2018, 1:41 p.m. | #3
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
Mika Westerberg May 3, 2018, 10:42 a.m. | #4
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.
Bjorn Helgaas May 3, 2018, 11:01 p.m. | #5
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
Lukas Wunner May 4, 2018, 7:18 a.m. | #6
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
Mika Westerberg May 4, 2018, 7:20 a.m. | #7
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.
Mika Westerberg May 4, 2018, 8:02 a.m. | #8
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.
Lukas Wunner May 30, 2018, 10:40 a.m. | #9
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,
Mika Westerberg May 30, 2018, 1:27 p.m. | #10
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 :-)

Patch

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;