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

[Quentin Schulz]

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@xxxxxxxxxxx>
> > ---
> > 
> > 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: signature.asc
Description: PGP signature