[PATCH net v2] net: phy: mscc: fix deadlock in vsc85xx_default_config

[PATCH net v2] net: phy: mscc: fix deadlock in vsc85xx_default_config

[Quentin Schulz]

The vsc85xx_default_config function called in the vsc85xx_config_init
function which is used by VSC8530, VSC8531, VSC8540 and VSC8541 PHYs
mistakenly calls phy_read and phy_write in-between phy_select_page and
phy_restore_page.

phy_select_page and phy_restore_page actually take and release the MDIO
bus lock and phy_write and phy_read take and release the lock to write
or read to a PHY register.

Let's fix this deadlock by using phy_modify_paged which handles
correctly a read followed by a write in a non-standard page.

Fixes: 6a0bfbbe20b0 ("net: phy: mscc: migrate to phy_select/restore_page functions")

Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---

v2:
 - use phy_modify_paged instead of
 phy_select_page -> __phy_read -> __phy_write -> phy_restore_page

 drivers/net/phy/mscc.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index 62269e578718..4dcf7ad06259 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -810,17 +810,16 @@ static int vsc85xx_default_config(struct phy_device *phydev)
 
 	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
 	mutex_lock(&phydev->lock);
-	rc = phy_select_page(phydev, MSCC_PHY_PAGE_EXTENDED_2);
+
+	reg_val = RGMII_RX_CLK_DELAY_1_1_NS << RGMII_RX_CLK_DELAY_POS;
+
+	rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
+			      MSCC_PHY_RGMII_CNTL, RGMII_RX_CLK_DELAY_MASK,
+			      reg_val);
 	if (rc < 0)
 		goto out_unlock;
 
-	reg_val = phy_read(phydev, MSCC_PHY_RGMII_CNTL);
-	reg_val &= ~(RGMII_RX_CLK_DELAY_MASK);
-	reg_val |= (RGMII_RX_CLK_DELAY_1_1_NS << RGMII_RX_CLK_DELAY_POS);
-	phy_write(phydev, MSCC_PHY_RGMII_CNTL, reg_val);
-
 out_unlock:
-	rc = phy_restore_page(phydev, rc, rc > 0 ? 0 : rc);
 	mutex_unlock(&phydev->lock);
 
 	return rc;
-- 
2.17.1

Re: [PATCH net v2] net: phy: mscc: fix deadlock in vsc85xx_default_config

[Andrew Lunn]

On Fri, Nov 23, 2018 at 09:16:36AM +0100, Quentin Schulz wrote:
> The vsc85xx_default_config function called in the vsc85xx_config_init
> function which is used by VSC8530, VSC8531, VSC8540 and VSC8541 PHYs
> mistakenly calls phy_read and phy_write in-between phy_select_page and
> phy_restore_page.
> 
> phy_select_page and phy_restore_page actually take and release the MDIO
> bus lock and phy_write and phy_read take and release the lock to write
> or read to a PHY register.
> 
> Let's fix this deadlock by using phy_modify_paged which handles
> correctly a read followed by a write in a non-standard page.
> 
> Fixes: 6a0bfbbe20b0 ("net: phy: mscc: migrate to phy_select/restore_page functions")
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
> ---
> 
> v2:
>  - use phy_modify_paged instead of
>  phy_select_page -> __phy_read -> __phy_write -> phy_restore_page
> 
>  drivers/net/phy/mscc.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
> index 62269e578718..4dcf7ad06259 100644
> --- a/drivers/net/phy/mscc.c
> +++ b/drivers/net/phy/mscc.c
> @@ -810,17 +810,16 @@ static int vsc85xx_default_config(struct phy_device *phydev)
>  
>  	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
>  	mutex_lock(&phydev->lock);
> -	rc = phy_select_page(phydev, MSCC_PHY_PAGE_EXTENDED_2);
> +
> +	reg_val = RGMII_RX_CLK_DELAY_1_1_NS << RGMII_RX_CLK_DELAY_POS;
> +
> +	rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
> +			      MSCC_PHY_RGMII_CNTL, RGMII_RX_CLK_DELAY_MASK,
> +			      reg_val);
>  	if (rc < 0)
>  		goto out_unlock;

Hi Quentin

Isn't this goto now pointless. You are not jumping over anything.

      Andrew

>  
> -	reg_val = phy_read(phydev, MSCC_PHY_RGMII_CNTL);
> -	reg_val &= ~(RGMII_RX_CLK_DELAY_MASK);
> -	reg_val |= (RGMII_RX_CLK_DELAY_1_1_NS << RGMII_RX_CLK_DELAY_POS);
> -	phy_write(phydev, MSCC_PHY_RGMII_CNTL, reg_val);
> -
>  out_unlock:
> -	rc = phy_restore_page(phydev, rc, rc > 0 ? 0 : rc);
>  	mutex_unlock(&phydev->lock);
>  
>  	return rc;
> -- 
> 2.17.1
> 

Re: [PATCH net v2] net: phy: mscc: fix deadlock in vsc85xx_default_config

[Quentin Schulz]

[-- Attachment #1: Type: text/plain, Size: 2189 bytes --]

Hi Andrew,

On Fri, Nov 23, 2018 at 04:08:06PM +0100, Andrew Lunn wrote:
> On Fri, Nov 23, 2018 at 09:16:36AM +0100, Quentin Schulz wrote:
> > The vsc85xx_default_config function called in the vsc85xx_config_init
> > function which is used by VSC8530, VSC8531, VSC8540 and VSC8541 PHYs
> > mistakenly calls phy_read and phy_write in-between phy_select_page and
> > phy_restore_page.
> > 
> > phy_select_page and phy_restore_page actually take and release the MDIO
> > bus lock and phy_write and phy_read take and release the lock to write
> > or read to a PHY register.
> > 
> > Let's fix this deadlock by using phy_modify_paged which handles
> > correctly a read followed by a write in a non-standard page.
> > 
> > Fixes: 6a0bfbbe20b0 ("net: phy: mscc: migrate to phy_select/restore_page functions")
> > 
> > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
> > ---
> > 
> > v2:
> >  - use phy_modify_paged instead of
> >  phy_select_page -> __phy_read -> __phy_write -> phy_restore_page
> > 
> >  drivers/net/phy/mscc.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
> > index 62269e578718..4dcf7ad06259 100644
> > --- a/drivers/net/phy/mscc.c
> > +++ b/drivers/net/phy/mscc.c
> > @@ -810,17 +810,16 @@ static int vsc85xx_default_config(struct phy_device *phydev)
> >  
> >  	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
> >  	mutex_lock(&phydev->lock);
> > -	rc = phy_select_page(phydev, MSCC_PHY_PAGE_EXTENDED_2);
> > +
> > +	reg_val = RGMII_RX_CLK_DELAY_1_1_NS << RGMII_RX_CLK_DELAY_POS;
> > +
> > +	rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
> > +			      MSCC_PHY_RGMII_CNTL, RGMII_RX_CLK_DELAY_MASK,
> > +			      reg_val);
> >  	if (rc < 0)
> >  		goto out_unlock;
> 
> Hi Quentin
> 
> Isn't this goto now pointless. You are not jumping over anything.
> 

Grmbl episode 2 :)

That's what you get from adding a line, taking the ones you're replacing
as reference and then remove the lines you've to replace without
checking what's left.

Sorry for the noise, I'll boot up my brain next time.

Thanks,
Quentin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]