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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250321205114.GA1144421@bhelgaas>
Date: Fri, 21 Mar 2025 15:51:14 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Lukas Wunner <lukas@...ner.de>
Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] PCI/hotplug: Don't enable HPIE in poll mode

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