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

From: Andrew Lunn <andrew@lunn.ch>
To: Frank Sae <Frank.Sae@motor-comm.com>
Cc: Peter Geis <pgwipeout@gmail.com>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Russell King <linux@armlinux.org.uk>,
	"David S . Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	xiaogang.fan@motor-comm.com, fei.zhang@motor-comm.com,
	hua.sun@motor-comm.com, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v2 5/5] net: phy: Add driver for Motorcomm yt8531 gigabit ethernet phy
Date: Sat, 28 Jan 2023 16:42:54 +0100	[thread overview]
Message-ID: <Y9VCfkzjHBDjXmet@lunn.ch> (raw)
In-Reply-To: <20230128031314.19752-6-Frank.Sae@motor-comm.com>

On Sat, Jan 28, 2023 at 11:13:14AM +0800, Frank Sae wrote:
>  Add a driver for the motorcomm yt8531 gigabit ethernet phy. We have
>  verified the driver on AM335x platform with yt8531 board. On the
>  board, yt8531 gigabit ethernet phy works in utp mode, RGMII
>  interface, supports 1000M/100M/10M speeds, and wol(magic package).
> 
> Signed-off-by: Frank Sae <Frank.Sae@motor-comm.com>
> ---
>  drivers/net/phy/Kconfig     |   2 +-
>  drivers/net/phy/motorcomm.c | 204 +++++++++++++++++++++++++++++++++++-
>  2 files changed, 203 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index f5df2edc94a5..dc2f7d0b0cd8 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -257,7 +257,7 @@ config MOTORCOMM_PHY
>  	tristate "Motorcomm PHYs"
>  	help
>  	  Enables support for Motorcomm network PHYs.
> -	  Currently supports the YT8511, YT8521, YT8531S Gigabit Ethernet PHYs.
> +	  Currently supports the YT8511, YT8521, YT8531, YT8531S Gigabit Ethernet PHYs.

This is O.K. for now, but when you add the next PHY, please do this in
some other way, because it does not scale. Maybe just say YT85xx?

>  
>  config NATIONAL_PHY
>  	tristate "National Semiconductor PHYs"
> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
> index 9559fc52814f..f1fc912738e0 100644
> --- a/drivers/net/phy/motorcomm.c
> +++ b/drivers/net/phy/motorcomm.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  /*
> - * Motorcomm 8511/8521/8531S PHY driver.
> + * Motorcomm 8511/8521/8531/8531S PHY driver.
>   *
>   * Author: Peter Geis <pgwipeout@gmail.com>
>   * Author: Frank <Frank.Sae@motor-comm.com>
> @@ -14,6 +14,7 @@
>  
>  #define PHY_ID_YT8511		0x0000010a
>  #define PHY_ID_YT8521		0x0000011A
> +#define PHY_ID_YT8531		0x4f51e91b
>  #define PHY_ID_YT8531S		0x4F51E91A
>  
>  /* YT8521/YT8531S Register Overview
> @@ -517,6 +518,68 @@ static int ytphy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
>  	return phy_restore_page(phydev, old_page, ret);
>  }
>  
> +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, 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;
> +		if (!is_valid_ether_addr(mac_addr))
> +			return -EINVAL;

Have you ever seen that happen? It suggests the MAC driver has a bug,
not validating its MAC address.

Also, does the PHY actually care? Will the firmware crash if given a
bad MAC address?

    Andrew