From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id BC83FCF31AE for ; Wed, 19 Nov 2025 12:01:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Type:MIME-Version: References:In-Reply-To:Subject:Cc:To:From:Message-ID:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=WRP0FmGDHWbhiYaDNJ9+K9WacpqUIjBU8wZL4NCOEcE=; b=FFjtY2SVkh5HOTXEqnONE3ipGc 7yfEhmXWWOlVZxwUTa7ELIR2FUBbBgldX3xcVrCMHwtw9EMMz/2dANJcXAINl+JGiD4ZdquQn8fH3 YvqaXeHgaQoGrUJqMcrq03E1yyXZi2AsyNBOU+J/jWRmfeTaIUj0SFGQtnnWGx0rpB5an4WIWNuFj ApyBIAUnMTHIkMoK592C6XPMDo/wCSjZvlhh/eGho0lASVPOIlDPEzrWtMTKUbgBiaDkEzQaiyDwP tz27hx77w07dmKmQmWm4kh60wa82b26zwCkD/+iUJCyWLmZCoGkbP+0Fk8GEeWO+j+PU5m1gXLbGi ACNQNN0w==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1vLgsH-000000034zM-1yho; Wed, 19 Nov 2025 12:01:25 +0000 Received: from sea.source.kernel.org ([172.234.252.31]) by bombadil.infradead.org with esmtps (Exim 4.98.2 #2 (Red Hat Linux)) id 1vLgsE-000000034ys-22yu for linux-arm-kernel@lists.infradead.org; Wed, 19 Nov 2025 12:01:23 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sea.source.kernel.org (Postfix) with ESMTP id CAAF540BCF; Wed, 19 Nov 2025 12:01:21 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 55A3CC2BCB1; Wed, 19 Nov 2025 12:01:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1763553681; bh=pUd9GcXWVkOw+Wyv1/2YeXTij1093jTJf2tFx1gxPqA=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=O6fFcK8ZBkyit8zbUqkkjh9nVcoTRxc+NDVv0ghKMWjAfmkrrOwBGAzU11fJ3csY+ 0HiMCbB61IRbktQ1GzWkXYDVAJFTkNclODYuBrE9f0BNHcpk+AV1v9+kBr08bzsCXT hIOEX2P9DiB0SyqhjihEjeh/n9kEE7JqMk6VO7pv1f32q3kSvVwNyWkUzaPm+ekX7V 1iIlKJQgp9BedvY/bEsvKWYbvNgznE6WMFgbRdSOyzNHIVaH0deHvmn6QSHE4K+W4S 0tOLnKJ2wPTF/+ZGDIb5m1xbUzDUIRoa8sAb8T7IQq/+UvPBbXZYayOaTgqbB0KrDn ikXvHFebpObKA== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.98.2) (envelope-from ) id 1vLgsA-00000006W6j-1BYD; Wed, 19 Nov 2025 12:01:18 +0000 Date: Wed, 19 Nov 2025 12:01:17 +0000 Message-ID: <86jyzms036.wl-maz@kernel.org> From: Marc Zyngier To: Radu Rendec Cc: Bjorn Helgaas , Manivannan Sadhasivam , Will Deacon , Rob Herring , Krzysztof =?UTF-8?B?V2lsY3p5xYRza2k=?= , Lorenzo Pieralisi , linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] PCI: host-common: Do not set drvdata in pci_host_common_init() In-Reply-To: <20251118221244.372423-2-rrendec@redhat.com> References: <20251118221244.372423-1-rrendec@redhat.com> <20251118221244.372423-2-rrendec@redhat.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/30.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: rrendec@redhat.com, bhelgaas@google.com, mani@kernel.org, will@kernel.org, robh@kernel.org, kwilczynski@kernel.org, lpieralisi@kernel.org, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20251119_040122_566676_56253B9D X-CRM114-Status: GOOD ( 34.96 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, 18 Nov 2025 22:12:43 +0000, Radu Rendec wrote: > > Currently pci_host_common_init() uses the platform device's drvdata to > store the pointer to the allocated struct pci_host_bridge. This makes > sense for drivers that use pci_host_common_{probe,remove}() directly as > the platform probe/remove functions, but leaves no option for more > complex drivers to store a pointer to their own private data. > > Change pci_host_common_init() to return the pointer to the allocated > struct pci_host_bridge, and move the platform_set_drvdata() call to > pci_host_common_probe(). This way, drivers that implement their own > probe function can still use pci_host_common_init() but store their own > pointer in the platform device's drvdata. > > For symmetry, move the release code to a new function that takes a > pointer to struct pci_host_bridge, and make pci_host_common_release() a > wrapper to it that extracts the pointer from the platform device's > drvdata. This way, drivers that store their own private pointer in the > platform device's drvdata can still use the library release code. > > No functional change to the existing users of pci-host-common is > intended, with the exception of the pcie-apple driver, which is modified > in a subsequent patch. This paragraph doesn't belong here. Maybe as a note, but not in the commit message. > > Signed-off-by: Radu Rendec > --- > drivers/pci/controller/pci-host-common.c | 36 ++++++++++++++++-------- > drivers/pci/controller/pci-host-common.h | 6 ++-- > 2 files changed, 29 insertions(+), 13 deletions(-) > > diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c > index 810d1c8de24e9..86002195c93ac 100644 > --- a/drivers/pci/controller/pci-host-common.c > +++ b/drivers/pci/controller/pci-host-common.c > @@ -52,25 +52,24 @@ 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, > - const struct pci_ecam_ops *ops) > +struct pci_host_bridge *pci_host_common_init(struct platform_device *pdev, > + const struct pci_ecam_ops *ops) > { > struct device *dev = &pdev->dev; > struct pci_host_bridge *bridge; > struct pci_config_window *cfg; > + int rc; > > bridge = devm_pci_alloc_host_bridge(dev, 0); > if (!bridge) > - return -ENOMEM; > + return ERR_PTR(-ENOMEM); > > of_pci_check_probe_only(); > > - platform_set_drvdata(pdev, bridge); > - > /* Parse and map our Configuration Space windows */ > cfg = pci_host_common_ecam_create(dev, bridge, ops); > if (IS_ERR(cfg)) > - return PTR_ERR(cfg); > + return (struct pci_host_bridge *)cfg; > > bridge->sysdata = cfg; > bridge->ops = (struct pci_ops *)&ops->pci_ops; > @@ -78,31 +77,46 @@ int pci_host_common_init(struct platform_device *pdev, > bridge->disable_device = ops->disable_device; > bridge->msi_domain = true; > > - return pci_host_probe(bridge); > + rc = pci_host_probe(bridge); > + if (rc) > + return ERR_PTR(rc); > + > + return bridge; > } > 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 = pci_host_common_init(pdev, ops); > + if (IS_ERR(bridge)) > + return PTR_ERR(bridge); > + > + platform_set_drvdata(pdev, bridge); Congratulations, you just broke pcie-microchip-host.c again. Yes, I did that myself in afc0a570bb613871 ("PCI: host-generic: Extract an ECAM bridge creation helper from pci_host_common_probe()"), and it was fixed by Geert in bdb32a0f67806 ("PCI: host-generic: Set driver_data before calling gen_pci_init()"). > + > + return 0; > } > EXPORT_SYMBOL_GPL(pci_host_common_probe); > > -void pci_host_common_remove(struct platform_device *pdev) > +void pci_host_common_release(struct pci_host_bridge *bridge) > { > - struct pci_host_bridge *bridge = platform_get_drvdata(pdev); > - > pci_lock_rescan_remove(); > pci_stop_root_bus(bridge->bus); > pci_remove_root_bus(bridge->bus); > pci_unlock_rescan_remove(); > } > +EXPORT_SYMBOL_GPL(pci_host_common_release); Even with the pcie-apple.c driver change, this is never called. I'd refrain from adding an export until we actually have an identified user. > + > +void pci_host_common_remove(struct platform_device *pdev) > +{ > + pci_host_common_release(platform_get_drvdata(pdev)); > +} > EXPORT_SYMBOL_GPL(pci_host_common_remove); > > MODULE_DESCRIPTION("Common library for PCI host controller drivers"); > diff --git a/drivers/pci/controller/pci-host-common.h b/drivers/pci/controller/pci-host-common.h > index 51c35ec0cf37d..018e593bafe47 100644 > --- a/drivers/pci/controller/pci-host-common.h > +++ b/drivers/pci/controller/pci-host-common.h > @@ -11,11 +11,13 @@ > #define _PCI_HOST_COMMON_H > > struct pci_ecam_ops; > +struct pci_host_bridge; > > int pci_host_common_probe(struct platform_device *pdev); > -int pci_host_common_init(struct platform_device *pdev, > - const struct pci_ecam_ops *ops); > +struct pci_host_bridge *pci_host_common_init(struct platform_device *pdev, > + const struct pci_ecam_ops *ops); > void pci_host_common_remove(struct platform_device *pdev); > +void pci_host_common_release(struct pci_host_bridge *bridge); > > struct pci_config_window *pci_host_common_ecam_create(struct device *dev, > struct pci_host_bridge *bridge, const struct pci_ecam_ops *ops); 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. Thanks, M. -- Without deviation from the norm, progress is not possible.