From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 74C3E2C80 for ; Wed, 26 Jan 2022 17:38:43 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 88173C340E3; Wed, 26 Jan 2022 17:38:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1643218723; bh=EFdscSLnaWLhYVWNI1QN+bbNVEh+ATXkGkbBTAYxJVU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=QKMFT5JIOjKA4WSdOxxZoprP9S5gopg2K0wTG2r2HrnH2fsvbccXZe4hjNxu0SYAp jIB1k3adqafrenS5Z1NabRPOFMZusn6sg3UQK4nhtpDtH9jBVmxDLsEHSdiptXmV62 li8OruZAgJdla3Dhv4Lzvp0EG3z9qgP1lx2cyEINGkFpaIXjW1la+H3FojZRBG5Xtk vcE4nEGqnx9HFaG2qUCjcDKWDIKa+Wiu6k28OK8BKoFR4VhE4X5AF2jxxaXNOBsV0N JSmpjuPNfitBM3tsSB/2PNgtRaqaryeZi+ma42g9YcyiBkLZiLp7N6TfD9qVdkJJhO +2H1WnvFufsbw== Date: Thu, 27 Jan 2022 01:31:05 +0800 From: Jisheng Zhang To: Samuel Holland , Joakim Zhang Cc: Giuseppe Cavallaro , Alexandre Torgue , Jose Abreu , "David S . Miller" , Jakub Kicinski , Maxime Ripard , Chen-Yu Tsai , Jernej Skrabec , Maxime Coquelin , netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linux-stm32@st-md-mailman.stormreply.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] net: stmmac: dwmac-sun8i: fix double disable and unprepare "stmmaceth" clk Message-ID: References: <20220123132805.758-1-jszhang@kernel.org> <38c41c04-abde-4d55-ed7c-515b6bba9c54@sholland.org> Precedence: bulk X-Mailing-List: linux-sunxi@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: On Thu, Jan 27, 2022 at 12:00:42AM +0800, Jisheng Zhang wrote: > On Wed, Jan 26, 2022 at 11:12:28PM +0800, Jisheng Zhang wrote: > > On Sun, Jan 23, 2022 at 01:43:37PM -0600, Samuel Holland wrote: > > > On 1/23/22 7:28 AM, Jisheng Zhang wrote: > > > > Fix warnings on Allwinner D1 platform: > > > > > > > > [ 1.604695] ------------[ cut here ]------------ > > > > [ 1.609328] bus-emac already disabled > > > > [ 1.613015] WARNING: CPU: 0 PID: 38 at drivers/clk/clk.c:952 clk_core_disable+0xcc/0xec > > > > [ 1.621039] CPU: 0 PID: 38 Comm: kworker/u2:1 Not tainted 5.14.0-rc4#1 > > > > [ 1.627653] Hardware name: Allwinner D1 NeZha (DT) > > > > [ 1.632443] Workqueue: events_unbound deferred_probe_work_func > > > > [ 1.638286] epc : clk_core_disable+0xcc/0xec > > > > [ 1.642561] ra : clk_core_disable+0xcc/0xec > > > > [ 1.646835] epc : ffffffff8023c2ec ra : ffffffff8023c2ec sp : ffffffd00411bb10 > > > > [ 1.654054] gp : ffffffff80ec9988 tp : ffffffe00143a800 t0 : ffffffff80ed6a6f > > > > [ 1.661272] t1 : ffffffff80ed6a60 t2 : 0000000000000000 s0 : ffffffe001509e00 > > > > [ 1.668489] s1 : 0000000000000001 a0 : 0000000000000019 a1 : ffffffff80e80bd8 > > > > [ 1.675707] a2 : 00000000ffffefff a3 : 00000000000000f4 a4 : 0000000000000002 > > > > [ 1.682924] a5 : 0000000000000001 a6 : 0000000000000030 a7 : 00000000028f5c29 > > > > [ 1.690141] s2 : 0000000000000800 s3 : ffffffe001375000 s4 : ffffffe01fdf7a80 > > > > [ 1.697358] s5 : ffffffe001375010 s6 : ffffffff8001fc10 s7 : ffffffffffffffff > > > > [ 1.704577] s8 : 0000000000000001 s9 : ffffffff80ecb248 s10: ffffffe001b80000 > > > > [ 1.711794] s11: ffffffe001b80760 t3 : 0000000000000062 t4 : ffffffffffffffff > > > > [ 1.719012] t5 : ffffffff80e0f6d8 t6 : ffffffd00411b8f0 > > > > [ 1.724321] status: 8000000201800100 badaddr: 0000000000000000 cause: 0000000000000003 > > > > [ 1.732233] [] clk_core_disable+0xcc/0xec > > > > [ 1.737810] [] clk_disable+0x38/0x78 > > > > [ 1.742956] [] worker_thread+0x1a8/0x4d8 > > > > [ 1.748451] [] stmmac_remove_config_dt+0x1c/0x4c > > > > [ 1.754646] [] sun8i_dwmac_probe+0x378/0x82c > > > > [ 1.760484] [] worker_thread+0x1a8/0x4d8 > > > > [ 1.765975] [] platform_probe+0x64/0xf0 > > > > [ 1.771382] [] really_probe.part.0+0x8c/0x30c > > > > [ 1.777305] [] __driver_probe_device+0xa0/0x148 > > > > [ 1.783402] [] driver_probe_device+0x38/0x138 > > > > [ 1.789324] [] __device_attach_driver+0xd0/0x170 > > > > [ 1.795508] [] __driver_attach_async_helper+0xbc/0xc0 > > > > [ 1.802125] [] bus_for_each_drv+0x68/0xb4 > > > > [ 1.807701] [] __device_attach+0xd8/0x184 > > > > [ 1.813277] [] bus_probe_device+0x98/0xbc > > > > [ 1.818852] [] deferred_probe_work_func+0x90/0xd4 > > > > [ 1.825122] [] process_one_work+0x1e4/0x390 > > > > [ 1.830872] [] worker_thread+0x31c/0x4d8 > > > > [ 1.836362] [] kthreadd+0x94/0x188 > > > > [ 1.841335] [] kthreadd+0x94/0x188 > > > > [ 1.846304] [] process_one_work+0x38c/0x390 > > > > [ 1.852054] [] kthread+0x124/0x160 > > > > [ 1.857021] [] set_kthread_struct+0x5c/0x60 > > > > [ 1.862770] [] ret_from_syscall_rejected+0x8/0xc > > > > [ 1.868956] ---[ end trace 8d5c6046255f84a0 ]--- > > > > [ 1.873675] ------------[ cut here ]------------ > > > > [ 1.878366] bus-emac already unprepared > > > > [ 1.882378] WARNING: CPU: 0 PID: 38 at drivers/clk/clk.c:810 clk_core_unprepare+0xe4/0x168 > > > > [ 1.890673] CPU: 0 PID: 38 Comm: kworker/u2:1 Tainted: G W 5.14.0-rc4 #1 > > > > [ 1.898674] Hardware name: Allwinner D1 NeZha (DT) > > > > [ 1.903464] Workqueue: events_unbound deferred_probe_work_func > > > > [ 1.909305] epc : clk_core_unprepare+0xe4/0x168 > > > > [ 1.913840] ra : clk_core_unprepare+0xe4/0x168 > > > > [ 1.918375] epc : ffffffff8023d6cc ra : ffffffff8023d6cc sp : ffffffd00411bb10 > > > > [ 1.925593] gp : ffffffff80ec9988 tp : ffffffe00143a800 t0 : 0000000000000002 > > > > [ 1.932811] t1 : ffffffe01f743be0 t2 : 0000000000000040 s0 : ffffffe001509e00 > > > > [ 1.940029] s1 : 0000000000000001 a0 : 000000000000001b a1 : ffffffe00143a800 > > > > [ 1.947246] a2 : 0000000000000000 a3 : 00000000000000f4 a4 : 0000000000000001 > > > > [ 1.954463] a5 : 0000000000000000 a6 : 0000000005fce2a5 a7 : 0000000000000001 > > > > [ 1.961680] s2 : 0000000000000800 s3 : ffffffff80afeb90 s4 : ffffffe01fdf7a80 > > > > [ 1.968898] s5 : ffffffe001375010 s6 : ffffffff8001fc10 s7 : ffffffffffffffff > > > > [ 1.976115] s8 : 0000000000000001 s9 : ffffffff80ecb248 s10: ffffffe001b80000 > > > > [ 1.983333] s11: ffffffe001b80760 t3 : ffffffff80b39120 t4 : 0000000000000001 > > > > [ 1.990550] t5 : 0000000000000000 t6 : ffffffe001600002 > > > > [ 1.995859] status: 8000000201800120 badaddr: 0000000000000000 cause: 0000000000000003 > > > > [ 2.003771] [] clk_core_unprepare+0xe4/0x168 > > > > [ 2.009609] [] clk_unprepare+0x24/0x3c > > > > [ 2.014929] [] stmmac_remove_config_dt+0x24/0x4c > > > > [ 2.021125] [] sun8i_dwmac_probe+0x378/0x82c > > > > [ 2.026965] [] worker_thread+0x1a8/0x4d8 > > > > [ 2.032463] [] platform_probe+0x64/0xf0 > > > > [ 2.037871] [] really_probe.part.0+0x8c/0x30c > > > > [ 2.043795] [] __driver_probe_device+0xa0/0x148 > > > > [ 2.049892] [] driver_probe_device+0x38/0x138 > > > > [ 2.055815] [] __device_attach_driver+0xd0/0x170 > > > > [ 2.061999] [] __driver_attach_async_helper+0xbc/0xc0 > > > > [ 2.068616] [] bus_for_each_drv+0x68/0xb4 > > > > [ 2.074193] [] __device_attach+0xd8/0x184 > > > > [ 2.079769] [] bus_probe_device+0x98/0xbc > > > > [ 2.085345] [] deferred_probe_work_func+0x90/0xd4 > > > > [ 2.091616] [] process_one_work+0x1e4/0x390 > > > > [ 2.097367] [] worker_thread+0x31c/0x4d8 > > > > [ 2.102858] [] kthreadd+0x94/0x188 > > > > [ 2.107830] [] kthreadd+0x94/0x188 > > > > [ 2.112800] [] process_one_work+0x38c/0x390 > > > > [ 2.118551] [] kthread+0x124/0x160 > > > > [ 2.123520] [] set_kthread_struct+0x5c/0x60 > > > > [ 2.129268] [] ret_from_syscall_rejected+0x8/0xc > > > > [ 2.135455] ---[ end trace 8d5c6046255f84a1 ]--- > > > > > > > > the dwmmac-sun8i driver will get the "stmmaceth" clk as tx_clk during > > > > driver initialization. If stmmac_dvr_probe() fails due to various > > > > reasons, sun8i_dwmac_exit() will disable and unprepare the "stmmaceth" > > > > clk, then stmmac_remove_config_dt() will disable and unprepare the > > > > clk again. > > > > > > This should still be balanced, because both stmmac_probe_config_dt and > > > sun8i_dwmac_init prepare/enable the clock, so the dwmac-sun8i glue layer calls > > > stmmac_dvr_probe with the clock having an enable count of 2. It looks like the > > > underlying issue is that commit 5ec55823438e ("net: stmmac: add clocks > > > management for gmac driver") introduces unbalanced runtime PM. > > > > I added some printk then retested, the problem is triggered as below: > > > > stmmac_probe_config_dt() enable the clk > > sun8i_dwmac_init() enble the clk again > > stmmac_dvr_probe() succeed, but it calls pm_runtime_put(), so rpm will > > disable the clk > > sun8i_dwmac_reset() fails due to various reason > > sun8i_dwmac_exit() disable the clk, this is fine > > stmmac_remove_config_dt() disable the clk again, so ccf complains. > > > > The key here is: whether we should let stmmac_dvr_probe() calls > > pm_runtime_put() or let stmmac users to determine whether we could > > let rpm go? > > If we keep current behavior: stmmac users need to take care > > the code after stmmac_dvr_probe, including error handling code path, > > if we touch access registers, we need to call pm_runtime_get_sync() > > firstly. > > > > Since the commit 5ec55823438e has been in for a long time, I'll submit > > a patch to follow this way. > > > > After reading the code and commit history, I found some users suffering > from the runtimepm issues, for example, dwmac-rk.c, so I changed my > idea. I think it's better to let stmmac users to call pm_runtime_put(): > > First of all, only the users know whether it's safe to finally > keep the mac runtime suspended after probe. > > Secondly, if the users need to do platform specific operations after > stmmac_dvr_probe(), especially needs to access some registers, we have > to resume the mac firstly. The error handling code path also needs to > take care of it. It looks a bit strange resume immediately resume the > mac after suspending it. > > Any suggestion is welcome. > > PS: Since this would partially revert commit 5ec55823438e, I add Joakim > into the email thread. > If we keep current behavior as is, and let stmmac users to take care necessary runtime resume before specific handling, we need similar patch as dwmac-sun8i below for all stmmac users, especially the error handling path: diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c index 09644ab0d87a..ef2acea3d036 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -1254,6 +1255,7 @@ static int sun8i_dwmac_probe(struct platform_device *pdev) ndev = dev_get_drvdata(&pdev->dev); priv = netdev_priv(ndev); + pm_runtime_get_sync(&pdev->dev); /* The mux must be registered after parent MDIO * so after stmmac_dvr_probe() */ @@ -1272,12 +1274,15 @@ static int sun8i_dwmac_probe(struct platform_device *pdev) goto dwmac_remove; } + pm_runtime_put(&pdev->dev); + return 0; dwmac_mux: reset_control_put(gmac->rst_ephy); clk_put(gmac->ephy_clk); dwmac_remove: + pm_runtime_put_noidle(&pdev->dev); stmmac_dvr_remove(&pdev->dev); dwmac_exit: sun8i_dwmac_exit(pdev, gmac); diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 8ded4be08b00..2f9bd1eacb81 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -7133,6 +7133,10 @@ int stmmac_dvr_remove(struct device *dev) netdev_info(priv->dev, "%s: removing driver", __func__); + pm_runtime_get_sync(dev); + pm_runtime_disable(dev); + pm_runtime_put_noidle(dev); + stmmac_stop_all_dma(priv); stmmac_mac_set(priv, priv->ioaddr, false); netif_carrier_off(ndev); @@ -7151,8 +7155,6 @@ int stmmac_dvr_remove(struct device *dev) if (priv->plat->stmmac_rst) reset_control_assert(priv->plat->stmmac_rst); reset_control_assert(priv->plat->stmmac_ahb_rst); - pm_runtime_put(dev); - pm_runtime_disable(dev); if (priv->hw->pcs != STMMAC_PCS_TBI && priv->hw->pcs != STMMAC_PCS_RTBI) stmmac_mdio_unregister(ndev); 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 C4B64C28CF5 for ; Wed, 26 Jan 2022 18:00:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=jxlFWyrD6qxim3ZNR/xF+k4QWK+CP5R04UXIQhK/Wf0=; b=MRrygA3zmq802j PkQTCjBQ9z2hPh9m1j9akHEgxSVBsqIZb6xUgo/hQ2DblSNs9iNEj4h6qThPcNFMMnjK12UNLMRdk Q1swPAFQ7cm48NJfvo6HDn3XSHO+Z8kgZYV/xfvvOiMts+ofhuZUCZW39SD9eUrXGM3NZ4rhKyZWy yQsAJgBNIipO8AGQJjHXzoPfDCdz0D0aWw98+Eo1w925bOoAG9OCCkSHg66bA+PizajRImNIrXn/B janLWnNeuVrQf+i2Ca22RcDPG4j6FaHqcMNgiY5FdW8epC0kOGOOWqhRvLG6c41yDaBoQjSQDNkfL JYrf8j6spc8HlHJdKBog==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nCmYs-00DB2y-Jg; Wed, 26 Jan 2022 17:58:27 +0000 Received: from ams.source.kernel.org ([2604:1380:4601:e00::1]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nCmFp-00D3uu-Mv for linux-arm-kernel@lists.infradead.org; Wed, 26 Jan 2022 17:38:48 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 52BA4B81ECB; Wed, 26 Jan 2022 17:38:44 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 88173C340E3; Wed, 26 Jan 2022 17:38:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1643218723; bh=EFdscSLnaWLhYVWNI1QN+bbNVEh+ATXkGkbBTAYxJVU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=QKMFT5JIOjKA4WSdOxxZoprP9S5gopg2K0wTG2r2HrnH2fsvbccXZe4hjNxu0SYAp jIB1k3adqafrenS5Z1NabRPOFMZusn6sg3UQK4nhtpDtH9jBVmxDLsEHSdiptXmV62 li8OruZAgJdla3Dhv4Lzvp0EG3z9qgP1lx2cyEINGkFpaIXjW1la+H3FojZRBG5Xtk vcE4nEGqnx9HFaG2qUCjcDKWDIKa+Wiu6k28OK8BKoFR4VhE4X5AF2jxxaXNOBsV0N JSmpjuPNfitBM3tsSB/2PNgtRaqaryeZi+ma42g9YcyiBkLZiLp7N6TfD9qVdkJJhO +2H1WnvFufsbw== Date: Thu, 27 Jan 2022 01:31:05 +0800 From: Jisheng Zhang To: Samuel Holland , Joakim Zhang Cc: Giuseppe Cavallaro , Alexandre Torgue , Jose Abreu , "David S . Miller" , Jakub Kicinski , Maxime Ripard , Chen-Yu Tsai , Jernej Skrabec , Maxime Coquelin , netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linux-stm32@st-md-mailman.stormreply.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] net: stmmac: dwmac-sun8i: fix double disable and unprepare "stmmaceth" clk Message-ID: References: <20220123132805.758-1-jszhang@kernel.org> <38c41c04-abde-4d55-ed7c-515b6bba9c54@sholland.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220126_093846_096800_E8D84DB7 X-CRM114-Status: GOOD ( 33.99 ) 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: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Jan 27, 2022 at 12:00:42AM +0800, Jisheng Zhang wrote: > On Wed, Jan 26, 2022 at 11:12:28PM +0800, Jisheng Zhang wrote: > > On Sun, Jan 23, 2022 at 01:43:37PM -0600, Samuel Holland wrote: > > > On 1/23/22 7:28 AM, Jisheng Zhang wrote: > > > > Fix warnings on Allwinner D1 platform: > > > > > > > > [ 1.604695] ------------[ cut here ]------------ > > > > [ 1.609328] bus-emac already disabled > > > > [ 1.613015] WARNING: CPU: 0 PID: 38 at drivers/clk/clk.c:952 clk_core_disable+0xcc/0xec > > > > [ 1.621039] CPU: 0 PID: 38 Comm: kworker/u2:1 Not tainted 5.14.0-rc4#1 > > > > [ 1.627653] Hardware name: Allwinner D1 NeZha (DT) > > > > [ 1.632443] Workqueue: events_unbound deferred_probe_work_func > > > > [ 1.638286] epc : clk_core_disable+0xcc/0xec > > > > [ 1.642561] ra : clk_core_disable+0xcc/0xec > > > > [ 1.646835] epc : ffffffff8023c2ec ra : ffffffff8023c2ec sp : ffffffd00411bb10 > > > > [ 1.654054] gp : ffffffff80ec9988 tp : ffffffe00143a800 t0 : ffffffff80ed6a6f > > > > [ 1.661272] t1 : ffffffff80ed6a60 t2 : 0000000000000000 s0 : ffffffe001509e00 > > > > [ 1.668489] s1 : 0000000000000001 a0 : 0000000000000019 a1 : ffffffff80e80bd8 > > > > [ 1.675707] a2 : 00000000ffffefff a3 : 00000000000000f4 a4 : 0000000000000002 > > > > [ 1.682924] a5 : 0000000000000001 a6 : 0000000000000030 a7 : 00000000028f5c29 > > > > [ 1.690141] s2 : 0000000000000800 s3 : ffffffe001375000 s4 : ffffffe01fdf7a80 > > > > [ 1.697358] s5 : ffffffe001375010 s6 : ffffffff8001fc10 s7 : ffffffffffffffff > > > > [ 1.704577] s8 : 0000000000000001 s9 : ffffffff80ecb248 s10: ffffffe001b80000 > > > > [ 1.711794] s11: ffffffe001b80760 t3 : 0000000000000062 t4 : ffffffffffffffff > > > > [ 1.719012] t5 : ffffffff80e0f6d8 t6 : ffffffd00411b8f0 > > > > [ 1.724321] status: 8000000201800100 badaddr: 0000000000000000 cause: 0000000000000003 > > > > [ 1.732233] [] clk_core_disable+0xcc/0xec > > > > [ 1.737810] [] clk_disable+0x38/0x78 > > > > [ 1.742956] [] worker_thread+0x1a8/0x4d8 > > > > [ 1.748451] [] stmmac_remove_config_dt+0x1c/0x4c > > > > [ 1.754646] [] sun8i_dwmac_probe+0x378/0x82c > > > > [ 1.760484] [] worker_thread+0x1a8/0x4d8 > > > > [ 1.765975] [] platform_probe+0x64/0xf0 > > > > [ 1.771382] [] really_probe.part.0+0x8c/0x30c > > > > [ 1.777305] [] __driver_probe_device+0xa0/0x148 > > > > [ 1.783402] [] driver_probe_device+0x38/0x138 > > > > [ 1.789324] [] __device_attach_driver+0xd0/0x170 > > > > [ 1.795508] [] __driver_attach_async_helper+0xbc/0xc0 > > > > [ 1.802125] [] bus_for_each_drv+0x68/0xb4 > > > > [ 1.807701] [] __device_attach+0xd8/0x184 > > > > [ 1.813277] [] bus_probe_device+0x98/0xbc > > > > [ 1.818852] [] deferred_probe_work_func+0x90/0xd4 > > > > [ 1.825122] [] process_one_work+0x1e4/0x390 > > > > [ 1.830872] [] worker_thread+0x31c/0x4d8 > > > > [ 1.836362] [] kthreadd+0x94/0x188 > > > > [ 1.841335] [] kthreadd+0x94/0x188 > > > > [ 1.846304] [] process_one_work+0x38c/0x390 > > > > [ 1.852054] [] kthread+0x124/0x160 > > > > [ 1.857021] [] set_kthread_struct+0x5c/0x60 > > > > [ 1.862770] [] ret_from_syscall_rejected+0x8/0xc > > > > [ 1.868956] ---[ end trace 8d5c6046255f84a0 ]--- > > > > [ 1.873675] ------------[ cut here ]------------ > > > > [ 1.878366] bus-emac already unprepared > > > > [ 1.882378] WARNING: CPU: 0 PID: 38 at drivers/clk/clk.c:810 clk_core_unprepare+0xe4/0x168 > > > > [ 1.890673] CPU: 0 PID: 38 Comm: kworker/u2:1 Tainted: G W 5.14.0-rc4 #1 > > > > [ 1.898674] Hardware name: Allwinner D1 NeZha (DT) > > > > [ 1.903464] Workqueue: events_unbound deferred_probe_work_func > > > > [ 1.909305] epc : clk_core_unprepare+0xe4/0x168 > > > > [ 1.913840] ra : clk_core_unprepare+0xe4/0x168 > > > > [ 1.918375] epc : ffffffff8023d6cc ra : ffffffff8023d6cc sp : ffffffd00411bb10 > > > > [ 1.925593] gp : ffffffff80ec9988 tp : ffffffe00143a800 t0 : 0000000000000002 > > > > [ 1.932811] t1 : ffffffe01f743be0 t2 : 0000000000000040 s0 : ffffffe001509e00 > > > > [ 1.940029] s1 : 0000000000000001 a0 : 000000000000001b a1 : ffffffe00143a800 > > > > [ 1.947246] a2 : 0000000000000000 a3 : 00000000000000f4 a4 : 0000000000000001 > > > > [ 1.954463] a5 : 0000000000000000 a6 : 0000000005fce2a5 a7 : 0000000000000001 > > > > [ 1.961680] s2 : 0000000000000800 s3 : ffffffff80afeb90 s4 : ffffffe01fdf7a80 > > > > [ 1.968898] s5 : ffffffe001375010 s6 : ffffffff8001fc10 s7 : ffffffffffffffff > > > > [ 1.976115] s8 : 0000000000000001 s9 : ffffffff80ecb248 s10: ffffffe001b80000 > > > > [ 1.983333] s11: ffffffe001b80760 t3 : ffffffff80b39120 t4 : 0000000000000001 > > > > [ 1.990550] t5 : 0000000000000000 t6 : ffffffe001600002 > > > > [ 1.995859] status: 8000000201800120 badaddr: 0000000000000000 cause: 0000000000000003 > > > > [ 2.003771] [] clk_core_unprepare+0xe4/0x168 > > > > [ 2.009609] [] clk_unprepare+0x24/0x3c > > > > [ 2.014929] [] stmmac_remove_config_dt+0x24/0x4c > > > > [ 2.021125] [] sun8i_dwmac_probe+0x378/0x82c > > > > [ 2.026965] [] worker_thread+0x1a8/0x4d8 > > > > [ 2.032463] [] platform_probe+0x64/0xf0 > > > > [ 2.037871] [] really_probe.part.0+0x8c/0x30c > > > > [ 2.043795] [] __driver_probe_device+0xa0/0x148 > > > > [ 2.049892] [] driver_probe_device+0x38/0x138 > > > > [ 2.055815] [] __device_attach_driver+0xd0/0x170 > > > > [ 2.061999] [] __driver_attach_async_helper+0xbc/0xc0 > > > > [ 2.068616] [] bus_for_each_drv+0x68/0xb4 > > > > [ 2.074193] [] __device_attach+0xd8/0x184 > > > > [ 2.079769] [] bus_probe_device+0x98/0xbc > > > > [ 2.085345] [] deferred_probe_work_func+0x90/0xd4 > > > > [ 2.091616] [] process_one_work+0x1e4/0x390 > > > > [ 2.097367] [] worker_thread+0x31c/0x4d8 > > > > [ 2.102858] [] kthreadd+0x94/0x188 > > > > [ 2.107830] [] kthreadd+0x94/0x188 > > > > [ 2.112800] [] process_one_work+0x38c/0x390 > > > > [ 2.118551] [] kthread+0x124/0x160 > > > > [ 2.123520] [] set_kthread_struct+0x5c/0x60 > > > > [ 2.129268] [] ret_from_syscall_rejected+0x8/0xc > > > > [ 2.135455] ---[ end trace 8d5c6046255f84a1 ]--- > > > > > > > > the dwmmac-sun8i driver will get the "stmmaceth" clk as tx_clk during > > > > driver initialization. If stmmac_dvr_probe() fails due to various > > > > reasons, sun8i_dwmac_exit() will disable and unprepare the "stmmaceth" > > > > clk, then stmmac_remove_config_dt() will disable and unprepare the > > > > clk again. > > > > > > This should still be balanced, because both stmmac_probe_config_dt and > > > sun8i_dwmac_init prepare/enable the clock, so the dwmac-sun8i glue layer calls > > > stmmac_dvr_probe with the clock having an enable count of 2. It looks like the > > > underlying issue is that commit 5ec55823438e ("net: stmmac: add clocks > > > management for gmac driver") introduces unbalanced runtime PM. > > > > I added some printk then retested, the problem is triggered as below: > > > > stmmac_probe_config_dt() enable the clk > > sun8i_dwmac_init() enble the clk again > > stmmac_dvr_probe() succeed, but it calls pm_runtime_put(), so rpm will > > disable the clk > > sun8i_dwmac_reset() fails due to various reason > > sun8i_dwmac_exit() disable the clk, this is fine > > stmmac_remove_config_dt() disable the clk again, so ccf complains. > > > > The key here is: whether we should let stmmac_dvr_probe() calls > > pm_runtime_put() or let stmmac users to determine whether we could > > let rpm go? > > If we keep current behavior: stmmac users need to take care > > the code after stmmac_dvr_probe, including error handling code path, > > if we touch access registers, we need to call pm_runtime_get_sync() > > firstly. > > > > Since the commit 5ec55823438e has been in for a long time, I'll submit > > a patch to follow this way. > > > > After reading the code and commit history, I found some users suffering > from the runtimepm issues, for example, dwmac-rk.c, so I changed my > idea. I think it's better to let stmmac users to call pm_runtime_put(): > > First of all, only the users know whether it's safe to finally > keep the mac runtime suspended after probe. > > Secondly, if the users need to do platform specific operations after > stmmac_dvr_probe(), especially needs to access some registers, we have > to resume the mac firstly. The error handling code path also needs to > take care of it. It looks a bit strange resume immediately resume the > mac after suspending it. > > Any suggestion is welcome. > > PS: Since this would partially revert commit 5ec55823438e, I add Joakim > into the email thread. > If we keep current behavior as is, and let stmmac users to take care necessary runtime resume before specific handling, we need similar patch as dwmac-sun8i below for all stmmac users, especially the error handling path: diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c index 09644ab0d87a..ef2acea3d036 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -1254,6 +1255,7 @@ static int sun8i_dwmac_probe(struct platform_device *pdev) ndev = dev_get_drvdata(&pdev->dev); priv = netdev_priv(ndev); + pm_runtime_get_sync(&pdev->dev); /* The mux must be registered after parent MDIO * so after stmmac_dvr_probe() */ @@ -1272,12 +1274,15 @@ static int sun8i_dwmac_probe(struct platform_device *pdev) goto dwmac_remove; } + pm_runtime_put(&pdev->dev); + return 0; dwmac_mux: reset_control_put(gmac->rst_ephy); clk_put(gmac->ephy_clk); dwmac_remove: + pm_runtime_put_noidle(&pdev->dev); stmmac_dvr_remove(&pdev->dev); dwmac_exit: sun8i_dwmac_exit(pdev, gmac); diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 8ded4be08b00..2f9bd1eacb81 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -7133,6 +7133,10 @@ int stmmac_dvr_remove(struct device *dev) netdev_info(priv->dev, "%s: removing driver", __func__); + pm_runtime_get_sync(dev); + pm_runtime_disable(dev); + pm_runtime_put_noidle(dev); + stmmac_stop_all_dma(priv); stmmac_mac_set(priv, priv->ioaddr, false); netif_carrier_off(ndev); @@ -7151,8 +7155,6 @@ int stmmac_dvr_remove(struct device *dev) if (priv->plat->stmmac_rst) reset_control_assert(priv->plat->stmmac_rst); reset_control_assert(priv->plat->stmmac_ahb_rst); - pm_runtime_put(dev); - pm_runtime_disable(dev); if (priv->hw->pcs != STMMAC_PCS_TBI && priv->hw->pcs != STMMAC_PCS_RTBI) stmmac_mdio_unregister(ndev); _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel