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

[Andrew Lunn <andrew@lunn.ch>]

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.

> +
> +	mutex_lock(&bus->mdio_lock);
> +	err = __mdiobus_modify_changed(bus, addr, regnum, mask, set);
> +	mutex_unlock(&bus->mdio_lock);
> +
> +	return err < 0 ? err : 0;
> +}
> +EXPORT_SYMBOL_GPL(mdiobus_modify);
> +

  Andrew