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