netdev - Re: [PATCH] net: phy: return EPROBE_DEFER if PHY is not accessible

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]

Date:   Thu, 23 Mar 2023 15:19:21 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     "Buzarra, Arturo" <Arturo.Buzarra@...i.com>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Russell King - ARM Linux <linux@...linux.org.uk>
Subject: Re: [PATCH] net: phy: return EPROBE_DEFER if PHY is not accessible

> Gigabit PHY has its own Crystal, however the 10/100 PHY uses a clock
> from the CPU and it is the root cause of the issue because when
> Linux asks the PHY capabilities the clock is not ready yet.

O.K, now we are getting closer.

Which clock is it exactly? Both for the MAC and the PHY?

> We have identified the root cause of the 0xFFFF issue, it is because
> the two PHYs are defined in the same MDIO bus node inside the first
> Ethernet MAC node, and when the 10/100 PHY is probed the PHY Clock
> from the CPU is not ready, this is the DT definition:

> ---------
> /* Gigabit Ethernet */
> &eth1 {
> 	status = "okay";
> 	pinctrl-0 = <&eth1_rgmii_pins>;
> 	pinctrl-names = "default";
> 	phy-mode = "rgmii-id";
> 	max-speed = <1000>;
> 	phy-handle = <&phy0_eth1>;
> 
> 	mdio1 {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 		compatible = "snps,dwmac-mdio";
> 
> 		phy0_eth1: ethernet-phy@0 {
> 			reg = <0>;
> 			compatible = "ethernet-phy-id0141.0dd0"; /* PHY ID for Marvell 88E1512 */
> 			reset-gpios = <&gpioi 2 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
> 			reset-assert-us = <1000>;
> 			reset-deassert-us = <2000>;
> 		};
> 
> 		phy0_eth2: ethernet-phy@1 {
> 			reg = <1>;
> 			compatible = "ethernet-phy-id0007.c0f0"; /* PHY ID for SMSC LAN8720Ai */
> 			reset-gpios = <&gpioh 7 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
> 			interrupt-parent = <&gpioh>;
> 			interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
> 		};
> 	};
> };

This all looks reasonable.

> /* 10/100 Ethernet */
> &eth2 {
> 	status = "okay";
> 	pinctrl-0 = <&eth2_rmii_pins>;
> 	pinctrl-names = "default";
> 	phy-mode = "rmii";
> 	max-speed = <100>;
> 	phy-handle = <&phy0_eth2>;
> 	st,ext-phyclk;
> };

st,ext-phyclk is interesting. But looking at the code, that means the
clock is coming from somewhere else. And there appears to be eth-ck,
which the driver does a

devm_clk_get() on.

Is eth-ck being fed to both the MAC and the PHY? Is this the missing
clock?

So this is what we need to fix. And the correct way to do this is to
express this clock in DT. If you look at the PHY driver smsc, in its
probe function it has:

       /* Make clk optional to keep DTB backward compatibility. */
        refclk = devm_clk_get_optional_enabled(dev, NULL);
        if (IS_ERR(refclk))
                return dev_err_probe(dev, PTR_ERR(refclk),
                                     "Failed to request clock\n");

If there is a clock in DT, it will try to enable it. If the clock does
not exist yet, it will fail the probe with -EPRODE_DEFER, and the core
will try the probe again later, hopefully once the clock exists.

So this is the clock consumer. You also need a clock provider. What
exactly is this clock? We need the answer to the questions above,
but... If it is a SoC clock, it is quite likely there already is a
clock provider for it. If it is a clock in the MAC, you need to extend
the MAC driver to implement a clock provider. Maybe
meson8b_dwmac_register_clk() is a useful example?

	Andrew