| Message ID | 20250321162114.3939-1-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2,1/1] PCI/hotplug: Don't enable HPIE in poll mode | expand |
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 >
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
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
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.
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
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); }
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(-) base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b