| 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
| ||
|
Message-ID: <868qg1rrb8.wl-maz@kernel.org>
Date: Thu, 20 Nov 2025 09:23:07 +0000
From: Marc Zyngier <maz@...nel.org>
To: Radu Rendec <rrendec@...hat.com>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, Manivannan Sadhasivam
<mani@...nel.org>, Will Deacon <will@...nel.org>, Rob Herring
<robh@...nel.org>, Krzysztof Wilczyński
<kwilczynski@...nel.org>, Lorenzo Pieralisi <lpieralisi@...nel.org>,
linux-pci@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] PCI: host-common: Do not set drvdata in pci_host_common_init()
On Wed, 19 Nov 2025 16:19:14 +0000,
Radu Rendec <rrendec@...hat.com> wrote:
>
> On Wed, 2025-11-19 at 12:01 +0000, Marc Zyngier wrote:
> > On Tue, 18 Nov 2025 22:12:43 +0000,
> > Radu Rendec <rrendec@...hat.com> wrote:
[...]
> > My concern with this is two-fold:
> >
> > - it is yet another obscure API change with odd side effects,
> > requiring to track and understand the per-driver flow of information
> > (and the apple pcie driver is a prime example of how hard this is)
> >
> > - we can't directly associate the PCIe port data with the bridge like
> > must drivers do, because the bridge allocation is done outside of
> > the calling driver, and there is no link back to the bridge from
> > pci_config_window.
> >
> > I'd rather that last point was addressed so that we could make the
> > Apple driver behave similarly to other drivers, and let it use the
> > bridge private data for its PCIe port information.
>
> Using the bridge private data to store the driver's private data was my
> first thought too. In fact, in the first version of my (local) changes
> I added a "size" parameter to pci_host_common_init(), to pass it down
> to devm_pci_alloc_host_bridge() as the priv size.
>
> But there's another problem with this approach: pci_host_common_init()
> also calls pci_host_common_ecam_create() -> pci_ecam_create() ->
> ops->init(), so init() would be called with the bridge private area
> allocated but not populated.
>
> I don't see an elegant solution to this. The two options that crossed
> my mind are these:
> * Add yet another parameter to pci_host_common_init(), a void *, so it
> takes a void *priv and a size_t size. The size would be passed down
> to devm_pci_alloc_host_bridge(), then `size` bytes would be copied
> from `priv` to the bridge private data area. What I don't like about
> this is the extra memcpy() and the fact that the calling driver
> would have to prepare an "offline" copy of its private data (likely
> as a local variable) and pass it to pci_host_common_init().
> * Pass just the void *priv to pci_host_common_init(). By convention,
> the bridge private data would be used to store that void *priv
> itself, which is a pointer to the driver's private data. So, the 2nd
> parameter to devm_pci_alloc_host_bridge() would be sizeof(void *).
> The downside is that there's an extra pointer indirection.
I don't think either method is appealing. I expect the correct way
would be to let individual drivers allocate the bridge (something that
is already pretty common), initialise it as required, and pass that to
pci_host_common_init().
> In any case, your other concern about linking back to the bridge from
> pci_config_window would be addressed because pci_config_window has a
> pointer to the device (the `parent` field), which (in this scenario, by
> convention) has the bridge pointer stored as drvdata.
Ah, of course, thanks for pointing that out.
> If any of the options above looks appealing, or if you have a better
> idea, please let me know, and I can prepare a v2.
See the patch below, which I have actually tested on an M2-pro box.
M.
diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
index 810d1c8de24e9..c473e7c03baca 100644
--- a/drivers/pci/controller/pci-host-common.c
+++ b/drivers/pci/controller/pci-host-common.c
@@ -53,16 +53,12 @@ struct pci_config_window *pci_host_common_ecam_create(struct device *dev,
EXPORT_SYMBOL_GPL(pci_host_common_ecam_create);
int pci_host_common_init(struct platform_device *pdev,
+ struct pci_host_bridge *bridge,
const struct pci_ecam_ops *ops)
{
struct device *dev = &pdev->dev;
- struct pci_host_bridge *bridge;
struct pci_config_window *cfg;
- bridge = devm_pci_alloc_host_bridge(dev, 0);
- if (!bridge)
- return -ENOMEM;
-
of_pci_check_probe_only();
platform_set_drvdata(pdev, bridge);
@@ -85,12 +81,17 @@ EXPORT_SYMBOL_GPL(pci_host_common_init);
int pci_host_common_probe(struct platform_device *pdev)
{
const struct pci_ecam_ops *ops;
+ struct pci_host_bridge *bridge;
ops = of_device_get_match_data(&pdev->dev);
if (!ops)
return -ENODEV;
- return pci_host_common_init(pdev, ops);
+ bridge = devm_pci_alloc_host_bridge(&pdev->dev, 0);
+ if (!bridge)
+ return -ENOMEM;
+
+ return pci_host_common_init(pdev, bridge, ops);
}
EXPORT_SYMBOL_GPL(pci_host_common_probe);
diff --git a/drivers/pci/controller/pci-host-common.h b/drivers/pci/controller/pci-host-common.h
index 51c35ec0cf37d..b5075d4bd7eb3 100644
--- a/drivers/pci/controller/pci-host-common.h
+++ b/drivers/pci/controller/pci-host-common.h
@@ -14,6 +14,7 @@ struct pci_ecam_ops;
int pci_host_common_probe(struct platform_device *pdev);
int pci_host_common_init(struct platform_device *pdev,
+ struct pci_host_bridge *bridge,
const struct pci_ecam_ops *ops);
void pci_host_common_remove(struct platform_device *pdev);
diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index 0380d300adca6..701fba1e45db9 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -206,9 +206,6 @@ struct apple_pcie_port {
int idx;
};
-static LIST_HEAD(pcie_list);
-static DEFINE_MUTEX(pcie_list_lock);
-
static void rmw_set(u32 set, void __iomem *addr)
{
writel_relaxed(readl_relaxed(addr) | set, addr);
@@ -724,32 +721,12 @@ static int apple_msi_init(struct apple_pcie *pcie)
return 0;
}
-static void apple_pcie_register(struct apple_pcie *pcie)
-{
- guard(mutex)(&pcie_list_lock);
-
- list_add_tail(&pcie->entry, &pcie_list);
-}
-
-static void apple_pcie_unregister(struct apple_pcie *pcie)
-{
- guard(mutex)(&pcie_list_lock);
-
- list_del(&pcie->entry);
-}
-
static struct apple_pcie *apple_pcie_lookup(struct device *dev)
{
- struct apple_pcie *pcie;
-
- guard(mutex)(&pcie_list_lock);
-
- list_for_each_entry(pcie, &pcie_list, entry) {
- if (pcie->dev == dev)
- return pcie;
- }
+ struct pci_host_bridge *bridge;
- return NULL;
+ bridge = dev_get_drvdata(dev);
+ return pci_host_bridge_priv(bridge);
}
static struct apple_pcie_port *apple_pcie_get_port(struct pci_dev *pdev)
@@ -875,13 +852,15 @@ static const struct pci_ecam_ops apple_pcie_cfg_ecam_ops = {
static int apple_pcie_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
+ struct pci_host_bridge *bridge;
struct apple_pcie *pcie;
int ret;
- pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
- if (!pcie)
+ bridge = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
+ if (!bridge)
return -ENOMEM;
+ pcie = pci_host_bridge_priv(bridge);
pcie->dev = dev;
pcie->hw = of_device_get_match_data(dev);
if (!pcie->hw)
@@ -897,12 +876,7 @@ static int apple_pcie_probe(struct platform_device *pdev)
if (ret)
return ret;
- apple_pcie_register(pcie);
-
- ret = pci_host_common_init(pdev, &apple_pcie_cfg_ecam_ops);
- if (ret)
- apple_pcie_unregister(pcie);
-
+ ret = pci_host_common_init(pdev, bridge, &apple_pcie_cfg_ecam_ops);
return ret;
}
--
Without deviation from the norm, progress is not possible.
Powered by blists - more mailing lists