<div data-ntes="ntes_mail_body_root" style="line-height:1.7;color:#000000;font-size:14px;font-family:Arial"><div id="spnEditorContent"><p style="margin: 0;">Hi, Andrew</p><p style="margin: 0;"><br></p><p style="margin: 0;"><b> It looks pretty deliberate ignoring errors</b></p><p style="margin: 0;"><b><br></b></p><p style="margin: 0;"> If that's the case, then these lines of code:</p><p style="margin: 0;"><br></p><p style="margin: 0;">    if (ret) {</p><p style="margin: 0;"><span style="white-space: normal;"><span style="white-space:pre">          </span>gmac_clk_enable(bsp_priv, false);</span></p><p style="margin: 0;"><span style="white-space: normal;"><span style="white-space:pre">            </span>return ret;</span></p><p style="margin: 0;"><span style="white-space: normal;">   }</span></p><p style="margin: 0;"><span style="white-space: normal;"><br></span></p><p style="margin: 0;">Will never get executed. I will send another patch to fix this issue<br>If the PHY power-on fails, then in the MAC-PHY connected configuration, the network interface cannot operate properly.</p><p style="margin: 0;">BUT, the MAC clock remains enabled and is not turned off, which is incorrec</p><p style="margin: 0;"><br></p><p style="margin: 0;"><br></p><p style="margin: 0;"><br></p></div><div style="position:relative;zoom:1"></div><div id="divNeteaseMailCard"></div><p style="margin: 0;"><br></p><pre><br>At 2025-05-24 22:48:15, "Andrew Lunn" <andrew@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
</pre></div><img style="width:1px;height:1px" src="https://count.mail.163.com/beacon/webmail.gif?type=webmail_mailtrace&guid=pre_0b388164e51ce62e7fa8d05648092d70">