[PATCH v2 1/1] PCI/hotplug: Don't enable HPIE in poll mode

[PATCH v2 1/1] PCI/hotplug: Don't enable HPIE in poll mode

[Ilpo Järvinen]

PCIe hotplug can operate in poll mode without interrupt handlers using
a polling kthread only. The commit eb34da60edee ("PCI: pciehp: Disable
hotplug interrupt during suspend") failed to consider that and enables
HPIE (Hot-Plug Interrupt Enable) unconditionally when resuming the
Port.

Only set HPIE if non-poll mode is in use. This makes
pcie_enable_interrupt() match how pcie_enable_notification() already
handles HPIE.

Fixes: eb34da60edee ("PCI: pciehp: Disable hotplug interrupt during suspend")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Lukas Wunner <lukas@wunner.de>
---

v2:
- Dropped other hotplug fixes/changes (Lukas' approach/fix is better)
- Fixed typo in shortlog

 drivers/pci/hotplug/pciehp_hpc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index bb5a8d9f03ad..28ab393af1c0 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -842,7 +842,9 @@ void pcie_enable_interrupt(struct controller *ctrl)
 {
 	u16 mask;
 
-	mask = PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_DLLSCE;
+	mask = PCI_EXP_SLTCTL_DLLSCE;
+	if (!pciehp_poll_mode)
+		mask |= PCI_EXP_SLTCTL_HPIE;
 	pcie_write_cmd(ctrl, mask, mask);
 }
 

base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
-- 
2.39.5

Re: [PATCH v2 1/1] PCI/hotplug: Don't enable HPIE in poll mode

[Bjorn Helgaas]

On Fri, Mar 21, 2025 at 06:21:14PM +0200, Ilpo Järvinen wrote:
> PCIe hotplug can operate in poll mode without interrupt handlers using
> a polling kthread only. The commit eb34da60edee ("PCI: pciehp: Disable
> hotplug interrupt during suspend") failed to consider that and enables
> HPIE (Hot-Plug Interrupt Enable) unconditionally when resuming the
> Port.
> 
> Only set HPIE if non-poll mode is in use. This makes
> pcie_enable_interrupt() match how pcie_enable_notification() already
> handles HPIE.
> 
> Fixes: eb34da60edee ("PCI: pciehp: Disable hotplug interrupt during suspend")
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Reviewed-by: Lukas Wunner <lukas@wunner.de>

Applied to pci/hotplug with subject:

  PCI: pciehp: Don't enable HPIE when resuming in poll mode

to match the pciehp history.

Random things that I noticed while looking at this:

  - It does make me wonder why we have both pcie_enable_interrupt()
    and pcie_enable_notification().  Apparently we don't need to
    restore the other PCI_EXP_SLTCTL bits when resuming?  Maybe we
    depend on some other restoration, e.g., pci_restore_pcie_state(),
    that happens first?

    That makes me worry that there's a window between
    pci_restore_pcie_state() and pcie_enable_interrupt().  I suppose
    we probably saved the pcie state after pcie_disable_interrupt(),
    so HPIE would be disabled in the saved state.

  - I also wonder about the fact that pci_restore_pcie_state() doesn't
    account for Command Completed events, so we might write to Slot
    Control too fast.

  - It's annoying that pcie_enable_interrupt() and
    pcie_disable_interrupt() are global symbols, a consequence of
    pciehp being split across five files instead of being one, which
    is also a nuisance for code browsing.

    Also annoying that they are generically named, with no pciehp
    connection (probably another consequence of being split into
    several files).

  - The eb34da60edee commit log hints at the reason for testing
    pme_is_native().  Would be nice if there were also a comment in
    the code about this because it's not 100% obvious why we test PME
    support in the PCIe native hotplug driver.

  - Also slightly weird that eb34da60edee added the pme_is_native()
    tests in pciehp_suspend() and pciehp_resume(), but somewhere along
    the line the suspend-side one got moved to
    pciehp_disable_interrupt(), so they're no longer parallel for no
    obvious reason.

  - I forgot why we have both pcie_write_cmd() and
    pcie_write_cmd_nowait() and how to decide which to use.

Whew, that was a lot.  I feel unusually ignorant this morning.

> ---
> 
> v2:
> - Dropped other hotplug fixes/changes (Lukas' approach/fix is better)
> - Fixed typo in shortlog
> 
>  drivers/pci/hotplug/pciehp_hpc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index bb5a8d9f03ad..28ab393af1c0 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -842,7 +842,9 @@ void pcie_enable_interrupt(struct controller *ctrl)
>  {
>  	u16 mask;
>  
> -	mask = PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_DLLSCE;
> +	mask = PCI_EXP_SLTCTL_DLLSCE;
> +	if (!pciehp_poll_mode)
> +		mask |= PCI_EXP_SLTCTL_HPIE;
>  	pcie_write_cmd(ctrl, mask, mask);
>  }
>  
> 
> base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
> -- 
> 2.39.5
> 

Re: [PATCH v2 1/1] PCI/hotplug: Don't enable HPIE in poll mode

[Lukas Wunner]

On Fri, Mar 21, 2025 at 12:09:19PM -0500, Bjorn Helgaas wrote:
>   - It does make me wonder why we have both pcie_enable_interrupt()
>     and pcie_enable_notification().  Apparently we don't need to
>     restore the other PCI_EXP_SLTCTL bits when resuming?  Maybe we
>     depend on some other restoration, e.g., pci_restore_pcie_state(),
>     that happens first?

Yes and yes.

> 
>     That makes me worry that there's a window between
>     pci_restore_pcie_state() and pcie_enable_interrupt().  I suppose
>     we probably saved the pcie state after pcie_disable_interrupt(),
>     so HPIE would be disabled in the saved state.

Yes.

> 
>   - I also wonder about the fact that pci_restore_pcie_state() doesn't
>     account for Command Completed events, so we might write to Slot
>     Control too fast.

We don't. :)  See commit 469e764c4a3c ("PCI: pciehp: Obey compulsory
command delay after resume").

So this is all a bit subtle and perhaps restoring the Slot Control
registers should not be done in pci_restore_pcie_state() but rather
in pciehp and pnv (which are the only hotplug drivers touching
PCI_EXP_SLTCTL).

In particular, if hotplug control was not granted by the platform,
I don't think the kernel is supposed to touch the Slot Control
registers at all.  So it probably shouldn't restore them either.

We've been doing this since 2006 with commit b56a5a23bfec ("PCI:
Restore PCI Express capability registers after PM event").
Negotiation of _OSC wasn't added until 2010 with 75fb60f26bef
("ACPI/PCI: Negotiate _OSC control bits before requesting them").
But the OSC_PCI_EXPRESS_NATIVE_HP_CONTROL predates the introduction
of git.  I guess noone ever realized that restoring Slot Control
shouldn't be done if hotplug control wasn't granted.  I can't
remember anyone ever reporting issues because of that though.

On the other hand, changing something like this always risks
regressions.

>   - It's annoying that pcie_enable_interrupt() and
>     pcie_disable_interrupt() are global symbols, a consequence of
>     pciehp being split across five files instead of being one, which
>     is also a nuisance for code browsing.

Roughly,
pciehp_core.c contains the interface to the PCI hotplug core
  (registering the hotplug_slot_ops etc),
pciehp_hpc.c contains the interaction with hardware registers,
pciehp_core.c contains the state machine,
pciehp_pci.c contains the interaction with the PCI core
  (enumeration / de-enumeration of devices on slot bringup / bringdown).

The only reason I've refrained from making major adjustments to this
structure in the past was that it would make "git blame" a little more
difficult and applying fixes to stable kernels would also become somewhat
more painful as it would require backporting.

>     Also annoying that they are generically named, with no pciehp
>     connection (probably another consequence of being split into
>     several files).

They can be renamed.  There's no good reason for using the "pcie_"
prefix, it just appears to be a historic artifact.

>   - The eb34da60edee commit log hints at the reason for testing
>     pme_is_native().  Would be nice if there were also a comment in
>     the code about this because it's not 100% obvious why we test PME
>     support in the PCIe native hotplug driver.
> 
>   - Also slightly weird that eb34da60edee added the pme_is_native()
>     tests in pciehp_suspend() and pciehp_resume(), but somewhere along
>     the line the suspend-side one got moved to
>     pciehp_disable_interrupt(), so they're no longer parallel for no
>     obvious reason.
> 
>   - I forgot why we have both pcie_write_cmd() and
>     pcie_write_cmd_nowait() and how to decide which to use.

pcie_write_cmd_nowait() is the "fire and forget" variant,
whereas pcie_write_cmd() can be thought of as the "_sync" variant,
i.e. the control flow doesn't continue until the command has been
processed by the slot.

E.g. pciehp_power_on_slot() waits for the slot command to complete
before making sure the Link Disable bit is clear.  It wouldn't make
much sense to do the latter when the former hasn't been completed yet.

Thanks,

Lukas

Re: [PATCH v2 1/1] PCI/hotplug: Don't enable HPIE in poll mode

[Bjorn Helgaas]

On Fri, Mar 21, 2025 at 07:07:47PM +0100, Lukas Wunner wrote:
> On Fri, Mar 21, 2025 at 12:09:19PM -0500, Bjorn Helgaas wrote:
> ...

> >   - It's annoying that pcie_enable_interrupt() and
> >     pcie_disable_interrupt() are global symbols, a consequence of
> >     pciehp being split across five files instead of being one, which
> >     is also a nuisance for code browsing.
> 
> Roughly,
> pciehp_core.c contains the interface to the PCI hotplug core
>   (registering the hotplug_slot_ops etc),
> pciehp_hpc.c contains the interaction with hardware registers,
> pciehp_core.c contains the state machine,
> pciehp_pci.c contains the interaction with the PCI core
>   (enumeration / de-enumeration of devices on slot bringup / bringdown).
> 
> The only reason I've refrained from making major adjustments to this
> structure in the past was that it would make "git blame" a little more
> difficult and applying fixes to stable kernels would also become somewhat
> more painful as it would require backporting.

Yeah, that's the main reason I haven't tried to do anything either.
On the other hand, the browsing nuisance is an everyday thing forever
if we leave it as-is.  I did consolidate portdrv.c a couple years ago
and don't regret it.  But moving things definitely makes "git blame" a
bit of a hassle; my notes are full of things like this:

  a1ccd3d91138 ("PCI/portdrv: Squash into portdrv.c")
    squash drivers/pci/pcie/portdrv_pci.c and portdrv_core.c into portdrv.c
  950bf6388bc2 ("PCI: Move DesignWare IP support to new drivers/pci/dwc/ directory")
    mv drivers/pci/host/pci-imx6.c drivers/pci/dwc/pci-imx6.c
  6e0832fa432e ("PCI: Collect all native drivers under drivers/pci/controller/")
    mv drivers/pci/dwc/pci-imx6.c drivers/pci/controller/dwc/pci-imx6.c

> >   - I forgot why we have both pcie_write_cmd() and
> >     pcie_write_cmd_nowait() and how to decide which to use.
> 
> pcie_write_cmd_nowait() is the "fire and forget" variant,
> whereas pcie_write_cmd() can be thought of as the "_sync" variant,
> i.e. the control flow doesn't continue until the command has been
> processed by the slot.
> 
> E.g. pciehp_power_on_slot() waits for the slot command to complete
> before making sure the Link Disable bit is clear.  It wouldn't make
> much sense to do the latter when the former hasn't been completed yet.

Right, I know what the difference is; I guess I just don't know how to
figure out when pcie_write_cmd_nowait() is safe.

Bjorn

Re: [PATCH v2 1/1] PCI/hotplug: Don't enable HPIE in poll mode

[Ilpo Järvinen]

On Fri, 21 Mar 2025, Bjorn Helgaas wrote:

> On Fri, Mar 21, 2025 at 07:07:47PM +0100, Lukas Wunner wrote:
> > On Fri, Mar 21, 2025 at 12:09:19PM -0500, Bjorn Helgaas wrote:
> > ...
> 
> > >   - It's annoying that pcie_enable_interrupt() and
> > >     pcie_disable_interrupt() are global symbols, a consequence of
> > >     pciehp being split across five files instead of being one, which
> > >     is also a nuisance for code browsing.
> > 
> > Roughly,
> > pciehp_core.c contains the interface to the PCI hotplug core
> >   (registering the hotplug_slot_ops etc),

This is at least oneway as it contains only static functions if 
init func is not counted.

But it has things like pciehp_check_presence() and set_attention_status()
that aren't about interfacing to the hp core.

> > pciehp_hpc.c contains the interaction with hardware registers,
> > pciehp_core.c contains the state machine,

pciehp_ctrl.c is full of PCI_EXP_* usage so it definitely is deeply 
intertwined with hardware registers and does HW related waits, etc.
Thus, the split between hpc and ctrl feels especially artificial and
that's where the back and forth calls also are.

> > pciehp_pci.c contains the interaction with the PCI core
> >   (enumeration / de-enumeration of devices on slot bringup / bringdown).

+ it plays with the reset_lock deep down in the long call chain. It's
also a very short file.

> > The only reason I've refrained from making major adjustments to this
> > structure in the past was that it would make "git blame" a little more
> > difficult and applying fixes to stable kernels would also become somewhat
> > more painful as it would require backporting.
> 
> Yeah, that's the main reason I haven't tried to do anything either.
> On the other hand, the browsing nuisance is an everyday thing forever
> if we leave it as-is.

I get half mad every time I need to browse code under hotplug/. I even 
started doing:

  cat ./pciehp*.[hc] | less -S 

...to workaround the constant need to jump between those files. I 
certainly would like to see the split gone especially between ctrl and 
hpc.

There are also some forward declaration within a file which are mostly not 
needed I think if the functions are shuffled around.

> I did consolidate portdrv.c a couple years ago
> and don't regret it.  But moving things definitely makes "git blame" a
> bit of a hassle; my notes are full of things like this:
> 
>   a1ccd3d91138 ("PCI/portdrv: Squash into portdrv.c")
>     squash drivers/pci/pcie/portdrv_pci.c and portdrv_core.c into portdrv.c
>   950bf6388bc2 ("PCI: Move DesignWare IP support to new drivers/pci/dwc/ directory")
>     mv drivers/pci/host/pci-imx6.c drivers/pci/dwc/pci-imx6.c
>   6e0832fa432e ("PCI: Collect all native drivers under drivers/pci/controller/")
>     mv drivers/pci/dwc/pci-imx6.c drivers/pci/controller/dwc/pci-imx6.c

Can't git blame be given -M -C to deal with this? Or are those truly lines 
that were introduced by the consolidation commit?

I usually need to look older changes as well since there is plenty of API 
reworks and other noise beyond such consolidations, so I tend to end up 
doing this a lot while browing the history of some code fragment with 
increasingly old commit IDs:

git annotate a1ccd3d911382^ portdrv_core.c

...to find the next points of interest in the history. So those commits 
don't stand out as much for me.

-- 
 i.

> > >   - I forgot why we have both pcie_write_cmd() and
> > >     pcie_write_cmd_nowait() and how to decide which to use.
> > 
> > pcie_write_cmd_nowait() is the "fire and forget" variant,
> > whereas pcie_write_cmd() can be thought of as the "_sync" variant,
> > i.e. the control flow doesn't continue until the command has been
> > processed by the slot.
> > 
> > E.g. pciehp_power_on_slot() waits for the slot command to complete
> > before making sure the Link Disable bit is clear.  It wouldn't make
> > much sense to do the latter when the former hasn't been completed yet.
> 
> Right, I know what the difference is; I guess I just don't know how to
> figure out when pcie_write_cmd_nowait() is safe.
> 
> Bjorn
> 

Re: [PATCH v2 1/1] PCI/hotplug: Don't enable HPIE in poll mode

[Bjorn Helgaas]

On Mon, Mar 24, 2025 at 01:00:33PM +0200, Ilpo Järvinen wrote:
> On Fri, 21 Mar 2025, Bjorn Helgaas wrote:
> 
> > On Fri, Mar 21, 2025 at 07:07:47PM +0100, Lukas Wunner wrote:
> > > On Fri, Mar 21, 2025 at 12:09:19PM -0500, Bjorn Helgaas wrote:
> > > ...

> > > >   - It's annoying that pcie_enable_interrupt() and
> > > >     pcie_disable_interrupt() are global symbols, a consequence of
> > > >     pciehp being split across five files instead of being one, which
> > > >     is also a nuisance for code browsing.
> ...

> > > The only reason I've refrained from making major adjustments to this
> > > structure in the past was that it would make "git blame" a little more
> > > difficult and applying fixes to stable kernels would also become somewhat
> > > more painful as it would require backporting.
> > 
> > Yeah, that's the main reason I haven't tried to do anything either.
> > On the other hand, the browsing nuisance is an everyday thing forever
> > if we leave it as-is.
> 
> I get half mad every time I need to browse code under hotplug/. I even 
> started doing:
> 
>   cat ./pciehp*.[hc] | less -S 
> 
> ...to workaround the constant need to jump between those files. I 
> certainly would like to see the split gone especially between ctrl and 
> hpc.

I would definitely take patches to consolidate pciehp and maybe
acpiphp.

The other drivers are annoying, too, but I'm not sure it's worth the
trouble since they're rarely used and updated.

> Can't git blame be given -M -C to deal with this? Or are those truly lines 
> that were introduced by the consolidation commit?

I'm so git-illiterate that I habitually just search "git log -p".  
"git blame -M -C ..." would probably be much more effective.

Bjorn