[PATCH] driver: ethernet: stmmac: remove the redundant clock disable action

[PATCH] driver: ethernet: stmmac: remove the redundant clock disable action

[Meng Li]

When run below command to remove ethernet driver on
stratix10 platform, there will be warning trace as below:

$ cd /sys/class/net/etha01/device/driver
$ echo ff800000.ethernet > unbind

WARNING: CPU: 3 PID: 386 at drivers/clk/clk.c:810 clk_core_unprepare+0x114/0x274
Modules linked in: sch_fq_codel
CPU: 3 PID: 386 Comm: sh Tainted: G        W         5.10.74-yocto-standard #1
Hardware name: SoCFPGA Stratix 10 SoCDK (DT)
pstate: 00000005 (nzcv daif -PAN -UAO -TCO BTYPE=--)
pc : clk_core_unprepare+0x114/0x274
lr : clk_core_unprepare+0x114/0x274
sp : ffff800011bdbb10
clk_core_unprepare+0x114/0x274
 clk_unprepare+0x38/0x50
 stmmac_remove_config_dt+0x40/0x80
 stmmac_pltfr_remove+0x64/0x80
 platform_drv_remove+0x38/0x60
 ... ..
 el0_sync_handler+0x1a4/0x1b0
 el0_sync+0x180/0x1c0
This issue is introduced by introducing upstream commit 8f269102baf7
("net: stmmac: disable clocks in stmmac_remove_config_dt()")
Because clock has been disabled in function stmmac_dvr_remove()
It not reasonable the remove clock disable action from function
stmmac_remove_config_dt(), because it is mainly used in probe failed,
and other platform drivers also use this common function. So, remove
stmmac_remove_config_dt() from stmmac_pltfr_remove(), only other
necessary code.

Fixes: 1af3a8e91f1a ("net: stmmac: disable clocks in stmmac_remove_config_dt()")
Signed-off-by: Meng Li <Meng.Li@windriver.com>

---

Some extra comments as below:

1. This patch is only for linux-stable kernel v5.10, so the fixed commit ID is the one
   in linux-stable kernel, not the one in mainline upsteam kernel.

2. I created a patch only to fix the linux-stable kernel v5.10, not submit it to upstream kernel.
   The reason as below:
   In fact, upstream kernel doesn't have this issue any more. Because it has a patch to improve
   the clock management and other 4 patches to fix the 1st patch. Detial patches as below:
   5ec55823438e("net: stmmac: add clocks management for gmac driver")
   30f347ae7cc1("net: stmmac: fix missing unlock on error in stmmac_suspend()")
   b3dcb3127786("net: stmmac: correct clocks enabled in stmmac_vlan_rx_kill_vid()")
   4691ffb18ac9("net: stmmac: fix system hang if change mac address after interface ifdown")
   ab00f3e051e8("net: stmmac: fix issue where clk is being unprepared twice")

   But I think it is a little complex to backport all the 5 patches. Moreover, it may be related
   with other patches and code context mofification.
   Therefore, I create a simple and clear patch to only this issue on linux-stable kernel, v 5.10

---
 drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 53be8fc1d125..0fb702ce2408 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -706,7 +706,8 @@ int stmmac_pltfr_remove(struct platform_device *pdev)
 	if (plat->exit)
 		plat->exit(pdev, plat->bsp_priv);
 
-	stmmac_remove_config_dt(pdev, plat);
+	of_node_put(plat->phy_node);
+	of_node_put(plat->mdio_node);
 
 	return ret;
 }
-- 
2.17.1

Re: [PATCH] driver: ethernet: stmmac: remove the redundant clock disable action

[Greg KH]

On Sat, Nov 06, 2021 at 06:44:01PM +0800, Meng Li wrote:
> When run below command to remove ethernet driver on
> stratix10 platform, there will be warning trace as below:
> 
> $ cd /sys/class/net/etha01/device/driver
> $ echo ff800000.ethernet > unbind
> 
> WARNING: CPU: 3 PID: 386 at drivers/clk/clk.c:810 clk_core_unprepare+0x114/0x274
> Modules linked in: sch_fq_codel
> CPU: 3 PID: 386 Comm: sh Tainted: G        W         5.10.74-yocto-standard #1
> Hardware name: SoCFPGA Stratix 10 SoCDK (DT)
> pstate: 00000005 (nzcv daif -PAN -UAO -TCO BTYPE=--)
> pc : clk_core_unprepare+0x114/0x274
> lr : clk_core_unprepare+0x114/0x274
> sp : ffff800011bdbb10
> clk_core_unprepare+0x114/0x274
>  clk_unprepare+0x38/0x50
>  stmmac_remove_config_dt+0x40/0x80
>  stmmac_pltfr_remove+0x64/0x80
>  platform_drv_remove+0x38/0x60
>  ... ..
>  el0_sync_handler+0x1a4/0x1b0
>  el0_sync+0x180/0x1c0
> This issue is introduced by introducing upstream commit 8f269102baf7
> ("net: stmmac: disable clocks in stmmac_remove_config_dt()")
> Because clock has been disabled in function stmmac_dvr_remove()
> It not reasonable the remove clock disable action from function
> stmmac_remove_config_dt(), because it is mainly used in probe failed,
> and other platform drivers also use this common function. So, remove
> stmmac_remove_config_dt() from stmmac_pltfr_remove(), only other
> necessary code.
> 
> Fixes: 1af3a8e91f1a ("net: stmmac: disable clocks in stmmac_remove_config_dt()")
> Signed-off-by: Meng Li <Meng.Li@windriver.com>
> 
> ---
> 
> Some extra comments as below:
> 
> 1. This patch is only for linux-stable kernel v5.10, so the fixed commit ID is the one
>    in linux-stable kernel, not the one in mainline upsteam kernel.

Ick, why?

> 2. I created a patch only to fix the linux-stable kernel v5.10, not submit it to upstream kernel.
>    The reason as below:
>    In fact, upstream kernel doesn't have this issue any more. Because it has a patch to improve
>    the clock management and other 4 patches to fix the 1st patch. Detial patches as below:
>    5ec55823438e("net: stmmac: add clocks management for gmac driver")
>    30f347ae7cc1("net: stmmac: fix missing unlock on error in stmmac_suspend()")
>    b3dcb3127786("net: stmmac: correct clocks enabled in stmmac_vlan_rx_kill_vid()")
>    4691ffb18ac9("net: stmmac: fix system hang if change mac address after interface ifdown")
>    ab00f3e051e8("net: stmmac: fix issue where clk is being unprepared twice")
> 
>    But I think it is a little complex to backport all the 5 patches. Moreover, it may be related
>    with other patches and code context mofification.
>    Therefore, I create a simple and clear patch to only this issue on linux-stable kernel, v 5.10

We almost ALWAYS want the original patches instead.  When we try to do
stable-only patches, 95% of the time it gets wrong and it makes
backporting future fixes for the same code area impossible.

So please submit the above patches as a series and I will be glad to
consider them.

thanks,

greg k-h