On Sun, Dec 31, 2017 at 10:07:14AM +0000, Russell King - ARM Linux wrote: > On Sun, Dec 31, 2017 at 09:10:39AM +0100, Andrew Lunn wrote: > > > +/** > > > + * phy_save_page() - take the bus lock and save the current page > > > + * @phydev: a pointer to a &struct phy_device > > > + * > > > + * Take the MDIO bus lock, and return the current page number. On error, > > > + * returns a negative errno. phy_restore_page() must be called after this > > > + * to release the lock even on failure. > > > + */ > > > +int phy_save_page(struct phy_device *phydev) > > > +{ > > > + mutex_lock(&phydev->mdio.bus->mdio_lock); > > > + return __phy_read_page(phydev); > > > +} > > > +EXPORT_SYMBOL_GPL(phy_save_page); > > > + > > > +/** > > > + * phy_select_page() - take the bus lock, save the current page, and set > > > a page > > > + * @phydev: a pointer to a &struct phy_device > > > + * @page: desired page > > > + * > > > + * Take the MDIO bus lock to protect against concurrent access, save the > > > + * current PHY page, and set the current page. On error, returns a > > > + * negative errno, otherwise returns the previous page number. > > > + * phy_restore_page() must be called after this to restore the page > > > + * number (if this call was successful) and release the lock. > > > > Hi Russell > > > > This comment seems wrong. It looks like you need to call > > phy_restore_page() on error as well. I think the text in () should be > > removed, and add the "even on failure" which the previous function > > states. > > It's one of those slightly confusing bits of English, and I doubt > there's a way to express it better. > > phy_restore_page() must be called after this to [restore the page > number (if this call was successful) and] release the lock. > > I could use: > > phy_restore_page() must always be called to release the lock, > and restore the page number if this call was successful. > > but that can still be read in an ambiguous manner.
I've decided to solve this by changing it to: + * phy_restore_page() must always be called after this, irrespective + * of success or failure of this call. iow, not explaining /why/. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up