Message-Id: <20250919073826.3629945-1-yicongsrfy@163.com>
Date: Fri, 19 Sep 2025 15:38:26 +0800
From: yicongsrfy@....com
To: linux@...linux.org.uk,
	andrew@...n.ch,
	Frank.Sae@...or-comm.com
Cc: davem@...emloft.net,
	edumazet@...gle.com,
	hkallweit1@...il.com,
	kuba@...nel.org,
	linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org,
	pabeni@...hat.com,
	yicong@...inos.cn
Subject: Re: [PATCH] net: phy: avoid config_init failure on unattached PHY during resume

On Wed, 17 Sep 2025 11:06:37 +0100, "Russell King (Oracle)" <linux@...linux.org.uk> wrote:
>
> I don't see that there is anything that a PHY driver can do to solve
> this as the code currently stands, especially if the MAC driver is
> coded to "use the lowest PHY address present on the MDIO bus" (which
> in this case will be your vendor's idea of a broadcast address.)
>
> I really don't think we should start adding hacks for this kind of
> stuff into phylib's core - we can't know that PHY address 0 is a
> duplicate of another address.
>
> The only thing I can come up with is:
>
> 1. There must be a way to configure the PHY to disable its non-
>    standard "broadcast MDIO address" to make it compliant with 802.3.
>    I suggest that board firmware needs to set that to make the PHY
>    compliant.
>
> 2. As a hard reset of the PHY will likely clear that setting, this is
>    another nail in the coffin of PHY hard reset handling in the kernel
>    which has been the cause of many issues. (Another nail in that
>    coffin is that some MACs require the RX clock from the PHY to be
>    running in order to properly reset.)

Thank you for your reply!

Since this issue cannot be fundamentally resolved within phylib,
we need to seek a solution within the PHY driver itself.

Let's return to the original problem: how to solve the suspend/resume
issue caused by a single physical PHY device generating multiple
`phy_device` instances?

A key requirement for a device's suspend/resume functionality is
power saving, which ultimately needs to take effect on the physical
hardware. Therefore, we only need to ensure that one `phy_device`
instance operates on the actual physical device.

Based on this idea, I've made the following modifications:
1.  Add a check within the PHY driver to identify the broadcast address.
2.  In `config_init`, `suspend`, and `resume` functions, if the broadcast
address is encountered, return immediately without further processing.

This approach assumes that the MAC driver correctly uses the PHY's
hardware address, rather than finding address 0 via `phy_find_first`,
which might be the default broadcast address for some PHY chips.

Here is a partial patch:
```
+/**
+ * yt8521_check_broadcast_phyaddr()
+ * - check is it a phydev with broadcast addr.
+ * @phydev: a pointer to a &struct phy_device
+ *
+ * returns true or false.
+ */
+static bool yt8521_check_broadcast_phyaddr(struct phy_device *phydev)
+{
+	if (/* addr 0 is broaddcast OR other broadcast addr */)
+		return true;
+
+	return false;
+}
+

static int yt8521_probe(struct phy_device *phydev) {
    ...
+	/* Logically speaking, it should return directly here,
+	 * although it don't have the expected effect.
+	 */
+	if (yt8521_check_broadcast_phyaddr(phydev))
+		return -ENODEV;
    ...
}

static int yt8521_suspend(struct phy_device *phydev) {
    ...
+	if (yt8521_check_broadcast_phyaddr(phydev))
+		return 0;
    ...
}

static int yt8521_resume(struct phy_device *phydev) {
    ...
+	if (yt8521_check_broadcast_phyaddr(phydev))
+		return 0;
    ...
}

static int yt8521_config_init(struct phy_device *phydev) {
    ...
+	if (yt8521_check_broadcast_phyaddr(phydev))
+		return 0;
    ...
}
```

Testing has confirmed that this fix is effective.

Could you please review if this fix approach is appropriate? If it's acceptable, I will send a complete patch v2.
Thank you very much!

Powered by blists - more mailing lists