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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200314215728.GG8622@lunn.ch>
Date:   Sat, 14 Mar 2020 22:57:28 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Russell King <rmk+kernel@...linux.org.uk>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/3] net: mdiobus: add APIs for modifying a MDIO
 device register

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@...linux.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