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

[Russell King (Oracle)]

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!