> > > +static int dp83822_suspend(struct phy_device *phydev)
> > > +{
> > > + int value;
> > > +
> > > + mutex_lock(&phydev->lock);
> > > + value = phy_read_mmd(phydev, DP83822_DEVADDR,
> > > MII_DP83822_WOL_CFG);
> > > + mutex_unlock(&phydev->lock);
>
> > Would we need mutex to access phy_read_mmd()?
> > phy_read_mmd() has mdio_lock for indirect access.
>
> Hi Woojung
>
> The mdio lock is not sufficient. It protects against two mdio
> accesses. But here we need to protect against two phy operations.
> There is a danger something else tries to access the phy during
> suspend.
Hi Andrew,
Thanks for comment. I have question then.
phy_read_mmd() is protected by bus->mdio_lock around mmd_indirect & mdio_read.
While these operation, other phy_read() & phy_write() will be blocked inside
mdiobus_read() & mdiobus_write().
Because this phy_read_mmd(.., DP83822_DEVADDR, MII_DP83822_WOL_CFG) is not
read-modify-write operation, I think phydev->lock may not be necessary.
Am I missing something?
> > > + if (!(value & DP83822_WOL_EN))
> > > + genphy_suspend(phydev);
>
> Releasing the lock before calling genphy_suspend() is not so nice.
> Maybe add a version which assumes the lock has already been taken?
>
We can expand genphy_suspend() per setting by phy_driver->set_wol.
When genphy_suspend() acts per wol setting, not many phy driver needs to extra
work
When WOL is enabled.
How do you think?
- Woojung
