netdev - Re: [PATCH net] net: stmmac: dwmac-rk: Ensure clk

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] [day] [month] [year] [list]

Message-ID: <bda5453b-5cc8-4d31-9143-3e23b5d914d2@rock-chips.com>
Date: Mon, 15 Sep 2025 11:38:10 +0800
From: Chaoyi Chen <chaoyi.chen@...k-chips.com>
To: Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: Yao Zi <ziyao@...root.org>, "Russell King (Oracle)"
 <linux@...linux.org.uk>, Andrew Lunn <andrew+netdev@...n.ch>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Jonas Karlman <jonas@...boo.se>, David Wu <david.wu@...k-chips.com>,
 netdev@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH net] net: stmmac: dwmac-rk: Ensure clk_phy doesn't contain
 invalid address

Hi Sebastian,

On 9/7/2025 4:25 AM, Sebastian Reichel wrote:
> Hi,
>
> On Sat, Sep 06, 2025 at 02:26:31PM +0800, Chaoyi Chen wrote:
>> On 9/6/2025 1:36 PM, Yao Zi wrote:
>>
>>> On Thu, Sep 04, 2025 at 12:07:26PM +0100, Russell King (Oracle) wrote:
>>>> On Thu, Sep 04, 2025 at 12:05:19PM +0100, Russell King (Oracle) wrote:
>>>>> On Thu, Sep 04, 2025 at 07:03:10PM +0800, Chaoyi Chen wrote:
>>>>>> On 9/4/2025 6:58 PM, Russell King (Oracle) wrote:
>>>>>>> On Thu, Sep 04, 2025 at 03:12:24AM +0000, Yao Zi wrote:
>>>>>>>>     	if (plat->phy_node) {
>>>>>>>>     		bsp_priv->clk_phy = of_clk_get(plat->phy_node, 0);
>>>>>>>>     		ret = PTR_ERR_OR_ZERO(bsp_priv->clk_phy);
>>>>>>>> -		/* If it is not integrated_phy, clk_phy is optional */
>>>>>>>> +		/*
>>>>>>>> +		 * If it is not integrated_phy, clk_phy is optional. But we must
>>>>>>>> +		 * set bsp_priv->clk_phy to NULL if clk_phy isn't proivded, or
>>>>>>>> +		 * the error code could be wrongly taken as an invalid pointer.
>>>>>>>> +		 */
>>>>>>> I'm concerned by this. This code is getting the first clock from the DT
>>>>>>> description of the PHY. We don't know what type of PHY it is, or what
>>>>>>> the DT description of that PHY might suggest that the first clock would
>>>>>>> be.
>>>>>>>
>>>>>>> However, we're geting it and setting it to 50MHz. What if the clock is
>>>>>>> not what we think it is?
>>>>>> We only set integrated_phy to 50M, which are all known targets. For external PHYs, we do not perform frequency settings.
>>>>> Same question concerning enabling and disabling another device's clock
>>>>> that the other device should be handling.
>>>> Let me be absolutely clear: I consider *everything* that is going on
>>>> with clk_phy here to be a dirty hack.
>>>>
>>>> Resources used by a device that has its own driver should be managed
>>>> by _that_ driver alone, not by some other random driver.
>>> Agree on this. Should we drop the patch, or fix it up for now to at
>>> least prevent the oops? Chaoyi, I guess there's no user of the feature
>>> for now, is it?
>> This at least needs fixing. Sorry, I have no idea how to implement
>> this in the PHY.
> I think the proper fix is to revert da114122b8314 ("net: ethernet:
> stmmac: dwmac-rk: Make the clk_phy could be used for external phy"),
> which has only recently been merged. External PHYs should reference
> their clocks themself instead of the MAC doing it.
>
> Chaoyi Chen: Have a look at the ROCK 4D devicetree:
>
> &mdio0 {
> 	rgmii_phy0: ethernet-phy@1 {
> 		compatible = "ethernet-phy-id001c.c916";
> 		reg = <0x1>;
> 		clocks = <&cru REFCLKO25M_GMAC0_OUT>;
> 		assigned-clocks = <&cru REFCLKO25M_GMAC0_OUT>;
> 		assigned-clock-rates = <25000000>;
>          ...
>      };
> };
>
> The clock is enabled by the RTL8211F PHY driver (check for
> devm_clk_get_optional_enabled in drivers/net/phy/realtek/realtek_main.c),
> as the PHY is the one needing the clock and not the Rockchip MAC. For
> this to work it is important to set the right compatible string, so
> that the kernel can probe the right driver without needing to read the
> identification registers (as that would require the clock to be already
> configured before the driver is being probed).

Yes, what you said is correct. This is also the issue we encountered earlier on RK3576 board :)