linux-kernel - Re: [PATCH net-next v2] net: phy: Add driver for Motorcomm yt8531 gigabit ethernet phy

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: <Y4n9T+KGj/hX3C0e@lunn.ch>
Date:   Fri, 2 Dec 2022 14:27:43 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Frank <Frank.Sae@...or-comm.com>
Cc:     Peter Geis <pgwipeout@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, yinghong.zhang@...or-comm.com,
        fei.zhang@...or-comm.com, hua.sun@...or-comm.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2] net: phy: Add driver for Motorcomm yt8531
 gigabit ethernet phy

> +static bool mdio_is_locked(struct phy_device *phydev)
> +{
> +	return mutex_is_locked(&phydev->mdio.bus->mdio_lock);
> +}
> +
> +#define ASSERT_MDIO(phydev) \
> +	WARN_ONCE(!mdio_is_locked(phydev), \
> +		  "MDIO: assertion failed at %s (%d)\n", __FILE__,  __LINE__)
> +

Hi Frank

You are not the only one who gets locking wrong. This could be used in
other drivers. Please add it to include/linux/phy.h,

>  /**
>   * ytphy_read_ext() - read a PHY's extended register
>   * @phydev: a pointer to a &struct phy_device
> @@ -258,6 +271,8 @@ static int ytphy_read_ext(struct phy_device *phydev, u16 regnum)
>  {
>  	int ret;
>  
> +	ASSERT_MDIO(phydev);
> +
>  	ret = __phy_write(phydev, YTPHY_PAGE_SELECT, regnum);
>  	if (ret < 0)
>  		return ret;
> @@ -297,6 +312,8 @@ static int ytphy_write_ext(struct phy_device *phydev, u16 regnum, u16 val)
>  {
>  	int ret;
>  
> +	ASSERT_MDIO(phydev);
> +
>  	ret = __phy_write(phydev, YTPHY_PAGE_SELECT, regnum);
>  	if (ret < 0)
>  		return ret;
> @@ -342,6 +359,8 @@ static int ytphy_modify_ext(struct phy_device *phydev, u16 regnum, u16 mask,
>  {
>  	int ret;
>  
> +	ASSERT_MDIO(phydev);
> +
>  	ret = __phy_write(phydev, YTPHY_PAGE_SELECT, regnum);
>  	if (ret < 0)
>  		return ret;
> @@ -479,6 +498,76 @@ static int ytphy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
>  	return phy_restore_page(phydev, old_page, ret);
>  }

Please make the above one patch, which adds the macro and its
users. There are a couple more below as well.

Did it find any problems in the current code? Any fixes mixed
in here?

Then add yt8531 is another patch.

> +/**
> + * yt8531_set_wol() - turn wake-on-lan on or off
> + * @phydev: a pointer to a &struct phy_device
> + * @wol: a pointer to a &struct ethtool_wolinfo
> + *
> + * returns 0 or negative errno code
> + */
> +static int yt8531_set_wol(struct phy_device *phydev,
> +			  struct ethtool_wolinfo *wol)
> +{
> +	struct net_device *p_attached_dev;
> +	const u16 mac_addr_reg[] = {
> +		YTPHY_WOL_MACADDR2_REG,
> +		YTPHY_WOL_MACADDR1_REG,
> +		YTPHY_WOL_MACADDR0_REG,
> +	};
> +	const u8 *mac_addr;
> +	u16 mask;
> +	u16 val;
> +	int ret;
> +	u8 i;
> +
> +	if (wol->wolopts & WAKE_MAGIC) {
> +		p_attached_dev = phydev->attached_dev;
> +		if (!p_attached_dev)
> +			return -ENODEV;
> +
> +		mac_addr = (const u8 *)p_attached_dev->dev_addr;

Why the cast?

> +		if (!is_valid_ether_addr(mac_addr))
> +			return -EINVAL;

  Andrew