[{"id":3687567,"web_url":"http://patchwork.ozlabs.org/comment/3687567/","msgid":"<20260507075959.47D77C2BCB8@smtp.kernel.org>","list_archive_url":null,"date":"2026-05-07T07:59:58","subject":"Re: [PATCH v8 phy-next 09/31] PCI: Remove device links to PHY","submitter":{"id":93311,"url":"http://patchwork.ozlabs.org/api/people/93311/","name":null,"email":"sashiko-bot@kernel.org"},"content":"Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:\n- [Critical] Critical memory corruption in cdns_plat_pcie_shutdown() due to incorrect pointer cast.\n- [High] Asynchronous pm_runtime_put() is called immediately before pm_runtime_disable(), cancelling suspend transitions.\n- [High] Shared IRQ handlers access hardware registers without checking PM runtime state, causing spurious interrupts or bus errors.\n--\n\ncommit abda94a24ef1bb0817706ae87aa20f04251c14ad\nAuthor: Vladimir Oltean \n\nPCI: Remove device links to PHY\n\nThis commit removes device links between the PCIe controllers and PHY\nproviders in the Cadence, DRA7xx, and Keystone drivers. The device links\nare now redundant since the PHY core automatically adds device links to\norder suspend/resume operations.\n\n> diff --git a/drivers/pci/controller/cadence/pcie-cadence-plat.c b/drivers/pci/controller/cadence/pcie-cadence-plat.c\n> index 8b12a46b56019..6fd16e09e2444 100644\n> --- a/drivers/pci/controller/cadence/pcie-cadence-plat.c\n> +++ b/drivers/pci/controller/cadence/pcie-cadence-plat.c\n> @@ -41,7 +41,6 @@ static int cdns_plat_pcie_probe(struct platform_device *pdev)\n> \tstruct pci_host_bridge *bridge;\n> \tstruct cdns_pcie_ep *ep;\n> \tstruct cdns_pcie_rc *rc;\n> -\tint phy_count;\n> \tbool is_rc;\n> \tint ret;\n\nIn cdns_plat_pcie_probe(), the driver data is initialized to point to a\nstruct cdns_plat_pcie object:\n\n\tcdns_plat_pcie = devm_kzalloc(dev, sizeof(*cdns_plat_pcie), GFP_KERNEL);\n\t...\n\tplatform_set_drvdata(pdev, cdns_plat_pcie);\n\nHowever, in cdns_plat_pcie_shutdown(), the pointer is extracted and cast\ndirectly to struct cdns_pcie *:\n\nstatic void cdns_plat_pcie_shutdown(struct platform_device *pdev)\n{\n\tstruct device *dev = &pdev->dev;\n\tstruct cdns_pcie *pcie = dev_get_drvdata(dev);\n\t...\n\tcdns_pcie_disable_phy(pcie);\n}\n\nSince struct cdns_plat_pcie is only 8 bytes large, but the phy_count field\nis at an offset of roughly 36 bytes in struct cdns_pcie, does this lead to\nan out-of-bounds read and memory corruption when cdns_pcie_disable_phy()\nattempts to iterate over the PHYs?\n\n[ ... ]\n> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c\n> index fe4836925c4e3..cab9fad212add 100644\n> --- a/drivers/pci/controller/dwc/pci-keystone.c\n> +++ b/drivers/pci/controller/dwc/pci-keystone.c\n[ ... ]\n> @@ -1337,25 +1321,18 @@ static int ks_pcie_probe(struct platform_device *pdev)\n> \tpm_runtime_disable(dev);\n> \tks_pcie_disable_phy(ks_pcie);\n> \n> -err_link:\n> -\twhile (--i >= 0 && link[i])\n> -\t\tdevice_link_del(link[i]);\n> -\n> +err:\n> \treturn ret;\n> }\n> \n> static void ks_pcie_remove(struct platform_device *pdev)\n> {\n> \tstruct keystone_pcie *ks_pcie = platform_get_drvdata(pdev);\n> -\tstruct device_link **link = ks_pcie->link;\n> -\tint num_lanes = ks_pcie->num_lanes;\n> \tstruct device *dev = &pdev->dev;\n> \n> \tpm_runtime_put(dev);\n> \tpm_runtime_disable(dev);\n\nDoes calling pm_runtime_put() immediately before pm_runtime_disable() cancel\nthe suspend transition?\n\npm_runtime_put() queues an asynchronous idle or suspend work item, but\ncalling pm_runtime_disable() immediately afterward cancels this pending\ntransition via __pm_runtime_barrier().\n\nCould this leave the hardware in an active or incorrect power state from\nthe controller's perspective? Would it be safer to use synchronous\npm_runtime_put_sync() instead?\n\nThis asynchronous put pattern also appears in the error paths of\ndra7xx_pcie_probe() and ks_pcie_probe().\n\nWhile reviewing the power management handling in these drivers, I also\nnoticed that the shared interrupt handlers read hardware registers directly\nwithout verifying if the device is active:\n\ndrivers/pci/controller/dwc/pci-dra7xx.c:dra7xx_pcie_irq_handler() {\n\t...\n\treg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN);\n\nIf another device sharing the same IRQ line triggers an interrupt while the\nPCIe controller is runtime suspended, will these handlers attempt to read\npowered-down hardware?\n\nShould the handlers check the PM runtime state using\npm_runtime_get_if_active() to prevent spurious interrupts or bus errors?","headers":{"Return-Path":"\n ","X-Original-To":["incoming@patchwork.ozlabs.org","linux-pci@vger.kernel.org"],"Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256\n header.s=k20201202 header.b=cHBVjQO+;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2600:3c0a:e001:db::12fc:5321; helo=sea.lore.kernel.org;\n envelope-from=linux-pci+bounces-54049-incoming=patchwork.ozlabs.org@vger.kernel.org;\n receiver=patchwork.ozlabs.org)","smtp.subspace.kernel.org;\n\tdkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=\"cHBVjQO+\"","smtp.subspace.kernel.org;\n arc=none smtp.client-ip=10.30.226.201"],"Received":["from sea.lore.kernel.org (sea.lore.kernel.org\n [IPv6:2600:3c0a:e001:db::12fc:5321])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange x25519)\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4gB4RV09gNz1yKd\n\tfor ; Thu, 07 May 2026 18:00:06 +1000 (AEST)","from smtp.subspace.kernel.org (conduit.subspace.kernel.org\n [100.90.174.1])\n\tby sea.lore.kernel.org (Postfix) with ESMTP id 78A8E302A2D4\n\tfor ; Thu, 7 May 2026 08:00:03 +0000 (UTC)","from localhost.localdomain (localhost.localdomain [127.0.0.1])\n\tby smtp.subspace.kernel.org (Postfix) with ESMTP id 409B037EFE9;\n\tThu, 7 May 2026 08:00:02 +0000 (UTC)","from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org\n [10.30.226.201])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby smtp.subspace.kernel.org (Postfix) with ESMTPS id 4BF6D37AA7E\n\tfor ; Thu, 7 May 2026 08:00:00 +0000 (UTC)","by smtp.kernel.org (Postfix) with ESMTPSA id 47D77C2BCB8;\n\tThu, 7 May 2026 07:59:59 +0000 (UTC)"],"ARC-Seal":"i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;\n\tt=1778140800; cv=none;\n b=j7TxXNF+sTlYWnqbFsGzNgnZuFhGW+5McFdSSeBmooTqfo7Zea2tFpDnKjrwaoCqvOwyu5m44wXHPVKsEn7eNa/j2/OyKZJ9KrDud3SK3FZ0v3nij5OUkNrSZcCmHr9Om+ynkdF0UaXwFVuwzpgn2rOcbzhHxWkBOzxz2/6itz4=","ARC-Message-Signature":"i=1; a=rsa-sha256; d=subspace.kernel.org;\n\ts=arc-20240116; t=1778140800; c=relaxed/simple;\n\tbh=TVSu35uwVUU84kaaUUHzbGpIol7cjYWHmARM/A+4SiA=;\n\th=From:Subject:To:Cc:In-Reply-To:References:Content-Type:Date:\n\t Message-Id;\n b=JT9MN9ilm0at7riTG+pm4cKjJs6hJDRT1zyYIjDKpL1Rc6/tuozJ/Mi2fCi/ZTB/OMfaqjGjCqeD2oMIXH4+2uwd4gT1Ye2UTbp1s9xVHLMJpEX1Fyr3qPrMg8S6K2DUi3YemaZaLAoLiPGnNR5b8M7teWS700Gn5TS6Oo5UXJA=","ARC-Authentication-Results":"i=1; smtp.subspace.kernel.org;\n dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=cHBVjQO+; arc=none smtp.client-ip=10.30.226.201","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n\ts=k20201202; t=1778140799;\n\tbh=TVSu35uwVUU84kaaUUHzbGpIol7cjYWHmARM/A+4SiA=;\n\th=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date:From;\n\tb=cHBVjQO+6UM3oMKrPM4eFz7oiRTWog0CVW4Z92erzruq5lrnN9SN8RvXYauE8vAs2\n\t N84Zl9oWu1EFokry5P4O4rTCid3Y6xoxI7vjAltjRg030CMuafjG9CJmXT2lDfdd8j\n\t W8UtGdqTPSUpib6xnmgmYsfIqpldlNoocNS5XSV9g/xLZjjDJ0ePZhSeljwuvWmWC3\n\t n5qvzbS96lpAH5wYCUjvgu/Os2Ilk2DDlbwzIYzezIkEQBFXbSpyBjjjjafpoKwRT6\n\t TqzQ62IiTVO0JrDfOAPhHZy7xwgl0fAz34tJpTSTIMNxxjrizJXsjSBifkoWxDECsx\n\t Bn+aC++jG2iMA==","From":"sashiko-bot@kernel.org","Subject":"Re: [PATCH v8 phy-next 09/31] PCI: Remove device links to PHY","Reply-To":"sashiko@lists.linux.dev","To":"\"Vladimir Oltean\" ","Cc":"wsa+renesas@sang-engineering.com, linux-pci@vger.kernel.org","In-Reply-To":"<20260505100523.1922388-10-vladimir.oltean@nxp.com>","References":"<20260505100523.1922388-10-vladimir.oltean@nxp.com>","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","Date":"Thu, 07 May 2026 07:59:58 +0000","Message-Id":"<20260507075959.47D77C2BCB8@smtp.kernel.org>","Precedence":"bulk","X-Mailing-List":"linux-pci@vger.kernel.org","List-Id":"","List-Subscribe":"","List-Unsubscribe":""}},{"id":3687897,"web_url":"http://patchwork.ozlabs.org/comment/3687897/","msgid":"<20260507154744.GA971447@bhelgaas>","list_archive_url":null,"date":"2026-05-07T15:47:44","subject":"Re: [PATCH v8 phy-next 09/31] PCI: Remove device links to PHY","submitter":{"id":67298,"url":"http://patchwork.ozlabs.org/api/people/67298/","name":"Bjorn Helgaas","email":"helgaas@kernel.org"},"content":"[+to Manikandan, Siddharth, Vignesh, Hans]\n\nOn Thu, May 07, 2026 at 07:59:58AM +0000, sashiko-bot@kernel.org wrote:\n> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:\n> - [Critical] Critical memory corruption in cdns_plat_pcie_shutdown() due to incorrect pointer cast.\n> - [High] Asynchronous pm_runtime_put() is called immediately before pm_runtime_disable(), cancelling suspend transitions.\n> - [High] Shared IRQ handlers access hardware registers without checking PM runtime state, causing spurious interrupts or bus errors.\n\nPlease take a look at these potential issues and see whether changes\nare needed.\n\npcie-cadence-plat.c and pci-keystone.c are orphaned since\n\n 0ee2030af4e3 (\"MAINTAINERS: Orphan Cadence PCIe IP\")\n 57e1d8206e48 (\"MAINTAINERS: move Murali Karicheri to credits\")\n\nWould anybody like to step up and take care of them?\n\n> commit abda94a24ef1bb0817706ae87aa20f04251c14ad\n> Author: Vladimir Oltean \n> \n> PCI: Remove device links to PHY\n> \n> This commit removes device links between the PCIe controllers and PHY\n> providers in the Cadence, DRA7xx, and Keystone drivers. The device links\n> are now redundant since the PHY core automatically adds device links to\n> order suspend/resume operations.\n> \n> > diff --git a/drivers/pci/controller/cadence/pcie-cadence-plat.c b/drivers/pci/controller/cadence/pcie-cadence-plat.c\n> > index 8b12a46b56019..6fd16e09e2444 100644\n> > --- a/drivers/pci/controller/cadence/pcie-cadence-plat.c\n> > +++ b/drivers/pci/controller/cadence/pcie-cadence-plat.c\n> > @@ -41,7 +41,6 @@ static int cdns_plat_pcie_probe(struct platform_device *pdev)\n> > \tstruct pci_host_bridge *bridge;\n> > \tstruct cdns_pcie_ep *ep;\n> > \tstruct cdns_pcie_rc *rc;\n> > -\tint phy_count;\n> > \tbool is_rc;\n> > \tint ret;\n> \n> In cdns_plat_pcie_probe(), the driver data is initialized to point to a\n> struct cdns_plat_pcie object:\n> \n> \tcdns_plat_pcie = devm_kzalloc(dev, sizeof(*cdns_plat_pcie), GFP_KERNEL);\n> \t...\n> \tplatform_set_drvdata(pdev, cdns_plat_pcie);\n> \n> However, in cdns_plat_pcie_shutdown(), the pointer is extracted and cast\n> directly to struct cdns_pcie *:\n> \n> static void cdns_plat_pcie_shutdown(struct platform_device *pdev)\n> {\n> \tstruct device *dev = &pdev->dev;\n> \tstruct cdns_pcie *pcie = dev_get_drvdata(dev);\n> \t...\n> \tcdns_pcie_disable_phy(pcie);\n> }\n> \n> Since struct cdns_plat_pcie is only 8 bytes large, but the phy_count field\n> is at an offset of roughly 36 bytes in struct cdns_pcie, does this lead to\n> an out-of-bounds read and memory corruption when cdns_pcie_disable_phy()\n> attempts to iterate over the PHYs?\n> \n> [ ... ]\n> > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c\n> > index fe4836925c4e3..cab9fad212add 100644\n> > --- a/drivers/pci/controller/dwc/pci-keystone.c\n> > +++ b/drivers/pci/controller/dwc/pci-keystone.c\n> [ ... ]\n> > @@ -1337,25 +1321,18 @@ static int ks_pcie_probe(struct platform_device *pdev)\n> > \tpm_runtime_disable(dev);\n> > \tks_pcie_disable_phy(ks_pcie);\n> > \n> > -err_link:\n> > -\twhile (--i >= 0 && link[i])\n> > -\t\tdevice_link_del(link[i]);\n> > -\n> > +err:\n> > \treturn ret;\n> > }\n> > \n> > static void ks_pcie_remove(struct platform_device *pdev)\n> > {\n> > \tstruct keystone_pcie *ks_pcie = platform_get_drvdata(pdev);\n> > -\tstruct device_link **link = ks_pcie->link;\n> > -\tint num_lanes = ks_pcie->num_lanes;\n> > \tstruct device *dev = &pdev->dev;\n> > \n> > \tpm_runtime_put(dev);\n> > \tpm_runtime_disable(dev);\n> \n> Does calling pm_runtime_put() immediately before pm_runtime_disable() cancel\n> the suspend transition?\n> \n> pm_runtime_put() queues an asynchronous idle or suspend work item, but\n> calling pm_runtime_disable() immediately afterward cancels this pending\n> transition via __pm_runtime_barrier().\n> \n> Could this leave the hardware in an active or incorrect power state from\n> the controller's perspective? Would it be safer to use synchronous\n> pm_runtime_put_sync() instead?\n> \n> This asynchronous put pattern also appears in the error paths of\n> dra7xx_pcie_probe() and ks_pcie_probe().\n> \n> While reviewing the power management handling in these drivers, I also\n> noticed that the shared interrupt handlers read hardware registers directly\n> without verifying if the device is active:\n> \n> drivers/pci/controller/dwc/pci-dra7xx.c:dra7xx_pcie_irq_handler() {\n> \t...\n> \treg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN);\n> \n> If another device sharing the same IRQ line triggers an interrupt while the\n> PCIe controller is runtime suspended, will these handlers attempt to read\n> powered-down hardware?\n> \n> Should the handlers check the PM runtime state using\n> pm_runtime_get_if_active() to prevent spurious interrupts or bus errors?\n> \n> -- \n> Sashiko AI review · https://sashiko.dev/#/patchset/20260505100523.1922388-1-vladimir.oltean@nxp.com?part=9","headers":{"Return-Path":"\n ","X-Original-To":["incoming@patchwork.ozlabs.org","linux-pci@vger.kernel.org"],"Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256\n header.s=k20201202 header.b=aR+rpeoE;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=172.105.105.114; helo=tor.lore.kernel.org;\n envelope-from=linux-pci+bounces-54093-incoming=patchwork.ozlabs.org@vger.kernel.org;\n receiver=patchwork.ozlabs.org)","smtp.subspace.kernel.org;\n\tdkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=\"aR+rpeoE\"","smtp.subspace.kernel.org;\n arc=none smtp.client-ip=10.30.226.201"],"Received":["from tor.lore.kernel.org (tor.lore.kernel.org [172.105.105.114])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange x25519)\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4gBGwN4PMsz1yM5\n\tfor ; Fri, 08 May 2026 01:52:20 +1000 (AEST)","from smtp.subspace.kernel.org (conduit.subspace.kernel.org\n [100.90.174.1])\n\tby tor.lore.kernel.org (Postfix) with ESMTP id 1EDBC308B1F4\n\tfor ; Thu, 7 May 2026 15:47:47 +0000 (UTC)","from localhost.localdomain (localhost.localdomain [127.0.0.1])\n\tby smtp.subspace.kernel.org (Postfix) with ESMTP id 2A7AB3A9D8F;\n\tThu, 7 May 2026 15:47:46 +0000 (UTC)","from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org\n [10.30.226.201])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby smtp.subspace.kernel.org (Postfix) with ESMTPS id 0740F3BED55\n\tfor ; Thu, 7 May 2026 15:47:45 +0000 (UTC)","by smtp.kernel.org (Postfix) with ESMTPSA id 713D4C2BCB2;\n\tThu, 7 May 2026 15:47:45 +0000 (UTC)"],"ARC-Seal":"i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;\n\tt=1778168866; cv=none;\n b=TKtQkgCBUXHm9AIn2zYPPmMpnqulw8KNxJc42shl+p5tj6hnvaymmZmxxenAnPQvysSiGQKqGjHj/c116db99Fpiz99bvamIAYOD3JtUspgARmrKn0WkkUqRVriTifBCTuKA/Enu4ERpZvF5285PM7XatX4ZHjhVtw5ZJPhV50g=","ARC-Message-Signature":"i=1; a=rsa-sha256; d=subspace.kernel.org;\n\ts=arc-20240116; t=1778168866; c=relaxed/simple;\n\tbh=XnKk5NVsgHUStExAtIyjAjrkPvaMaYkuwLAtAgF99oM=;\n\th=Date:From:To:Cc:Subject:Message-ID:MIME-Version:Content-Type:\n\t Content-Disposition:In-Reply-To;\n b=mFCmkVJoQ0FbNn4vmGiVFoVfLxRl/ta2xChycxCMDdJNSyeQbX+UC2yH7IsDQFZcwEdVrs0C2WOpoVNgID8VtqkZWGjcQE8krvV0cOIjGg8flwHSbDdrrB1jnOaZM7wqTjN8HgrxD2qI399Sh8uffl+UbxAQHmlvoa4fDLX3g8k=","ARC-Authentication-Results":"i=1; smtp.subspace.kernel.org;\n dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=aR+rpeoE; arc=none smtp.client-ip=10.30.226.201","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n\ts=k20201202; t=1778168865;\n\tbh=XnKk5NVsgHUStExAtIyjAjrkPvaMaYkuwLAtAgF99oM=;\n\th=Date:From:To:Cc:Subject:In-Reply-To:From;\n\tb=aR+rpeoEEuUvLTKbCRsnjdzr3se5Vpcx8pMG+byRmhQ3Ol/7IvJygBtZUy8u2UzHO\n\t n6xMY68m+HJ4f9/i1Rj6BvDd6cbm0YzbfAJfXnJ07bSgIeEp1ycl9TLrUvBJUPIhLP\n\t nrsxC0shv3ThUiABgQ0qbWbcz/dS8Drpsa24aHoDHbCUenZ313s+qovNGSzKA0dMM3\n\t qccKghxdWyjCs5I29SiRDIpKTXj1IkgHE3eXaUdin1Td9feLhaIU4QQVmAOqA+pV7C\n\t 4G+Uf4QF+hjWK18ZzPSLobZnzKNlfsWS4hqq1ln5bjJoQQxqjJyDoFy/WOwMo9X8zz\n\t uiFyUUKj4GhCw==","Date":"Thu, 7 May 2026 10:47:44 -0500","From":"Bjorn Helgaas ","To":"Manikandan K Pillai ,\n\tSiddharth Vadapalli ,\n\tVignesh Raghavendra ,\n\tHans Zhang ","Cc":"Vladimir Oltean ,\n\twsa+renesas@sang-engineering.com, linux-pci@vger.kernel.org","Subject":"Re: [PATCH v8 phy-next 09/31] PCI: Remove device links to PHY","Message-ID":"<20260507154744.GA971447@bhelgaas>","Precedence":"bulk","X-Mailing-List":"linux-pci@vger.kernel.org","List-Id":"","List-Subscribe":"","List-Unsubscribe":"","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20260507075959.47D77C2BCB8@smtp.kernel.org>"}},{"id":3688121,"web_url":"http://patchwork.ozlabs.org/comment/3688121/","msgid":"<938ad958-f057-4909-8547-80b95f224864@163.com>","list_archive_url":null,"date":"2026-05-08T02:14:36","subject":"Re: [PATCH v8 phy-next 09/31] PCI: Remove device links to PHY","submitter":{"id":89937,"url":"http://patchwork.ozlabs.org/api/people/89937/","name":"Hans Zhang","email":"18255117159@163.com"},"content":"On 5/7/26 23:47, Bjorn Helgaas wrote:\n> [+to Manikandan, Siddharth, Vignesh, Hans]\n> \n> On Thu, May 07, 2026 at 07:59:58AM +0000, sashiko-bot@kernel.org wrote:\n>> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:\n>> - [Critical] Critical memory corruption in cdns_plat_pcie_shutdown() due to incorrect pointer cast.\n>> - [High] Asynchronous pm_runtime_put() is called immediately before pm_runtime_disable(), cancelling suspend transitions.\n>> - [High] Shared IRQ handlers access hardware registers without checking PM runtime state, causing spurious interrupts or bus errors.\n> \n> Please take a look at these potential issues and see whether changes\n> are needed.\n> \n> pcie-cadence-plat.c and pci-keystone.c are orphaned since\n> \n> 0ee2030af4e3 (\"MAINTAINERS: Orphan Cadence PCIe IP\")\n> 57e1d8206e48 (\"MAINTAINERS: move Murali Karicheri to credits\")\n> \n> Would anybody like to step up and take care of them?\n\nHi Bjorn,\n\n\nI have sent a separate email to Manikandan to inquire if he has the time \nto maintain the driver related to Cadence PCIe IP. If Manikandan replies \nto the email, I will then reply to you.\n\nBecause Manikandan has a better understanding of Cadence IP's registers \nthan our clients do, we have also previously collaborated on the \nupstream work of the HPA driver. Additionally, I also asked Siddharth \nand Vignesh directly if they had the time. My idea is that we all work \ntogether to maintain these drives.\n\n\nThe email address hans.zhang@cixtech.com is no longer valid. Please use \nHans Zhang <18255117159@163.com> instead.\n\nThe maintainer of the file drivers/pci/controller/cadence/pci-sky1.c: I \nwill update my email address.\n\nBest regards,\nHans\n\n> \n>> commit abda94a24ef1bb0817706ae87aa20f04251c14ad\n>> Author: Vladimir Oltean \n>>\n>> PCI: Remove device links to PHY\n>>\n>> This commit removes device links between the PCIe controllers and PHY\n>> providers in the Cadence, DRA7xx, and Keystone drivers. The device links\n>> are now redundant since the PHY core automatically adds device links to\n>> order suspend/resume operations.\n>>\n>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-plat.c b/drivers/pci/controller/cadence/pcie-cadence-plat.c\n>>> index 8b12a46b56019..6fd16e09e2444 100644\n>>> --- a/drivers/pci/controller/cadence/pcie-cadence-plat.c\n>>> +++ b/drivers/pci/controller/cadence/pcie-cadence-plat.c\n>>> @@ -41,7 +41,6 @@ static int cdns_plat_pcie_probe(struct platform_device *pdev)\n>>> \tstruct pci_host_bridge *bridge;\n>>> \tstruct cdns_pcie_ep *ep;\n>>> \tstruct cdns_pcie_rc *rc;\n>>> -\tint phy_count;\n>>> \tbool is_rc;\n>>> \tint ret;\n>>\n>> In cdns_plat_pcie_probe(), the driver data is initialized to point to a\n>> struct cdns_plat_pcie object:\n>>\n>> \tcdns_plat_pcie = devm_kzalloc(dev, sizeof(*cdns_plat_pcie), GFP_KERNEL);\n>> \t...\n>> \tplatform_set_drvdata(pdev, cdns_plat_pcie);\n>>\n>> However, in cdns_plat_pcie_shutdown(), the pointer is extracted and cast\n>> directly to struct cdns_pcie *:\n>>\n>> static void cdns_plat_pcie_shutdown(struct platform_device *pdev)\n>> {\n>> \tstruct device *dev = &pdev->dev;\n>> \tstruct cdns_pcie *pcie = dev_get_drvdata(dev);\n>> \t...\n>> \tcdns_pcie_disable_phy(pcie);\n>> }\n>>\n>> Since struct cdns_plat_pcie is only 8 bytes large, but the phy_count field\n>> is at an offset of roughly 36 bytes in struct cdns_pcie, does this lead to\n>> an out-of-bounds read and memory corruption when cdns_pcie_disable_phy()\n>> attempts to iterate over the PHYs?\n>>\n>> [ ... ]\n>>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c\n>>> index fe4836925c4e3..cab9fad212add 100644\n>>> --- a/drivers/pci/controller/dwc/pci-keystone.c\n>>> +++ b/drivers/pci/controller/dwc/pci-keystone.c\n>> [ ... ]\n>>> @@ -1337,25 +1321,18 @@ static int ks_pcie_probe(struct platform_device *pdev)\n>>> \tpm_runtime_disable(dev);\n>>> \tks_pcie_disable_phy(ks_pcie);\n>>> \n>>> -err_link:\n>>> -\twhile (--i >= 0 && link[i])\n>>> -\t\tdevice_link_del(link[i]);\n>>> -\n>>> +err:\n>>> \treturn ret;\n>>> }\n>>> \n>>> static void ks_pcie_remove(struct platform_device *pdev)\n>>> {\n>>> \tstruct keystone_pcie *ks_pcie = platform_get_drvdata(pdev);\n>>> -\tstruct device_link **link = ks_pcie->link;\n>>> -\tint num_lanes = ks_pcie->num_lanes;\n>>> \tstruct device *dev = &pdev->dev;\n>>> \n>>> \tpm_runtime_put(dev);\n>>> \tpm_runtime_disable(dev);\n>>\n>> Does calling pm_runtime_put() immediately before pm_runtime_disable() cancel\n>> the suspend transition?\n>>\n>> pm_runtime_put() queues an asynchronous idle or suspend work item, but\n>> calling pm_runtime_disable() immediately afterward cancels this pending\n>> transition via __pm_runtime_barrier().\n>>\n>> Could this leave the hardware in an active or incorrect power state from\n>> the controller's perspective? Would it be safer to use synchronous\n>> pm_runtime_put_sync() instead?\n>>\n>> This asynchronous put pattern also appears in the error paths of\n>> dra7xx_pcie_probe() and ks_pcie_probe().\n>>\n>> While reviewing the power management handling in these drivers, I also\n>> noticed that the shared interrupt handlers read hardware registers directly\n>> without verifying if the device is active:\n>>\n>> drivers/pci/controller/dwc/pci-dra7xx.c:dra7xx_pcie_irq_handler() {\n>> \t...\n>> \treg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN);\n>>\n>> If another device sharing the same IRQ line triggers an interrupt while the\n>> PCIe controller is runtime suspended, will these handlers attempt to read\n>> powered-down hardware?\n>>\n>> Should the handlers check the PM runtime state using\n>> pm_runtime_get_if_active() to prevent spurious interrupts or bus errors?\n>>\n>> -- \n>> Sashiko AI review · https://sashiko.dev/#/patchset/20260505100523.1922388-1-vladimir.oltean@nxp.com?part=9\n>","headers":{"Return-Path":"\n ","X-Original-To":["incoming@patchwork.ozlabs.org","linux-pci@vger.kernel.org"],"Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (1024-bit key;\n unprotected) header.d=163.com header.i=@163.com header.a=rsa-sha256\n header.s=s110527 header.b=L+2WhAK4;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2600:3c0a:e001:db::12fc:5321; helo=sea.lore.kernel.org;\n envelope-from=linux-pci+bounces-54151-incoming=patchwork.ozlabs.org@vger.kernel.org;\n receiver=patchwork.ozlabs.org)","smtp.subspace.kernel.org;\n\tdkim=pass (1024-bit key) header.d=163.com header.i=@163.com\n header.b=\"L+2WhAK4\"","smtp.subspace.kernel.org;\n arc=none smtp.client-ip=117.135.210.2","smtp.subspace.kernel.org;\n dmarc=pass (p=none dis=none) header.from=163.com","smtp.subspace.kernel.org;\n spf=pass smtp.mailfrom=163.com"],"Received":["from sea.lore.kernel.org (sea.lore.kernel.org\n [IPv6:2600:3c0a:e001:db::12fc:5321])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange x25519)\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4gBXlV10Tcz1yCg\n\tfor ; Fri, 08 May 2026 12:15:34 +1000 (AEST)","from smtp.subspace.kernel.org (conduit.subspace.kernel.org\n [100.90.174.1])\n\tby sea.lore.kernel.org (Postfix) with ESMTP id 94EF0301CCDD\n\tfor ; Fri, 8 May 2026 02:15:31 +0000 (UTC)","from localhost.localdomain (localhost.localdomain [127.0.0.1])\n\tby smtp.subspace.kernel.org (Postfix) with ESMTP id 1478315E8B;\n\tFri, 8 May 2026 02:15:31 +0000 (UTC)","from m16.mail.163.com (m16.mail.163.com [117.135.210.2])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby smtp.subspace.kernel.org (Postfix) with ESMTPS id 7A2F633985\n\tfor ; Fri, 8 May 2026 02:15:27 +0000 (UTC)","from [192.168.50.71] (unknown [])\n\tby gzsmtp5 (Coremail) with SMTP id QCgvCgBHl1UNR_1pTo67Cw--.165S2;\n\tFri, 08 May 2026 10:14:39 +0800 (CST)"],"ARC-Seal":"i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;\n\tt=1778206531; cv=none;\n b=flo9eyd9RKHXETehoBI4WD9Vr3nPL3KcSyViaXCwTfIr261WZov+LdCvAYcLmEQdUyCQwbhqnygyGr2sRHBFf6XhEpYOSAbhxZKMCROm3eLf/XLOdOzH5xhJYRct+BOXpBst56IY/1pHyB/bhlO3G0DsskH2Iar0ZQR3Xbc+pTQ=","ARC-Message-Signature":"i=1; a=rsa-sha256; d=subspace.kernel.org;\n\ts=arc-20240116; t=1778206531; c=relaxed/simple;\n\tbh=pi/8l4elz07UIxfTXrFRxZpLe27XTs6NHSQOWSxu+1U=;\n\th=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:\n\t In-Reply-To:Content-Type;\n b=HQcXg9z/6Nb+M43zfL6FgGqEy1fpIrJH+jeNiscOrM/hirHaqmY0r2eJjfFZXYjpw2kQa7PrwFLj/amdjUm7eY/47ZwjUqHEoLsV1g8Ns6G5y2f64sZzNqCPavxk+df9x8KcZcK2fM8+5zo1LEGsYHmsFQi/SnwrJOIXFadTyjY=","ARC-Authentication-Results":"i=1; smtp.subspace.kernel.org;\n dmarc=pass (p=none dis=none) header.from=163.com;\n spf=pass smtp.mailfrom=163.com;\n dkim=pass (1024-bit key) header.d=163.com header.i=@163.com\n header.b=L+2WhAK4; arc=none smtp.client-ip=117.135.210.2","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=163.com;\n\ts=s110527; h=Message-ID:Date:MIME-Version:Subject:To:From:\n\tContent-Type; bh=BKNcxBNF4k4qY7sWRJhXpvWPI7DG+pSATa9C23PkKHg=;\n\tb=L+2WhAK45tZdGxK4g2XqC0neJfZj29yi/6AvEqv/nIb6osnOt3ogQ9tR75fvoC\n\tn63xJnVvFnouGu0G68W1cXfg8qkYKWbM8nvrnQe0MVHig545/+p/d5Sq57vEKQr6\n\txUDSMX4/W+eG16w3+Yk2439rkt59mibE24mOB4Uq09YUQ=","Message-ID":"<938ad958-f057-4909-8547-80b95f224864@163.com>","Date":"Fri, 8 May 2026 10:14:36 +0800","Precedence":"bulk","X-Mailing-List":"linux-pci@vger.kernel.org","List-Id":"","List-Subscribe":"","List-Unsubscribe":"","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v8 phy-next 09/31] PCI: Remove device links to PHY","To":"Bjorn Helgaas ,\n Manikandan K Pillai ,\n Siddharth Vadapalli ,\n Vignesh Raghavendra , Hans Zhang ","Cc":"Vladimir Oltean ,\n wsa+renesas@sang-engineering.com, linux-pci@vger.kernel.org","References":"<20260507154744.GA971447@bhelgaas>","Content-Language":"en-US","From":"Hans Zhang <18255117159@163.com>","In-Reply-To":"<20260507154744.GA971447@bhelgaas>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-CM-TRANSID":"QCgvCgBHl1UNR_1pTo67Cw--.165S2","X-Coremail-Antispam":"1Uf129KBjvJXoW3JFyktF4xJw1UZF18GFWxJFb_yoW7KF45pF\n\tWDXas2kF48WF4jv3Z293WUZFyftF9ay347Jwnak3WS9r1Y9r98JFy7JFyF93ZakrWkZw12\n\tvw15twnrZFsYvFUanT9S1TB71UUUUU7qnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2\n\t9KBjDUYxBIdaVFxhVjvjDU0xZFpf9x07UxkuxUUUUU=","X-CM-SenderInfo":"rpryjkyvrrlimvzbiqqrwthudrp/xtbCww+uUmn9Rw-J3QAA34"}}]