Re: [Patch net-next v3] net: phy: Add driver for Motorcomm Quad 2.5GbE phy

On 7/17/26 23:11, Andrew Lunn wrote:

quoted

+/**
+ * ytphy_read_top_ext() - read a PHY's top extended register for YT8824
+ * @phydev: a pointer to a &struct phy_device
+ * @regnum: register number to read
+ *
+ * NOTE:The caller must have taken the MDIO bus lock.

Didn't i request this is done in code, not comments?

Ans: fix done in patch v4 by called lockdep_assert_held to make
sure to hold the lock.

quoted

+static int ytphy_read_top_ext(struct phy_device *phydev, u16 regnum)
+{
+	struct yt8824_shared_priv *shared_priv;
+	unsigned int offset;
+	int ret;
+
+	shared_priv = phy_package_get_priv(phydev);
+	offset = shared_priv->offset;
+	ret = __phy_package_write(phydev, offset, YTPHY_PAGE_SELECT, regnum);

You look to be using the package wrongly.

https://elixir.bootlin.com/linux/v7.1.3/source/drivers/net/phy/phy_package.c#L192

 * The base_addr parameter serves as cookie which has to have the same values
 * for all members of one group and as the base PHY address of the PHY package
 * for offset calculation to access generic registers of a PHY package.
 * Usually, one of the PHY addresses of the different PHYs in the package
 * provides access to these global registers.
 * The address which is given here, will be used in the __phy_package_read()
 * and __phy_package_write() convenience functions as base and added to the
 * passed offset in those functions.

Although it is called base, it can be above all the others. So when
you create the package, set the cookie value to your top address. You
can then use __phy_package_write() directly, offset of 0.

Ans: fix done in patch v4, the base_addr is used to access to the top extend register
for phy8824.

quoted

-static struct phy_driver motorcomm_phy_drvs[] = {
-	{
-		PHY_ID_MATCH_EXACT(PHY_ID_YT8511),
-		.name		= "YT8511 Gigabit Ethernet",
-		.config_init	= yt8511_config_init,
-		.suspend	= genphy_suspend,
-		.resume		= genphy_resume,
-		.read_page	= yt8511_read_page,
-		.write_page	= yt8511_write_page,
-	},

I still don't like this. Please stop diff deleting all these and
putting them back later. It makes me thing something changed, maybe
white space?

Ans: I tried to refactor this issue by hanging the position of the newly
added code. However, the same issue may still arise in other code segments, 
without modifying the existing code.

quoted

+static int yt8824_utp_softreset_paged(struct phy_device *phydev,
+				      int reg_space)
+{
+	int old_page;
+	int ret = 0;
+	int val;
+
+	old_page = phy_select_page(phydev, reg_space);
+	if (old_page < 0)
+		goto err_restore_page;
+	if (reg_space == YT8824_RSSR_UTP_SPACE) {
+		ret = __phy_read(phydev, MII_BMCR);
+		if (ret < 0)
+			goto err_restore_page;
+		ret |= BMCR_RESET;
+		ret = __phy_write(phydev, MII_BMCR, ret);

So here you are using __phy_write() so it does not take the mdio lock.

quoted

+		if (ret < 0)
+			goto err_restore_page;
+		/* wait until softreset done. */
+		ret = phy_read_poll_timeout(phydev, MII_BMCR, val, !(val & BMCR_RESET),
+					    50000, 600000, true);

This uses phy_read(), so does take the lock.

How can this be correct?

Please take some time to think about locking. Please enable all the
locking debug options in the kernel and see if you get splats. And
also really do turn your locking comments into code.

Ans: this issue fixed done in patch v4. The correct procedure might be to acquire the mdio lock 
within this phy_select_page() and release it within phy_restore_page(). During this period, 
all interfaces that involve acquiring the mdio lock are unacceptable and incorrect.

    Andrew