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

["Frank.Sae" <Frank.Sae@motor-comm.com>]

Hi Andrew,

On 2023/1/28 23:42, Andrew Lunn wrote:
> 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.

I have never seen that happen.
Do you mean that I should change the code from

+	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;

to

+	if (wol->wolopts & WAKE_MAGIC) {
+		mac_addr = phydev->attached_dev->dev_addr;

?

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

The PHY actually is not care this. The firmware is not crash if given a
bad MAC address.

>     Andrew