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
>