[PATCH net v2 0/2] WoL fixes for DP83822 and DP83tc811

[PATCH net v2 0/2] WoL fixes for DP83822 and DP83tc811

[Dan Murphy]

Hello

The WoL feature for each device was enabled during boot or when the PHY was
brought up which may be undesired.  These patches disable the WoL in the
config_init.  The disabling and enabling of the WoL is now done though the
set_wol call.

Dan

Dan Murphy (2):
  net: phy: DP83822: Fix WoL in config init to be disabled
  net: phy: DP83TC811: Fix WoL in config init to be disabled

 drivers/net/phy/dp83822.c   | 30 ++++++++++++++----------------
 drivers/net/phy/dp83tc811.c | 21 ++++++++++++---------
 2 files changed, 26 insertions(+), 25 deletions(-)

-- 
2.25.1

[PATCH net v2 1/2] net: phy: DP83822: Fix WoL in config init to be disabled

[Dan Murphy]

The WoL feature should be disabled when config_init is called and the
feature should turned on or off  when set_wol is called.

In addition updated the calls to modify the registers to use the set_bit
and clear_bit function calls.

Fixes: 3b427751a9d0 ("net: phy: DP83822 initial driver submission")
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/net/phy/dp83822.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
index fe9aa3ad52a7..1dd19d0cb269 100644
--- a/drivers/net/phy/dp83822.c
+++ b/drivers/net/phy/dp83822.c
@@ -137,19 +137,18 @@ static int dp83822_set_wol(struct phy_device *phydev,
 			value &= ~DP83822_WOL_SECURE_ON;
 		}
 
-		value |= (DP83822_WOL_EN | DP83822_WOL_INDICATION_SEL |
-			  DP83822_WOL_CLR_INDICATION);
-		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,
-			      value);
+		/* Clear any pending WoL interrupt */
+		phy_read(phydev, MII_DP83822_MISR2);
+
+		value |= DP83822_WOL_EN | DP83822_WOL_INDICATION_SEL |
+			 DP83822_WOL_CLR_INDICATION;
+
+		return phy_write_mmd(phydev, DP83822_DEVADDR,
+				     MII_DP83822_WOL_CFG, value);
 	} else {
-		value = phy_read_mmd(phydev, DP83822_DEVADDR,
-				     MII_DP83822_WOL_CFG);
-		value &= ~DP83822_WOL_EN;
-		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,
-			      value);
+		return phy_clear_bits_mmd(phydev, DP83822_DEVADDR,
+					  MII_DP83822_WOL_CFG, DP83822_WOL_EN);
 	}
-
-	return 0;
 }
 
 static void dp83822_get_wol(struct phy_device *phydev,
@@ -258,12 +257,11 @@ static int dp83822_config_intr(struct phy_device *phydev)
 
 static int dp83822_config_init(struct phy_device *phydev)
 {
-	int value;
-
-	value = DP83822_WOL_MAGIC_EN | DP83822_WOL_SECURE_ON | DP83822_WOL_EN;
+	int value = DP83822_WOL_EN | DP83822_WOL_MAGIC_EN |
+		    DP83822_WOL_SECURE_ON;
 
-	return phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,
-	      value);
+	return phy_clear_bits_mmd(phydev, DP83822_DEVADDR,
+				  MII_DP83822_WOL_CFG, value);
 }
 
 static int dp83822_phy_reset(struct phy_device *phydev)
-- 
2.25.1

[PATCH net v2 2/2] net: phy: DP83TC811: Fix WoL in config init to be disabled

[Dan Murphy]

The WoL feature should be disabled when config_init is called and the
feature should turned on or off  when set_wol is called.

In addition updated the calls to modify the registers to use the set_bit
and clear_bit function calls.

Fixes: 6d749428788b ("net: phy: DP83TC811: Introduce support for the
DP83TC811 phy")
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/net/phy/dp83tc811.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/dp83tc811.c b/drivers/net/phy/dp83tc811.c
index 06f08832ebcd..ff325fb748b9 100644
--- a/drivers/net/phy/dp83tc811.c
+++ b/drivers/net/phy/dp83tc811.c
@@ -139,16 +139,19 @@ static int dp83811_set_wol(struct phy_device *phydev,
 			value &= ~DP83811_WOL_SECURE_ON;
 		}
 
-		value |= (DP83811_WOL_EN | DP83811_WOL_INDICATION_SEL |
-			  DP83811_WOL_CLR_INDICATION);
-		phy_write_mmd(phydev, DP83811_DEVADDR, MII_DP83811_WOL_CFG,
-			      value);
+		/* Clear any pending WoL interrupt */
+		phy_read(phydev, MII_DP83811_INT_STAT1);
+
+		value |= DP83811_WOL_EN | DP83811_WOL_INDICATION_SEL |
+			 DP83811_WOL_CLR_INDICATION;
+
+		return phy_write_mmd(phydev, DP83822_DEVADDR,
+				     MII_DP83811_WOL_CFG, value);
 	} else {
-		phy_clear_bits_mmd(phydev, DP83811_DEVADDR, MII_DP83811_WOL_CFG,
-				   DP83811_WOL_EN);
+		return phy_clear_bits_mmd(phydev, DP83811_DEVADDR,
+					  MII_DP83811_WOL_CFG, DP83811_WOL_EN);
 	}
 
-	return 0;
 }
 
 static void dp83811_get_wol(struct phy_device *phydev,
@@ -292,8 +295,8 @@ static int dp83811_config_init(struct phy_device *phydev)
 
 	value = DP83811_WOL_MAGIC_EN | DP83811_WOL_SECURE_ON | DP83811_WOL_EN;
 
-	return phy_write_mmd(phydev, DP83811_DEVADDR, MII_DP83811_WOL_CFG,
-	      value);
+	return phy_clear_bits_mmd(phydev, DP83811_DEVADDR, MII_DP83811_WOL_CFG,
+				  value);
 }
 
 static int dp83811_phy_reset(struct phy_device *phydev)
-- 
2.25.1

Re: [PATCH net v2 2/2] net: phy: DP83TC811: Fix WoL in config init to be disabled

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3924 bytes --]

Hi Dan,

I love your patch! Yet something to improve:

[auto build test ERROR on net/master]
[also build test ERROR on linus/master v5.7-rc3 next-20200428]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Dan-Murphy/WoL-fixes-for-DP83822-and-DP83tc811/20200428-130050
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 37255e7a8f470a7d9678be89c18ba15d6ebf43f7
config: x86_64-randconfig-c002-20200428 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project f30416fdde922eaa655934e050026930fefbd260)
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/net/phy/dp83tc811.c:148:32: error: use of undeclared identifier 'DP83822_DEVADDR'
                   return phy_write_mmd(phydev, DP83822_DEVADDR,
                                                ^
   1 error generated.

vim +/DP83822_DEVADDR +148 drivers/net/phy/dp83tc811.c

    96	
    97	static int dp83811_set_wol(struct phy_device *phydev,
    98				   struct ethtool_wolinfo *wol)
    99	{
   100		struct net_device *ndev = phydev->attached_dev;
   101		const u8 *mac;
   102		u16 value;
   103	
   104		if (wol->wolopts & (WAKE_MAGIC | WAKE_MAGICSECURE)) {
   105			mac = (const u8 *)ndev->dev_addr;
   106	
   107			if (!is_valid_ether_addr(mac))
   108				return -EINVAL;
   109	
   110			/* MAC addresses start with byte 5, but stored in mac[0].
   111			 * 811 PHYs store bytes 4|5, 2|3, 0|1
   112			 */
   113			phy_write_mmd(phydev, DP83811_DEVADDR, MII_DP83811_WOL_DA1,
   114				      (mac[1] << 8) | mac[0]);
   115			phy_write_mmd(phydev, DP83811_DEVADDR, MII_DP83811_WOL_DA2,
   116				      (mac[3] << 8) | mac[2]);
   117			phy_write_mmd(phydev, DP83811_DEVADDR, MII_DP83811_WOL_DA3,
   118				      (mac[5] << 8) | mac[4]);
   119	
   120			value = phy_read_mmd(phydev, DP83811_DEVADDR,
   121					     MII_DP83811_WOL_CFG);
   122			if (wol->wolopts & WAKE_MAGIC)
   123				value |= DP83811_WOL_MAGIC_EN;
   124			else
   125				value &= ~DP83811_WOL_MAGIC_EN;
   126	
   127			if (wol->wolopts & WAKE_MAGICSECURE) {
   128				phy_write_mmd(phydev, DP83811_DEVADDR,
   129					      MII_DP83811_RXSOP1,
   130					      (wol->sopass[1] << 8) | wol->sopass[0]);
   131				phy_write_mmd(phydev, DP83811_DEVADDR,
   132					      MII_DP83811_RXSOP2,
   133					      (wol->sopass[3] << 8) | wol->sopass[2]);
   134				phy_write_mmd(phydev, DP83811_DEVADDR,
   135					      MII_DP83811_RXSOP3,
   136					      (wol->sopass[5] << 8) | wol->sopass[4]);
   137				value |= DP83811_WOL_SECURE_ON;
   138			} else {
   139				value &= ~DP83811_WOL_SECURE_ON;
   140			}
   141	
   142			/* Clear any pending WoL interrupt */
   143			phy_read(phydev, MII_DP83811_INT_STAT1);
   144	
   145			value |= DP83811_WOL_EN | DP83811_WOL_INDICATION_SEL |
   146				 DP83811_WOL_CLR_INDICATION;
   147	
 > 148			return phy_write_mmd(phydev, DP83822_DEVADDR,
   149					     MII_DP83811_WOL_CFG, value);
   150		} else {
   151			return phy_clear_bits_mmd(phydev, DP83811_DEVADDR,
   152						  MII_DP83811_WOL_CFG, DP83811_WOL_EN);
   153		}
   154	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36881 bytes --]

Re: [PATCH net v2 2/2] net: phy: DP83TC811: Fix WoL in config init to be disabled

[David Miller]

From: Dan Murphy <dmurphy@ti.com>
Date: Mon, 27 Apr 2020 16:21:12 -0500

> +		return phy_write_mmd(phydev, DP83822_DEVADDR,
                                             ^^^^^^^^^^^^^^^^

Please don't submit patches that have not even had a conversation with
the compiler.

This register define only exists in dp83822.c and you are trying to use
it in dp83tc811.c

If this doesn't compile, how did you do functional testing of this
change?

If you compile tested these changes against a tree other than the 'net'
tree, please don't do that.

Thanks.

Re: [PATCH net v2 2/2] net: phy: DP83TC811: Fix WoL in config init to be disabled

[Dan Murphy]

David

On 4/28/20 3:05 PM, David Miller wrote:
> From: Dan Murphy <dmurphy@ti.com>
> Date: Mon, 27 Apr 2020 16:21:12 -0500
>
>> +		return phy_write_mmd(phydev, DP83822_DEVADDR,
>                                               ^^^^^^^^^^^^^^^^
>
> Please don't submit patches that have not even had a conversation with
> the compiler.
>
> This register define only exists in dp83822.c and you are trying to use
> it in dp83tc811.c
>
> If this doesn't compile, how did you do functional testing of this
> change?

My fault, I thought my defconfig had this turned on to compile as it did 
in v1.

I functionally tested these changes on the DP83822 as they are register 
and feature compatible for WoL

Dan

> If you compile tested these changes against a tree other than the 'net'
> tree, please don't do that.
>
> Thanks.