| 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: <863467srea.wl-maz@kernel.org>
Date: Fri, 21 Nov 2025 08:48:13 +0000
From: Marc Zyngier <maz@...nel.org>
To: Radu Rendec <rrendec@...hat.com>
Cc: linux-pci@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, Bjorn Helgaas <bhelgaas@...gle.com>,
Manivannan Sadhasivam <mani@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Wilczyński <kwilczynski@...nel.org>, Lorenzo
Pieralisi <lpieralisi@...nel.org>
Subject: Re: [PATCH] PCI: host-generic: Move bridge allocation outside of pci_host_common_init()
On Thu, 20 Nov 2025 17:58:42 +0000,
Radu Rendec <rrendec@...hat.com> wrote:
>
> On Thu, 2025-11-20 at 11:36 +0000, Marc Zyngier wrote:
> > Having the host bridge allocation inside pci_host_common_init() results
> > in a lot of complexity in the pcie-apple driver (the only direct user
> > of this function outside of code PCI code).
> ^^^^
> nit: s/code/core :)
>
> > It forces the allocation of driver-specific tracking structures outside
> > of the bridge allocation, which in turns requires it to use inneficient
> > data structures to match the bridge and the private structre as needed.
> >
> > Instead, let the bridge structure be passed to pci_host_common_init(),
> > allowing the driver to allocate it together with the private data,
> > as it is usually intended. The driver can then retrieve the bridge
> > via the owning device attached to the PCI config window structure.
> > This allows the pcie-apple driver to be significantly simplified.
> >
> > Both core and driver code are changed in one go to avoid going via
> > a transitional interface.
> >
> > Link: https://lore.kernel.org/r/86jyzms036.wl-maz@kernel.org
> > Signed-off-by: Marc Zyngier <maz@...nel.org>
> > Cc: Radu Rendec <rrendec@...hat.com>
> > Cc: Bjorn Helgaas <bhelgaas@...gle.com>
> > Cc: Manivannan Sadhasivam <mani@...nel.org>
> > Cc: Rob Herring <robh@...nel.org>
> > Cc: Krzysztof Wilczyński <kwilczynski@...nel.org>
> > Cc: Lorenzo Pieralisi <lpieralisi@...nel.org>
> > ---
> > drivers/pci/controller/pci-host-common.c | 13 ++++----
> > drivers/pci/controller/pci-host-common.h | 1 +
> > drivers/pci/controller/pcie-apple.c | 42 ++++--------------------
> > 3 files changed, 14 insertions(+), 42 deletions(-)
> >
> > 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..a902aa81a497e 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,9 @@ 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;
> > - }
> > -
> > - return NULL;
> > + return pci_host_bridge_priv(dev_get_drvdata(dev));
> > }
> >
> >
>
> You forgot to remove the `entry` field from struct apple_pcie. It's no
> longer needed now that pcie_list is gone.
Ah, good point. Fixed.
>
> >
> > static struct apple_pcie_port *apple_pcie_get_port(struct pci_dev *pdev)
> > @@ -875,13 +849,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,13 +873,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);
> > -
> > - return ret;
> > + return pci_host_common_init(pdev, bridge, &apple_pcie_cfg_ecam_ops);
> > }
> >
> > static const struct of_device_id apple_pcie_of_match[] = {
>
> With those two nitpicks addressed,
>
> Reviewed-by: Radu Rendec <rrendec@...hat.com>
Thanks. I'll repost this patch some time next week, as this is only an
optimisation, not really a fix.
>
> And thanks again for spending time on this and creating the patch.
Nerd-sniping is a sport, it seems... ;-)
Cheers,
M.
--
Without deviation from the norm, progress is not possible.
Powered by blists - more mailing lists