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(-)
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).
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@kernel.org>
Cc: Radu Rendec <rrendec@redhat.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Manivannan Sadhasivam <mani@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: Krzysztof Wilczyński <kwilczynski@kernel.org>
Cc: Lorenzo Pieralisi <lpieralisi@kernel.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));
}
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[] = {
--
2.47.3
On Thu, Nov 20, 2025 at 11:36:30AM +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). > > 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. Nits since you plan to repost: s/in turns/in turn/ (maybe a British/American idiom difference?) s/inneficient/inefficient/ s/structre/structure/ > 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. Nice simplification, thanks for doing this! Bjorn
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@kernel.org>
> Cc: Radu Rendec <rrendec@redhat.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Manivannan Sadhasivam <mani@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Krzysztof Wilczyński <kwilczynski@kernel.org>
> Cc: Lorenzo Pieralisi <lpieralisi@kernel.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.
>
> 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@redhat.com>
And thanks again for spending time on this and creating the patch.
--
Radu
On Thu, 20 Nov 2025 17:58:42 +0000,
Radu Rendec <rrendec@redhat.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@kernel.org>
> > Cc: Radu Rendec <rrendec@redhat.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Manivannan Sadhasivam <mani@kernel.org>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Krzysztof Wilczyński <kwilczynski@kernel.org>
> > Cc: Lorenzo Pieralisi <lpieralisi@kernel.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@redhat.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.
© 2016 - 2025 Red Hat, Inc.