Re: [PATCH net-next v3 3/5] net: phy: mscc: Drop unnecessary phydev->lock

Russell King (Oracle)
2026-04-14T17:48:07Z

On Sun, Apr 12, 2026 at 03:00:25PM +0100, Biju wrote:
> @@ -486,15 +486,9 @@ static int vsc85xx_dt_led_modes_get(struct phy_device *phydev,
>
> static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev, u8 edge_rate)
> {
> - int rc;
> -
> - mutex_lock(&phydev->lock);
> - rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
> - MSCC_PHY_WOL_MAC_CONTROL, EDGE_RATE_CNTL_MASK,
> - edge_rate << EDGE_RATE_CNTL_POS);
> - mutex_unlock(&phydev->lock);
> -
> - return rc;
> + return phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
> + MSCC_PHY_WOL_MAC_CONTROL, EDGE_RATE_CNTL_MASK,
> + edge_rate << EDGE_RATE_CNTL_POS);

This one is fine.

> @@ -503,7 +497,6 @@ static int vsc85xx_mac_if_set(struct phy_device *phydev,
> int rc;
> u16 reg_val;
>
> - mutex_lock(&phydev->lock);
> reg_val = phy_read(phydev, MSCC_PHY_EXT_PHY_CNTL_1);
> reg_val &= ~(MAC_IF_SELECTION_MASK);
> switch (interface) {
> @@ -522,17 +515,15 @@ static int vsc85xx_mac_if_set(struct phy_device *phydev,
> break;
> default:
> rc = -EINVAL;
> - goto out_unlock;
> + goto err;
> }
> rc = phy_write(phydev, MSCC_PHY_EXT_PHY_CNTL_1, reg_val);

I would much rather this was converted to use phy_modify() as well so
that we ensure that the update is atomic.

rc = phy_modify(phydev, MSCC_PHY_EXT_PHY_CNTL_1,
MAC_IF_SELECTION_MASK, reg_val);

where reg_val is assigned the field value in the switch above.

> @@ -668,19 +659,15 @@ static int vsc8531_pre_init_seq_set(struct phy_device *phydev)
> if (rc < 0)
> return rc;
>
> - mutex_lock(&phydev->lock);
> oldpage = phy_select_page(phydev, MSCC_PHY_PAGE_TR);
> if (oldpage < 0)
> - goto out_unlock;
> + goto restore_oldpage;
>
> for (i = 0; i < ARRAY_SIZE(init_seq); i++)
> vsc85xx_tr_write(phydev, init_seq[i].reg, init_seq[i].val);
>
> -out_unlock:
> - oldpage = phy_restore_page(phydev, oldpage, oldpage);
> - mutex_unlock(&phydev->lock);
> -
> - return oldpage;
> +restore_oldpage:
> + return phy_restore_page(phydev, oldpage, oldpage);

This is fine.

> @@ -708,19 +695,15 @@ static int vsc85xx_eee_init_seq_set(struct phy_device *phydev)
> unsigned int i;
> int oldpage;
>
> - mutex_lock(&phydev->lock);
> oldpage = phy_select_page(phydev, MSCC_PHY_PAGE_TR);
> if (oldpage < 0)
> - goto out_unlock;
> + goto restore_oldpage;
>
> for (i = 0; i < ARRAY_SIZE(init_eee); i++)
> vsc85xx_tr_write(phydev, init_eee[i].reg, init_eee[i].val);
>
> -out_unlock:
> - oldpage = phy_restore_page(phydev, oldpage, oldpage);
> - mutex_unlock(&phydev->lock);
> -
> - return oldpage;
> +restore_oldpage:
> + return phy_restore_page(phydev, oldpage, oldpage);

Also fine.

Thanks.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!