Re: [PATCH net-next 4/7] net: phy: add paged phy register accessors

[Russell King - ARM Linux <linux@armlinux.org.uk>]

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