Re: [PATCH net-next v3 2/3] net: phy: dp83869: support Wake on LAN

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 3 Sep 2020 06:42:58 -0500 Dan Murphy wrote:
> This adds WoL support on TI DP83869 for magic, magic secure, unicast and
> broadcast.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  drivers/net/phy/dp83869.c | 128 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 128 insertions(+)
> 
> diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
> index 48a68474f89c..5045df9515a5 100644
> --- a/drivers/net/phy/dp83869.c
> +++ b/drivers/net/phy/dp83869.c
> @@ -4,6 +4,7 @@
>   */
>  
>  #include <linux/ethtool.h>
> +#include <linux/etherdevice.h>
>  #include <linux/kernel.h>
>  #include <linux/mii.h>
>  #include <linux/module.h>
> @@ -27,6 +28,13 @@
>  #define DP83869_RGMIICTL	0x0032
>  #define DP83869_STRAP_STS1	0x006e
>  #define DP83869_RGMIIDCTL	0x0086
> +#define DP83869_RXFCFG		0x0134
> +#define DP83869_RXFPMD1		0x0136
> +#define DP83869_RXFPMD2		0x0137
> +#define DP83869_RXFPMD3		0x0138
> +#define DP83869_RXFSOP1		0x0139
> +#define DP83869_RXFSOP2		0x013A
> +#define DP83869_RXFSOP3		0x013B
>  #define DP83869_IO_MUX_CFG	0x0170
>  #define DP83869_OP_MODE		0x01df
>  #define DP83869_FX_CTRL		0x0c00
> @@ -105,6 +113,14 @@
>  #define DP83869_OP_MODE_MII			BIT(5)
>  #define DP83869_SGMII_RGMII_BRIDGE		BIT(6)
>  
> +/* RXFCFG bits*/
> +#define DP83869_WOL_MAGIC_EN		BIT(0)
> +#define DP83869_WOL_PATTERN_EN		BIT(1)
> +#define DP83869_WOL_BCAST_EN		BIT(2)
> +#define DP83869_WOL_UCAST_EN		BIT(4)
> +#define DP83869_WOL_SEC_EN		BIT(5)
> +#define DP83869_WOL_ENH_MAC		BIT(7)
> +
>  enum {
>  	DP83869_PORT_MIRRORING_KEEP,
>  	DP83869_PORT_MIRRORING_EN,
> @@ -156,6 +172,115 @@ static int dp83869_config_intr(struct phy_device *phydev)
>  	return phy_write(phydev, MII_DP83869_MICR, micr_status);
>  }
>  
> +static int dp83869_set_wol(struct phy_device *phydev,
> +			   struct ethtool_wolinfo *wol)
> +{
> +	struct net_device *ndev = phydev->attached_dev;
> +	u16 val_rxcfg, val_micr;
> +	u8 *mac;
> +
> +	val_rxcfg = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_RXFCFG);
> +	val_micr = phy_read(phydev, MII_DP83869_MICR);

In the previous patch you checked if phy_read() failed, here you don't.

> +	if (wol->wolopts & (WAKE_MAGIC | WAKE_MAGICSECURE | WAKE_UCAST |
> +			    WAKE_BCAST)) {
> +		val_rxcfg |= DP83869_WOL_ENH_MAC;
> +		val_micr |= MII_DP83869_MICR_WOL_INT_EN;
> +
> +		if (wol->wolopts & WAKE_MAGIC) {
> +			mac = (u8 *)ndev->dev_addr;
> +
> +			if (!is_valid_ether_addr(mac))
> +				return -EINVAL;
> +
> +			phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RXFPMD1,
> +				      (mac[1] << 8 | mac[0]));

parenthesis unnecessary

> +			phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RXFPMD2,
> +				      (mac[3] << 8 | mac[2]));
> +			phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RXFPMD3,
> +				      (mac[5] << 8 | mac[4]));

Why only program mac addr for wake_magic, does magic_secure or unicast
not require it?

> +
> +			val_rxcfg |= DP83869_WOL_MAGIC_EN;
> +		} else {
> +			val_rxcfg &= ~DP83869_WOL_MAGIC_EN;
> +		}
> +
> +		if (wol->wolopts & WAKE_MAGICSECURE) {
> +			phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RXFSOP1,
> +				      (wol->sopass[1] << 8) | wol->sopass[0]);
> +			phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RXFSOP2,
> +				      (wol->sopass[3] << 8) | wol->sopass[2]);
> +			phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RXFSOP3,
> +				      (wol->sopass[5] << 8) | wol->sopass[4]);
> +
> +			val_rxcfg |= DP83869_WOL_SEC_EN;
> +		} else {
> +			val_rxcfg &= ~DP83869_WOL_SEC_EN;
> +		}
> +
> +		if (wol->wolopts & WAKE_UCAST)
> +			val_rxcfg |= DP83869_WOL_UCAST_EN;
> +		else
> +			val_rxcfg &= ~DP83869_WOL_UCAST_EN;
> +
> +		if (wol->wolopts & WAKE_BCAST)
> +			val_rxcfg |= DP83869_WOL_BCAST_EN;
> +		else
> +			val_rxcfg &= ~DP83869_WOL_BCAST_EN;
> +	} else {
> +		val_rxcfg &= ~DP83869_WOL_ENH_MAC;
> +		val_micr &= ~MII_DP83869_MICR_WOL_INT_EN;
> +	}
> +
> +	phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RXFCFG, val_rxcfg);
> +	phy_write(phydev, MII_DP83869_MICR, val_micr);
> +
> +	return 0;
> +}
> +
> +static void dp83869_get_wol(struct phy_device *phydev,
> +			    struct ethtool_wolinfo *wol)
> +{
> +	u16 value, sopass_val;
> +
> +	wol->supported = (WAKE_UCAST | WAKE_BCAST | WAKE_MAGIC |
> +			WAKE_MAGICSECURE);
> +	wol->wolopts = 0;
> +
> +	value = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_RXFCFG);
> +
> +	if (value & DP83869_WOL_UCAST_EN)
> +		wol->wolopts |= WAKE_UCAST;
> +
> +	if (value & DP83869_WOL_BCAST_EN)
> +		wol->wolopts |= WAKE_BCAST;
> +
> +	if (value & DP83869_WOL_MAGIC_EN)
> +		wol->wolopts |= WAKE_MAGIC;
> +
> +	if (value & DP83869_WOL_SEC_EN) {
> +		sopass_val = phy_read_mmd(phydev, DP83869_DEVADDR,
> +					  DP83869_RXFSOP1);
> +		wol->sopass[0] = (sopass_val & 0xff);
> +		wol->sopass[1] = (sopass_val >> 8);
> +
> +		sopass_val = phy_read_mmd(phydev, DP83869_DEVADDR,
> +					  DP83869_RXFSOP2);
> +		wol->sopass[2] = (sopass_val & 0xff);
> +		wol->sopass[3] = (sopass_val >> 8);
> +
> +		sopass_val = phy_read_mmd(phydev, DP83869_DEVADDR,
> +					  DP83869_RXFSOP3);
> +		wol->sopass[4] = (sopass_val & 0xff);
> +		wol->sopass[5] = (sopass_val >> 8);
> +
> +		wol->wolopts |= WAKE_MAGICSECURE;
> +	}
> +
> +	if (!(value & DP83869_WOL_ENH_MAC))
> +		wol->wolopts = 0;

What does ENH stand for?

Perhaps it would be cleaner to make a helper like this:

u32 helper(u16 rxfsop1)
{
	u32 wolopts;

	if (!(value & DP83869_WOL_ENH_MAC))
		return 0;

	if (value & DP83869_WOL_UCAST_EN)
		wolopts |= WAKE_UCAST;
	if (value & DP83869_WOL_BCAST_EN)
		wolopts |= WAKE_BCAST;
	if (value & DP83869_WOL_MAGIC_EN)
		wolopts |= WAKE_MAGIC;
	if (value & DP83869_WOL_SEC_EN)
		wolopts |= WAKE_MAGICSECURE;

	return wolopts;
}

wol->wolopts = helper(value);

setting the bits and then clearing the value looks strange.

> +}
> +
>  static int dp83869_config_port_mirroring(struct phy_device *phydev)
>  {
>  	struct dp83869_private *dp83869 = phydev->priv;

Overall this code looks quite similar to dp83867, is there no way to
factor this out?