Re: [PATCH net] net: stmmac: dwmac-rk: Ensure clk_phy doesn't contain invalid address

[Chaoyi Chen <chaoyi.chen@rock-chips.com>]

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 :)


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip