Re: [PATCH net-next 1/3] net: mdiobus: add APIs for modifying a MDIO device register

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

On Sat, Mar 14, 2020 at 10:57:28PM +0100, Andrew Lunn wrote:
> On Sat, Mar 14, 2020 at 10:31:24AM +0000, Russell King wrote:
> > Add APIs for modifying a MDIO device register, similar to the existing
> > phy_modify() group of functions, but at mdiobus level instead.  Adapt
> > __phy_modify_changed() to use the new mdiobus level helper.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/phy/mdio_bus.c | 55 ++++++++++++++++++++++++++++++++++++++
> >  drivers/net/phy/phy-core.c | 31 ---------------------
> >  include/linux/mdio.h       |  4 +++
> >  include/linux/phy.h        | 19 +++++++++++++
> >  4 files changed, 78 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> > index 3ab9ca7614d1..b33d1e793686 100644
> > --- a/drivers/net/phy/mdio_bus.c
> > +++ b/drivers/net/phy/mdio_bus.c
> > @@ -824,6 +824,38 @@ int __mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
> >  }
> >  EXPORT_SYMBOL(__mdiobus_write);
> >  
> > +/**
> > + * __mdiobus_modify_changed - Unlocked version of the mdiobus_modify function
> > + * @bus: the mii_bus struct
> > + * @addr: the phy address
> > + * @regnum: register number to modify
> > + * @mask: bit mask of bits to clear
> > + * @set: bit mask of bits to set
> > + *
> > + * Read, modify, and if any change, write the register value back to the
> > + * device. Any error returns a negative number.
> > + *
> > + * NOTE: MUST NOT be called from interrupt context.
> > + */
> > +int __mdiobus_modify_changed(struct mii_bus *bus, int addr, u32 regnum,
> > +			     u16 mask, u16 set)
> > +{
> > +	int new, ret;
> > +
> > +	ret = __mdiobus_read(bus, addr, regnum);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	new = (ret & ~mask) | set;
> > +	if (new == ret)
> > +		return 0;
> > +
> > +	ret = __mdiobus_write(bus, addr, regnum, new);
> > +
> > +	return ret < 0 ? ret : 1;
> > +}
> > +EXPORT_SYMBOL_GPL(__mdiobus_modify_changed);
> > +
> >  /**
> >   * mdiobus_read_nested - Nested version of the mdiobus_read function
> >   * @bus: the mii_bus struct
> > @@ -928,6 +960,29 @@ int mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
> >  }
> >  EXPORT_SYMBOL(mdiobus_write);
> >  
> > +/**
> > + * mdiobus_modify - Convenience function for modifying a given mdio device
> > + *	register
> > + * @bus: the mii_bus struct
> > + * @addr: the phy address
> > + * @regnum: register number to write
> > + * @mask: bit mask of bits to clear
> > + * @set: bit mask of bits to set
> > + */
> > +int mdiobus_modify(struct mii_bus *bus, int addr, u32 regnum, u16 mask, u16 set)
> > +{
> > +	int err;
> > +
> > +	BUG_ON(in_interrupt());
> 
> Hi Russell
> 
> There seems to be growing push back on using BUG_ON and its
> variants. If should only be used if the system is so badly messed up,
> going further would only cause more damage. What really happens here
> if it is called in interrupt context? The mutex lock probably won't
> work, and we might corrupt the state of the PCS. That is not the end
> of the world. So i would suggest a WARN_ON here.

Do we even need these checks? (phylib has them scattered throughout
on the bus accessors.)  Aren't the might_sleep() checks that are
already in the locking functions already sufficient?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up