netdev - [net-next,v2,06/15] net: ftgmac100: Use devm_clk_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <SEYPR06MB51340AC0AF94D5DD315658AA9D97A@SEYPR06MB5134.apcprd06.prod.outlook.com>
Date: Thu, 22 Jan 2026 05:37:59 +0000
From: Jacky Chou <jacky_chou@...eedtech.com>
To: Simon Horman <horms@...nel.org>
CC: "davem@...emloft.net" <davem@...emloft.net>, "kuba@...nel.org"
	<kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "andrew+netdev@...n.ch"
	<andrew+netdev@...n.ch>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: [net-next,v2,06/15] net: ftgmac100: Use devm_clk_get_enabled

> 寄件者: Simon Horman <horms@...nel.org>
> 寄件日期: Tuesday, January 20, 2026 8:20 PM
> 收件者: Jacky Chou <jacky_chou@...eedtech.com>
> 副本: Simon Horman <horms@...nel.org>; davem@...emloft.net;
> kuba@...nel.org; pabeni@...hat.com; edumazet@...gle.com;
> andrew+netdev@...n.ch; netdev@...r.kernel.org
> 主旨: Re: [net-next,v2,06/15] net: ftgmac100: Use devm_clk_get_enabled
> 
> This is an AI-generated review of your patch. The human sending this email has
> considered the AI review valid, or at least plausible.
> 
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---
> net: ftgmac100: Use devm_clk_get_enabled
> 
> This patch converts the ftgmac100 driver to use devm_clk_get_enabled() and
> devm_clk_get_optional_enabled() to simplify clock management and cleanup
> paths.
> 
> > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
> > b/drivers/net/ethernet/faraday/ftgmac100.c
> > index ec2e7ec23ddf..ffd86655bcc8 100644
> > --- a/drivers/net/ethernet/faraday/ftgmac100.c
> > +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> > @@ -1801,13 +1801,10 @@ static int ftgmac100_setup_clk(struct
> ftgmac100 *priv)
> >  	struct clk *clk;
> >  	int rc;
> >
> > -	clk = devm_clk_get(priv->dev, NULL /* MACCLK */);
> > +	clk = devm_clk_get_enabled(priv->dev, NULL /* MACCLK */);
> >  	if (IS_ERR(clk))
> >  		return PTR_ERR(clk);
> >  	priv->clk = clk;
> > -	rc = clk_prepare_enable(priv->clk);
> > -	if (rc)
> > -		return rc;
> >
> >  	/* Aspeed specifies a 100MHz clock is required for up to
> >  	 * 1000Mbit link speeds. As NCSI is limited to 100Mbit, 25MHz @@
> > -1816,21 +1813,15 @@ static int ftgmac100_setup_clk(struct ftgmac100
> *priv)
> >  	rc = clk_set_rate(priv->clk, priv->use_ncsi ? FTGMAC_25MHZ :
> >  			  FTGMAC_100MHZ);
> >  	if (rc)
> > -		goto cleanup_clk;
> > +		return rc;
> >
> >  	/* RCLK is for RMII, typically used for NCSI. Optional because it's not
> >  	 * necessary if it's the AST2400 MAC, or the MAC is configured for
> >  	 * RGMII, or the controller is not an ASPEED-based controller.
> >  	 */
> > -	priv->rclk = devm_clk_get_optional(priv->dev, "RCLK");
> > -	rc = clk_prepare_enable(priv->rclk);
> > -	if (!rc)
> > -		return 0;
> > +	priv->rclk = devm_clk_get_optional_enabled(priv->dev, "RCLK");
> >
> > -cleanup_clk:
> > -	clk_disable_unprepare(priv->clk);
> > -
> > -	return rc;
> > +	return 0;
> >  }
> 
> Should the return value of devm_clk_get_optional_enabled() be checked here?
> The old code would fail probe if RCLK existed but
> clk_prepare_enable() failed. The new code unconditionally returns 0, ignoring
> any potential error from devm_clk_get_optional_enabled().
> 
> Looking at other drivers using this API, most check for IS_ERR(). For example,
> cctrng_probe() in drivers/char/hw_random/cctrng.c does:
> 
>     drvdata->clk = devm_clk_get_optional_enabled(dev, NULL);
>     if (IS_ERR(drvdata->clk))
>         return dev_err_probe(...);
> 
> While the clock is optional (may not exist), if it does exist but fails to enable,
> this seems like a condition that should cause probe to fail rather than succeed
> silently?

Agreed for AI review.
We missed checking the return value from devm_clk_get_optional_enabled().
I will fix this in the next version.

Thanks,
Jacky