[Linux-stm32] [PATCH] net: dwmac-rk: MAC clock should be truned off

[lizhe]

Hi, Andrew




 It looks pretty deliberate ignoring errors




 If that's the case, then these lines of code:




    if (ret) {

gmac_clk_enable(bsp_priv, false);

return ret;

   }




Will never get executed. I will send another patch to fix this issue
If the PHY power-on fails, then in the MAC-PHY connected configuration, the network interface cannot operate properly.

BUT, the MAC clock remains enabled and is not turned off, which is incorrec














At 2025-05-24 22:48:15, "Andrew Lunn" <andrew at lunn.ch> wrote:
>On Sat, May 24, 2025 at 10:05:47PM +0800, lizhe wrote:
>> Hi， Anerdw
>> The following is the logic for calling this function： 
>> 
>> 
>> rk_gmac_powerup() {
>> 
>> ret = phy_power_on(bsp_priv, true);      // here.
>> 
>> if (ret) {
>> 
>> gmac_clk_enable(bsp_priv, false);
>> 
>> return ret;
>> 
>> }
>> 
>> }
>
>Ah, there is something funny with your patch. Look at the diff:
>
>diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>index 700858ff6f7c..036e45be5828 100644
>--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>@@ -1648,7 +1648,7 @@  static int gmac_clk_enable(struct rk_priv_data *bsp_priv, bool enable)
>
>This line tells you where in the file you are patching, and the
>function to be patched. This is what i looked at,
>gmac_clk_enable(). And gmac_clk_enable() has a similar structure, ret
>declared at the beginning, return 0 at the end. But the only way to
>that return 0 is without error.
>
>But patch is actually for:
>
> static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
> {
> 	struct regulator *ldo = bsp_priv->regulator;
>-	int ret;
>+	int ret = 0;
> 	struct device *dev = &bsp_priv->pdev->dev;
> 
> 	if (enable) {
>@@ -1661,7 +1661,7 @@  static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
> 			dev_err(dev, "fail to disable phy-supply\n");
> 	}
> 
>-	return 0;
>+	return ret;
> }
>
>And agree, the error codes are ignored in phy_power_on().
>
>But i have a few questions:
>
>How did you generate this diff? This is the first time i've made this
>mistake, as far as i know. I trust the context information when
>reviewing patches. Yet here it is wrong. Why? Is this actually normal?
>I know diff gets it wrong for python, i don't trust it at all with
>that language, but i've not noticed such problems with C.
>
>Did you look at the history of phy_power_on()? It looks pretty
>deliberate ignoring errors. Maybe there is a reason why this happens?
>git blame and git log might explain why it is like this.
>
>	Andrew
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://st-md-mailman.stormreply.com/pipermail/linux-stm32/attachments/20250525/44204e25/attachment.html>