netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs.
@ 2023-05-27  1:39 Tristram.Ha
  2023-05-27 13:56 ` Simon Horman
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: Tristram.Ha @ 2023-05-27  1:39 UTC (permalink / raw)
  To: David S. Miller, Andrew Lunn, Florian Fainelli
  Cc: netdev, UNGLinuxDriver, Tristram Ha

From: Tristram Ha <Tristram.Ha@microchip.com>

Microchip LAN8740/LAN8742 PHYs support basic unicast, broadcast, and
Magic Packet WoL.  They have one pattern filter matching up to 128 bytes
of frame data, which can be used to implement ARP or multicast WoL.

ARP WoL matches ARP request for IPv4 address of the net device using the
PHY.

Multicast WoL matches IPv6 Neighbor Solicitation which is sent when
somebody wants to talk to the net device using IPv6.  This
implementation may not be appropriate and can be changed by users later.

Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
---
 drivers/net/phy/smsc.c  | 297 +++++++++++++++++++++++++++++++++++++++-
 include/linux/smscphy.h |  34 +++++
 2 files changed, 329 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index 692930750215..99c1eb0ab395 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -20,6 +20,11 @@
 #include <linux/of.h>
 #include <linux/phy.h>
 #include <linux/netdevice.h>
+#include <linux/crc16.h>
+#include <linux/etherdevice.h>
+#include <linux/inetdevice.h>
+#include <net/if_inet6.h>
+#include <net/ipv6.h>
 #include <linux/smscphy.h>
 
 /* Vendor-specific PHY Definitions */
@@ -51,6 +56,7 @@ struct smsc_phy_priv {
 	unsigned int edpd_enable:1;
 	unsigned int edpd_mode_set_by_user:1;
 	unsigned int edpd_max_wait_ms;
+	bool wol_arp;
 };
 
 static int smsc_phy_ack_interrupt(struct phy_device *phydev)
@@ -258,6 +264,285 @@ int lan87xx_read_status(struct phy_device *phydev)
 }
 EXPORT_SYMBOL_GPL(lan87xx_read_status);
 
+static int lan874x_phy_config_init(struct phy_device *phydev)
+{
+	u16 val;
+	int rc;
+
+	/* Setup LED2/nINT/nPME pin to function as nPME.  May need user option
+	 * to use LED1/nINT/nPME.
+	 */
+	val = MII_LAN874X_PHY_PME2_SET;
+
+	/* The bits MII_LAN874X_PHY_WOL_PFDA_FR, MII_LAN874X_PHY_WOL_WUFR,
+	 * MII_LAN874X_PHY_WOL_MPR, and MII_LAN874X_PHY_WOL_BCAST_FR need to
+	 * be cleared to de-assert PME signal after a WoL event happens, but
+	 * using PME auto clear gets around that.
+	 */
+	val |= MII_LAN874X_PHY_PME_SELF_CLEAR;
+	rc = phy_write_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR,
+			   val);
+	if (rc < 0)
+		return rc;
+
+	/* set nPME self clear delay time */
+	rc = phy_write_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_MCFGR,
+			   MII_LAN874X_PHY_PME_SELF_CLEAR_DELAY);
+	if (rc < 0)
+		return rc;
+
+	return smsc_phy_config_init(phydev);
+}
+
+static void lan874x_get_wol(struct phy_device *phydev,
+			    struct ethtool_wolinfo *wol)
+{
+	struct smsc_phy_priv *priv = phydev->priv;
+	int rc;
+
+	wol->supported = (WAKE_UCAST | WAKE_BCAST | WAKE_MAGIC |
+			  WAKE_ARP | WAKE_MCAST);
+	wol->wolopts = 0;
+
+	rc = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR);
+	if (rc < 0)
+		return;
+
+	if (rc & MII_LAN874X_PHY_WOL_PFDAEN)
+		wol->wolopts |= WAKE_UCAST;
+
+	if (rc & MII_LAN874X_PHY_WOL_BCSTEN)
+		wol->wolopts |= WAKE_BCAST;
+
+	if (rc & MII_LAN874X_PHY_WOL_MPEN)
+		wol->wolopts |= WAKE_MAGIC;
+
+	if (rc & MII_LAN874X_PHY_WOL_WUEN) {
+		if (priv->wol_arp)
+			wol->wolopts |= WAKE_ARP;
+		else
+			wol->wolopts |= WAKE_MCAST;
+	}
+}
+
+static u16 smsc_crc16(const u8 *buffer, size_t len)
+{
+	return bitrev16(crc16(0xFFFF, buffer, len));
+}
+
+static int lan874x_chk_wol_pattern(const u8 pattern[], const u16 *mask,
+				   u8 len, u8 *data, u8 *datalen)
+{
+	size_t i, j, k;
+	u16 bits;
+
+	i = 0;
+	k = 0;
+	while (len > 0) {
+		bits = *mask;
+		for (j = 0; j < 16; j++, i++, len--) {
+			/* No more pattern. */
+			if (!len) {
+				/* The rest of bitmap is not empty. */
+				if (bits)
+					return i + 1;
+				break;
+			}
+			if (bits & 1)
+				data[k++] = pattern[i];
+			bits >>= 1;
+		}
+		mask++;
+	}
+	*datalen = k;
+	return 0;
+}
+
+static int lan874x_set_wol_pattern(struct phy_device *phydev, u16 val,
+				   const u8 data[], u8 datalen,
+				   const u16 *mask, u8 masklen)
+{
+	u16 crc, reg;
+	int rc;
+
+	val |= MII_LAN874X_PHY_WOL_FILTER_EN;
+	rc = phy_write_mmd(phydev, MDIO_MMD_PCS,
+			   MII_LAN874X_PHY_MMD_WOL_WUF_CFGA, val);
+	if (rc < 0)
+		return rc;
+
+	crc = smsc_crc16(data, datalen);
+	rc = phy_write_mmd(phydev, MDIO_MMD_PCS,
+			   MII_LAN874X_PHY_MMD_WOL_WUF_CFGB, crc);
+	if (rc < 0)
+		return rc;
+
+	masklen = (masklen + 15) & ~0xf;
+	reg = MII_LAN874X_PHY_MMD_WOL_WUF_MASK7;
+	while (masklen >= 16) {
+		rc = phy_write_mmd(phydev, MDIO_MMD_PCS, reg, *mask);
+		if (rc < 0)
+			return rc;
+		reg--;
+		mask++;
+		masklen -= 16;
+	}
+
+	/* Clear out the rest of mask registers. */
+	while (reg != MII_LAN874X_PHY_MMD_WOL_WUF_MASK0) {
+		phy_write_mmd(phydev, MDIO_MMD_PCS, reg, 0);
+		reg--;
+	}
+	return rc;
+}
+
+static int lan874x_set_wol(struct phy_device *phydev,
+			   struct ethtool_wolinfo *wol)
+{
+	struct net_device *ndev = phydev->attached_dev;
+	struct smsc_phy_priv *priv = phydev->priv;
+	u16 val, val_wucsr;
+	u8 data[128];
+	u8 datalen;
+	int rc;
+
+	if (wol->wolopts & WAKE_PHY)
+		return -EOPNOTSUPP;
+
+	/* lan874x has only one WoL filter pattern */
+	if ((wol->wolopts & (WAKE_ARP | WAKE_MCAST)) ==
+	    (WAKE_ARP | WAKE_MCAST)) {
+		phydev_info(phydev,
+			    "lan874x WoL supports one of ARP|MCAST at a time\n");
+		return -EOPNOTSUPP;
+	}
+
+	rc = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR);
+	if (rc < 0)
+		return rc;
+
+	val_wucsr = rc;
+
+	if (wol->wolopts & WAKE_UCAST)
+		val_wucsr |= MII_LAN874X_PHY_WOL_PFDAEN;
+	else
+		val_wucsr &= ~MII_LAN874X_PHY_WOL_PFDAEN;
+
+	if (wol->wolopts & WAKE_BCAST)
+		val_wucsr |= MII_LAN874X_PHY_WOL_BCSTEN;
+	else
+		val_wucsr &= ~MII_LAN874X_PHY_WOL_BCSTEN;
+
+	if (wol->wolopts & WAKE_MAGIC)
+		val_wucsr |= MII_LAN874X_PHY_WOL_MPEN;
+	else
+		val_wucsr &= ~MII_LAN874X_PHY_WOL_MPEN;
+
+	/* Need to use pattern matching */
+	if (wol->wolopts & (WAKE_ARP | WAKE_MCAST))
+		val_wucsr |= MII_LAN874X_PHY_WOL_WUEN;
+	else
+		val_wucsr &= ~MII_LAN874X_PHY_WOL_WUEN;
+
+	if (wol->wolopts & WAKE_ARP) {
+		const u8 *ip_addr =
+			((const u8 *)&((ndev->ip_ptr)->ifa_list)->ifa_address);
+		const u16 mask[3] = { 0xF03F, 0x003F, 0x03C0 };
+		u8 pattern[42] = {
+			0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+			0x08, 0x06,
+			0x00, 0x01, 0x08, 0x00, 0x06, 0x04, 0x00, 0x01,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+			0x00, 0x00, 0x00, 0x00,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+			0x00, 0x00, 0x00, 0x00 };
+		u8 len = 42;
+
+		memcpy(&pattern[38], ip_addr, 4);
+		rc = lan874x_chk_wol_pattern(pattern, mask, len,
+					     data, &datalen);
+		if (rc)
+			phydev_dbg(phydev, "pattern not valid at %d\n", rc);
+
+		/* Need to match broadcast destination address. */
+		val = MII_LAN874X_PHY_WOL_FILTER_BCSTEN;
+		rc = lan874x_set_wol_pattern(phydev, val, data, datalen, mask,
+					     len);
+		if (rc < 0)
+			return rc;
+		priv->wol_arp = true;
+	}
+
+	if (wol->wolopts & WAKE_MCAST) {
+		u8 pattern[6] = { 0x33, 0x33, 0xFF, 0x00, 0x00, 0x00 };
+		u16 mask[1] = { 0x0007 };
+		u8 len = 3;
+
+		/* Try to match IPv6 Neighbor Solicitation. */
+		if (ndev->ip6_ptr) {
+			struct list_head *addr_list =
+				&ndev->ip6_ptr->addr_list;
+			struct inet6_ifaddr *ifa;
+
+			list_for_each_entry(ifa, addr_list, if_list) {
+				if (ifa->scope == IFA_LINK) {
+					memcpy(&pattern[3],
+					       &ifa->addr.in6_u.u6_addr8[13],
+					       3);
+					mask[0] = 0x003F;
+					len = 6;
+					break;
+				}
+			}
+		}
+		rc = lan874x_chk_wol_pattern(pattern, mask, len,
+					     data, &datalen);
+		if (rc)
+			phydev_dbg(phydev, "pattern not valid at %d\n", rc);
+
+		/* Need to match multicast destination address. */
+		val = MII_LAN874X_PHY_WOL_FILTER_MCASTTEN;
+		rc = lan874x_set_wol_pattern(phydev, val, data, datalen, mask,
+					     len);
+		if (rc < 0)
+			return rc;
+		priv->wol_arp = false;
+	}
+
+	if (wol->wolopts & (WAKE_MAGIC | WAKE_UCAST)) {
+		const u8 *mac = (const u8 *)ndev->dev_addr;
+
+		if (!is_valid_ether_addr(mac))
+			return -EINVAL;
+
+		rc = phy_write_mmd(phydev, MDIO_MMD_PCS,
+				   MII_LAN874X_PHY_MMD_WOL_RX_ADDRC,
+				   ((mac[1] << 8) | mac[0]));
+		if (rc < 0)
+			return rc;
+
+		rc = phy_write_mmd(phydev, MDIO_MMD_PCS,
+				   MII_LAN874X_PHY_MMD_WOL_RX_ADDRB,
+				   ((mac[3] << 8) | mac[2]));
+		if (rc < 0)
+			return rc;
+
+		rc = phy_write_mmd(phydev, MDIO_MMD_PCS,
+				   MII_LAN874X_PHY_MMD_WOL_RX_ADDRA,
+				   ((mac[5] << 8) | mac[4]));
+		if (rc < 0)
+			return rc;
+	}
+
+	rc = phy_write_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR,
+			   val_wucsr);
+	if (rc < 0)
+		return rc;
+
+	return 0;
+}
+
 static int smsc_get_sset_count(struct phy_device *phydev)
 {
 	return ARRAY_SIZE(smsc_hw_stats);
@@ -533,7 +818,7 @@ static struct phy_driver smsc_phy_driver[] = {
 
 	/* basic functions */
 	.read_status	= lan87xx_read_status,
-	.config_init	= smsc_phy_config_init,
+	.config_init	= lan874x_phy_config_init,
 	.soft_reset	= smsc_phy_reset,
 
 	/* IRQ related */
@@ -548,6 +833,10 @@ static struct phy_driver smsc_phy_driver[] = {
 	.get_tunable	= smsc_phy_get_tunable,
 	.set_tunable	= smsc_phy_set_tunable,
 
+	/* WoL */
+	.set_wol	= lan874x_set_wol,
+	.get_wol	= lan874x_get_wol,
+
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 }, {
@@ -566,7 +855,7 @@ static struct phy_driver smsc_phy_driver[] = {
 
 	/* basic functions */
 	.read_status	= lan87xx_read_status,
-	.config_init	= smsc_phy_config_init,
+	.config_init	= lan874x_phy_config_init,
 	.soft_reset	= smsc_phy_reset,
 
 	/* IRQ related */
@@ -581,6 +870,10 @@ static struct phy_driver smsc_phy_driver[] = {
 	.get_tunable	= smsc_phy_get_tunable,
 	.set_tunable	= smsc_phy_set_tunable,
 
+	/* WoL */
+	.set_wol	= lan874x_set_wol,
+	.get_wol	= lan874x_get_wol,
+
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 } };
diff --git a/include/linux/smscphy.h b/include/linux/smscphy.h
index e1c88627755a..b876333257bf 100644
--- a/include/linux/smscphy.h
+++ b/include/linux/smscphy.h
@@ -38,4 +38,38 @@ int smsc_phy_set_tunable(struct phy_device *phydev,
 			 struct ethtool_tunable *tuna, const void *data);
 int smsc_phy_probe(struct phy_device *phydev);
 
+#define MII_LAN874X_PHY_MMD_WOL_WUCSR		0x8010
+#define MII_LAN874X_PHY_MMD_WOL_WUF_CFGA	0x8011
+#define MII_LAN874X_PHY_MMD_WOL_WUF_CFGB	0x8012
+#define MII_LAN874X_PHY_MMD_WOL_WUF_MASK0	0x8021
+#define MII_LAN874X_PHY_MMD_WOL_WUF_MASK1	0x8022
+#define MII_LAN874X_PHY_MMD_WOL_WUF_MASK2	0x8023
+#define MII_LAN874X_PHY_MMD_WOL_WUF_MASK3	0x8024
+#define MII_LAN874X_PHY_MMD_WOL_WUF_MASK4	0x8025
+#define MII_LAN874X_PHY_MMD_WOL_WUF_MASK5	0x8026
+#define MII_LAN874X_PHY_MMD_WOL_WUF_MASK6	0x8027
+#define MII_LAN874X_PHY_MMD_WOL_WUF_MASK7	0x8028
+#define MII_LAN874X_PHY_MMD_WOL_RX_ADDRA	0x8061
+#define MII_LAN874X_PHY_MMD_WOL_RX_ADDRB	0x8062
+#define MII_LAN874X_PHY_MMD_WOL_RX_ADDRC	0x8063
+#define MII_LAN874X_PHY_MMD_MCFGR		0x8064
+
+#define MII_LAN874X_PHY_PME1_SET		(2<<13)
+#define MII_LAN874X_PHY_PME2_SET		(2<<11)
+#define MII_LAN874X_PHY_PME_SELF_CLEAR		BIT(9)
+#define MII_LAN874X_PHY_WOL_PFDA_FR		BIT(7)
+#define MII_LAN874X_PHY_WOL_WUFR		BIT(6)
+#define MII_LAN874X_PHY_WOL_MPR			BIT(5)
+#define MII_LAN874X_PHY_WOL_BCAST_FR		BIT(4)
+#define MII_LAN874X_PHY_WOL_PFDAEN		BIT(3)
+#define MII_LAN874X_PHY_WOL_WUEN		BIT(2)
+#define MII_LAN874X_PHY_WOL_MPEN		BIT(1)
+#define MII_LAN874X_PHY_WOL_BCSTEN		BIT(0)
+
+#define MII_LAN874X_PHY_WOL_FILTER_EN		BIT(15)
+#define MII_LAN874X_PHY_WOL_FILTER_MCASTTEN	BIT(9)
+#define MII_LAN874X_PHY_WOL_FILTER_BCSTEN	BIT(8)
+
+#define MII_LAN874X_PHY_PME_SELF_CLEAR_DELAY	0x1000 /* 81 milliseconds */
+
 #endif /* __LINUX_SMSCPHY_H__ */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs.
  2023-05-27  1:39 [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs Tristram.Ha
@ 2023-05-27 13:56 ` Simon Horman
  2023-05-31 22:52   ` Tristram.Ha
  2023-05-27 17:04 ` kernel test robot
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 11+ messages in thread
From: Simon Horman @ 2023-05-27 13:56 UTC (permalink / raw)
  To: Tristram.Ha
  Cc: David S. Miller, Andrew Lunn, Florian Fainelli, netdev, UNGLinuxDriver

On Fri, May 26, 2023 at 06:39:34PM -0700, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
> 
> Microchip LAN8740/LAN8742 PHYs support basic unicast, broadcast, and
> Magic Packet WoL.  They have one pattern filter matching up to 128 bytes
> of frame data, which can be used to implement ARP or multicast WoL.
> 
> ARP WoL matches ARP request for IPv4 address of the net device using the
> PHY.
> 
> Multicast WoL matches IPv6 Neighbor Solicitation which is sent when
> somebody wants to talk to the net device using IPv6.  This
> implementation may not be appropriate and can be changed by users later.
> 
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>

...

> +static int lan874x_set_wol(struct phy_device *phydev,
> +			   struct ethtool_wolinfo *wol)
> +{
> +	struct net_device *ndev = phydev->attached_dev;
> +	struct smsc_phy_priv *priv = phydev->priv;
> +	u16 val, val_wucsr;
> +	u8 data[128];
> +	u8 datalen;
> +	int rc;
> +
> +	if (wol->wolopts & WAKE_PHY)
> +		return -EOPNOTSUPP;
> +
> +	/* lan874x has only one WoL filter pattern */
> +	if ((wol->wolopts & (WAKE_ARP | WAKE_MCAST)) ==
> +	    (WAKE_ARP | WAKE_MCAST)) {
> +		phydev_info(phydev,
> +			    "lan874x WoL supports one of ARP|MCAST at a time\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	rc = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR);
> +	if (rc < 0)
> +		return rc;
> +
> +	val_wucsr = rc;
> +
> +	if (wol->wolopts & WAKE_UCAST)
> +		val_wucsr |= MII_LAN874X_PHY_WOL_PFDAEN;
> +	else
> +		val_wucsr &= ~MII_LAN874X_PHY_WOL_PFDAEN;
> +
> +	if (wol->wolopts & WAKE_BCAST)
> +		val_wucsr |= MII_LAN874X_PHY_WOL_BCSTEN;
> +	else
> +		val_wucsr &= ~MII_LAN874X_PHY_WOL_BCSTEN;
> +
> +	if (wol->wolopts & WAKE_MAGIC)
> +		val_wucsr |= MII_LAN874X_PHY_WOL_MPEN;
> +	else
> +		val_wucsr &= ~MII_LAN874X_PHY_WOL_MPEN;
> +
> +	/* Need to use pattern matching */
> +	if (wol->wolopts & (WAKE_ARP | WAKE_MCAST))
> +		val_wucsr |= MII_LAN874X_PHY_WOL_WUEN;
> +	else
> +		val_wucsr &= ~MII_LAN874X_PHY_WOL_WUEN;
> +
> +	if (wol->wolopts & WAKE_ARP) {
> +		const u8 *ip_addr =
> +			((const u8 *)&((ndev->ip_ptr)->ifa_list)->ifa_address);

Hi Tristram,

Sparse seems unhappy about this:

.../smsc.c:449:27: warning: cast removes address space '__rcu' of expression

> +		const u16 mask[3] = { 0xF03F, 0x003F, 0x03C0 };
> +		u8 pattern[42] = {
> +			0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
> +			0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +			0x08, 0x06,
> +			0x00, 0x01, 0x08, 0x00, 0x06, 0x04, 0x00, 0x01,
> +			0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +			0x00, 0x00, 0x00, 0x00,
> +			0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +			0x00, 0x00, 0x00, 0x00 };
> +		u8 len = 42;
> +
> +		memcpy(&pattern[38], ip_addr, 4);
> +		rc = lan874x_chk_wol_pattern(pattern, mask, len,
> +					     data, &datalen);
> +		if (rc)
> +			phydev_dbg(phydev, "pattern not valid at %d\n", rc);
> +
> +		/* Need to match broadcast destination address. */
> +		val = MII_LAN874X_PHY_WOL_FILTER_BCSTEN;
> +		rc = lan874x_set_wol_pattern(phydev, val, data, datalen, mask,
> +					     len);
> +		if (rc < 0)
> +			return rc;
> +		priv->wol_arp = true;
> +	}
> +
> +	if (wol->wolopts & WAKE_MCAST) {
> +		u8 pattern[6] = { 0x33, 0x33, 0xFF, 0x00, 0x00, 0x00 };
> +		u16 mask[1] = { 0x0007 };
> +		u8 len = 3;
> +
> +		/* Try to match IPv6 Neighbor Solicitation. */
> +		if (ndev->ip6_ptr) {
> +			struct list_head *addr_list =
> +				&ndev->ip6_ptr->addr_list;

And this:

.../smsc.c:485:38: warning: incorrect type in initializer (different address spaces)
.../smsc.c:485:38:    expected struct list_head *addr_list
.../smsc.c:485:38:    got struct list_head [noderef] __rcu *
.../smsc.c:449:45: warning: dereference of noderef expression

Please make sure that patches don't intoduce new warnings with W=1 C=1 builds.

> +			struct inet6_ifaddr *ifa;
> +
> +			list_for_each_entry(ifa, addr_list, if_list) {
> +				if (ifa->scope == IFA_LINK) {
> +					memcpy(&pattern[3],
> +					       &ifa->addr.in6_u.u6_addr8[13],
> +					       3);
> +					mask[0] = 0x003F;
> +					len = 6;
> +					break;
> +				}
> +			}
> +		}
> +		rc = lan874x_chk_wol_pattern(pattern, mask, len,
> +					     data, &datalen);
> +		if (rc)
> +			phydev_dbg(phydev, "pattern not valid at %d\n", rc);
> +
> +		/* Need to match multicast destination address. */
> +		val = MII_LAN874X_PHY_WOL_FILTER_MCASTTEN;
> +		rc = lan874x_set_wol_pattern(phydev, val, data, datalen, mask,
> +					     len);
> +		if (rc < 0)
> +			return rc;
> +		priv->wol_arp = false;
> +	}

...

> diff --git a/include/linux/smscphy.h b/include/linux/smscphy.h
> index e1c88627755a..b876333257bf 100644
> --- a/include/linux/smscphy.h
> +++ b/include/linux/smscphy.h
> @@ -38,4 +38,38 @@ int smsc_phy_set_tunable(struct phy_device *phydev,
>  			 struct ethtool_tunable *tuna, const void *data);
>  int smsc_phy_probe(struct phy_device *phydev);
>  
> +#define MII_LAN874X_PHY_MMD_WOL_WUCSR		0x8010
> +#define MII_LAN874X_PHY_MMD_WOL_WUF_CFGA	0x8011
> +#define MII_LAN874X_PHY_MMD_WOL_WUF_CFGB	0x8012
> +#define MII_LAN874X_PHY_MMD_WOL_WUF_MASK0	0x8021
> +#define MII_LAN874X_PHY_MMD_WOL_WUF_MASK1	0x8022
> +#define MII_LAN874X_PHY_MMD_WOL_WUF_MASK2	0x8023
> +#define MII_LAN874X_PHY_MMD_WOL_WUF_MASK3	0x8024
> +#define MII_LAN874X_PHY_MMD_WOL_WUF_MASK4	0x8025
> +#define MII_LAN874X_PHY_MMD_WOL_WUF_MASK5	0x8026
> +#define MII_LAN874X_PHY_MMD_WOL_WUF_MASK6	0x8027
> +#define MII_LAN874X_PHY_MMD_WOL_WUF_MASK7	0x8028
> +#define MII_LAN874X_PHY_MMD_WOL_RX_ADDRA	0x8061
> +#define MII_LAN874X_PHY_MMD_WOL_RX_ADDRB	0x8062
> +#define MII_LAN874X_PHY_MMD_WOL_RX_ADDRC	0x8063
> +#define MII_LAN874X_PHY_MMD_MCFGR		0x8064
> +
> +#define MII_LAN874X_PHY_PME1_SET		(2<<13)
> +#define MII_LAN874X_PHY_PME2_SET		(2<<11)

nit: Maybe GENMASK is appropriate here.
     If not, please consider spaces around '<<'

> +#define MII_LAN874X_PHY_PME_SELF_CLEAR		BIT(9)
> +#define MII_LAN874X_PHY_WOL_PFDA_FR		BIT(7)
> +#define MII_LAN874X_PHY_WOL_WUFR		BIT(6)
> +#define MII_LAN874X_PHY_WOL_MPR			BIT(5)
> +#define MII_LAN874X_PHY_WOL_BCAST_FR		BIT(4)
> +#define MII_LAN874X_PHY_WOL_PFDAEN		BIT(3)
> +#define MII_LAN874X_PHY_WOL_WUEN		BIT(2)
> +#define MII_LAN874X_PHY_WOL_MPEN		BIT(1)
> +#define MII_LAN874X_PHY_WOL_BCSTEN		BIT(0)
> +
> +#define MII_LAN874X_PHY_WOL_FILTER_EN		BIT(15)
> +#define MII_LAN874X_PHY_WOL_FILTER_MCASTTEN	BIT(9)
> +#define MII_LAN874X_PHY_WOL_FILTER_BCSTEN	BIT(8)
> +
> +#define MII_LAN874X_PHY_PME_SELF_CLEAR_DELAY	0x1000 /* 81 milliseconds */
> +
>  #endif /* __LINUX_SMSCPHY_H__ */

-- 
pw-bot: cr


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs.
  2023-05-27  1:39 [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs Tristram.Ha
  2023-05-27 13:56 ` Simon Horman
@ 2023-05-27 17:04 ` kernel test robot
  2023-05-28  4:50 ` kernel test robot
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2023-05-27 17:04 UTC (permalink / raw)
  To: Tristram.Ha, David S. Miller, Andrew Lunn, Florian Fainelli
  Cc: oe-kbuild-all, netdev, UNGLinuxDriver, Tristram Ha

Hi,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Tristram-Ha-microchip-com/net-phy-smsc-add-WoL-support-to-LAN8740-LAN8742-PHYs/20230527-094102
base:   net-next/main
patch link:    https://lore.kernel.org/r/1685151574-2752-1-git-send-email-Tristram.Ha%40microchip.com
patch subject: [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs.
config: x86_64-randconfig-x076-20230526 (https://download.01.org/0day-ci/archive/20230528/202305280053.LordyDHm-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/a1e40c5a7a32445d5ae4541d4e57bbc4b5065057
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Tristram-Ha-microchip-com/net-phy-smsc-add-WoL-support-to-LAN8740-LAN8742-PHYs/20230527-094102
        git checkout a1e40c5a7a32445d5ae4541d4e57bbc4b5065057
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305280053.LordyDHm-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: drivers/net/phy/smsc.o: in function `smsc_crc16':
>> drivers/net/phy/smsc.c:330: undefined reference to `crc16'


vim +330 drivers/net/phy/smsc.c

   327	
   328	static u16 smsc_crc16(const u8 *buffer, size_t len)
   329	{
 > 330		return bitrev16(crc16(0xFFFF, buffer, len));
   331	}
   332	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs.
  2023-05-27  1:39 [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs Tristram.Ha
  2023-05-27 13:56 ` Simon Horman
  2023-05-27 17:04 ` kernel test robot
@ 2023-05-28  4:50 ` kernel test robot
  2023-05-29 14:48 ` Andrew Lunn
  2023-05-31 23:07 ` Florian Fainelli
  4 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2023-05-28  4:50 UTC (permalink / raw)
  To: Tristram.Ha, David S. Miller, Andrew Lunn, Florian Fainelli
  Cc: oe-kbuild-all, netdev, UNGLinuxDriver, Tristram Ha

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Tristram-Ha-microchip-com/net-phy-smsc-add-WoL-support-to-LAN8740-LAN8742-PHYs/20230527-094102
base:   net-next/main
patch link:    https://lore.kernel.org/r/1685151574-2752-1-git-send-email-Tristram.Ha%40microchip.com
patch subject: [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs.
config: i386-randconfig-s002-20230528 (https://download.01.org/0day-ci/archive/20230528/202305281254.hziqmfSD-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/a1e40c5a7a32445d5ae4541d4e57bbc4b5065057
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Tristram-Ha-microchip-com/net-phy-smsc-add-WoL-support-to-LAN8740-LAN8742-PHYs/20230527-094102
        git checkout a1e40c5a7a32445d5ae4541d4e57bbc4b5065057
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 olddefconfig
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/net/phy/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305281254.hziqmfSD-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/net/phy/smsc.c:449:27: sparse: sparse: cast removes address space '__rcu' of expression
>> drivers/net/phy/smsc.c:485:38: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected struct list_head *addr_list @@     got struct list_head [noderef] __rcu * @@
   drivers/net/phy/smsc.c:485:38: sparse:     expected struct list_head *addr_list
   drivers/net/phy/smsc.c:485:38: sparse:     got struct list_head [noderef] __rcu *
>> drivers/net/phy/smsc.c:449:45: sparse: sparse: dereference of noderef expression

vim +/__rcu +449 drivers/net/phy/smsc.c

   398	
   399	static int lan874x_set_wol(struct phy_device *phydev,
   400				   struct ethtool_wolinfo *wol)
   401	{
   402		struct net_device *ndev = phydev->attached_dev;
   403		struct smsc_phy_priv *priv = phydev->priv;
   404		u16 val, val_wucsr;
   405		u8 data[128];
   406		u8 datalen;
   407		int rc;
   408	
   409		if (wol->wolopts & WAKE_PHY)
   410			return -EOPNOTSUPP;
   411	
   412		/* lan874x has only one WoL filter pattern */
   413		if ((wol->wolopts & (WAKE_ARP | WAKE_MCAST)) ==
   414		    (WAKE_ARP | WAKE_MCAST)) {
   415			phydev_info(phydev,
   416				    "lan874x WoL supports one of ARP|MCAST at a time\n");
   417			return -EOPNOTSUPP;
   418		}
   419	
   420		rc = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR);
   421		if (rc < 0)
   422			return rc;
   423	
   424		val_wucsr = rc;
   425	
   426		if (wol->wolopts & WAKE_UCAST)
   427			val_wucsr |= MII_LAN874X_PHY_WOL_PFDAEN;
   428		else
   429			val_wucsr &= ~MII_LAN874X_PHY_WOL_PFDAEN;
   430	
   431		if (wol->wolopts & WAKE_BCAST)
   432			val_wucsr |= MII_LAN874X_PHY_WOL_BCSTEN;
   433		else
   434			val_wucsr &= ~MII_LAN874X_PHY_WOL_BCSTEN;
   435	
   436		if (wol->wolopts & WAKE_MAGIC)
   437			val_wucsr |= MII_LAN874X_PHY_WOL_MPEN;
   438		else
   439			val_wucsr &= ~MII_LAN874X_PHY_WOL_MPEN;
   440	
   441		/* Need to use pattern matching */
   442		if (wol->wolopts & (WAKE_ARP | WAKE_MCAST))
   443			val_wucsr |= MII_LAN874X_PHY_WOL_WUEN;
   444		else
   445			val_wucsr &= ~MII_LAN874X_PHY_WOL_WUEN;
   446	
   447		if (wol->wolopts & WAKE_ARP) {
   448			const u8 *ip_addr =
 > 449				((const u8 *)&((ndev->ip_ptr)->ifa_list)->ifa_address);
   450			const u16 mask[3] = { 0xF03F, 0x003F, 0x03C0 };
   451			u8 pattern[42] = {
   452				0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
   453				0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
   454				0x08, 0x06,
   455				0x00, 0x01, 0x08, 0x00, 0x06, 0x04, 0x00, 0x01,
   456				0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
   457				0x00, 0x00, 0x00, 0x00,
   458				0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
   459				0x00, 0x00, 0x00, 0x00 };
   460			u8 len = 42;
   461	
   462			memcpy(&pattern[38], ip_addr, 4);
   463			rc = lan874x_chk_wol_pattern(pattern, mask, len,
   464						     data, &datalen);
   465			if (rc)
   466				phydev_dbg(phydev, "pattern not valid at %d\n", rc);
   467	
   468			/* Need to match broadcast destination address. */
   469			val = MII_LAN874X_PHY_WOL_FILTER_BCSTEN;
   470			rc = lan874x_set_wol_pattern(phydev, val, data, datalen, mask,
   471						     len);
   472			if (rc < 0)
   473				return rc;
   474			priv->wol_arp = true;
   475		}
   476	
   477		if (wol->wolopts & WAKE_MCAST) {
   478			u8 pattern[6] = { 0x33, 0x33, 0xFF, 0x00, 0x00, 0x00 };
   479			u16 mask[1] = { 0x0007 };
   480			u8 len = 3;
   481	
   482			/* Try to match IPv6 Neighbor Solicitation. */
   483			if (ndev->ip6_ptr) {
   484				struct list_head *addr_list =
 > 485					&ndev->ip6_ptr->addr_list;
   486				struct inet6_ifaddr *ifa;
   487	
   488				list_for_each_entry(ifa, addr_list, if_list) {
   489					if (ifa->scope == IFA_LINK) {
   490						memcpy(&pattern[3],
   491						       &ifa->addr.in6_u.u6_addr8[13],
   492						       3);
   493						mask[0] = 0x003F;
   494						len = 6;
   495						break;
   496					}
   497				}
   498			}
   499			rc = lan874x_chk_wol_pattern(pattern, mask, len,
   500						     data, &datalen);
   501			if (rc)
   502				phydev_dbg(phydev, "pattern not valid at %d\n", rc);
   503	
   504			/* Need to match multicast destination address. */
   505			val = MII_LAN874X_PHY_WOL_FILTER_MCASTTEN;
   506			rc = lan874x_set_wol_pattern(phydev, val, data, datalen, mask,
   507						     len);
   508			if (rc < 0)
   509				return rc;
   510			priv->wol_arp = false;
   511		}
   512	
   513		if (wol->wolopts & (WAKE_MAGIC | WAKE_UCAST)) {
   514			const u8 *mac = (const u8 *)ndev->dev_addr;
   515	
   516			if (!is_valid_ether_addr(mac))
   517				return -EINVAL;
   518	
   519			rc = phy_write_mmd(phydev, MDIO_MMD_PCS,
   520					   MII_LAN874X_PHY_MMD_WOL_RX_ADDRC,
   521					   ((mac[1] << 8) | mac[0]));
   522			if (rc < 0)
   523				return rc;
   524	
   525			rc = phy_write_mmd(phydev, MDIO_MMD_PCS,
   526					   MII_LAN874X_PHY_MMD_WOL_RX_ADDRB,
   527					   ((mac[3] << 8) | mac[2]));
   528			if (rc < 0)
   529				return rc;
   530	
   531			rc = phy_write_mmd(phydev, MDIO_MMD_PCS,
   532					   MII_LAN874X_PHY_MMD_WOL_RX_ADDRA,
   533					   ((mac[5] << 8) | mac[4]));
   534			if (rc < 0)
   535				return rc;
   536		}
   537	
   538		rc = phy_write_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR,
   539				   val_wucsr);
   540		if (rc < 0)
   541			return rc;
   542	
   543		return 0;
   544	}
   545	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs.
  2023-05-27  1:39 [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs Tristram.Ha
                   ` (2 preceding siblings ...)
  2023-05-28  4:50 ` kernel test robot
@ 2023-05-29 14:48 ` Andrew Lunn
  2023-05-31 22:43   ` Tristram.Ha
  2023-05-31 23:07 ` Florian Fainelli
  4 siblings, 1 reply; 11+ messages in thread
From: Andrew Lunn @ 2023-05-29 14:48 UTC (permalink / raw)
  To: Tristram.Ha; +Cc: David S. Miller, Florian Fainelli, netdev, UNGLinuxDriver

> +	if (wol->wolopts & WAKE_ARP) {
> +		const u8 *ip_addr =
> +			((const u8 *)&((ndev->ip_ptr)->ifa_list)->ifa_address);

I'm not sure this is safe. What happens when the interface only has an
IPv6 address? Is ifa_list a NULL pointer? I really think you need to
be using a core helper to get the IPv4 address.

> +		const u16 mask[3] = { 0xF03F, 0x003F, 0x03C0 };

Are there any endianness issues here? I've not looked at how mask is
used, but if it is indicating which bytes in the pattern should be
matched on, i guess endian does matter.

> +		u8 pattern[42] = {
> +			0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
> +			0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +			0x08, 0x06,
> +			0x00, 0x01, 0x08, 0x00, 0x06, 0x04, 0x00, 0x01,
> +			0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +			0x00, 0x00, 0x00, 0x00,
> +			0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +			0x00, 0x00, 0x00, 0x00 };


> +	if (wol->wolopts & WAKE_MCAST) {
> +		u8 pattern[6] = { 0x33, 0x33, 0xFF, 0x00, 0x00, 0x00 };
> +		u16 mask[1] = { 0x0007 };
> +		u8 len = 3;
> +
> +		/* Try to match IPv6 Neighbor Solicitation. */
> +		if (ndev->ip6_ptr) {
> +			struct list_head *addr_list =
> +				&ndev->ip6_ptr->addr_list;
> +			struct inet6_ifaddr *ifa;
> +
> +			list_for_each_entry(ifa, addr_list, if_list) {
> +				if (ifa->scope == IFA_LINK) {
> +					memcpy(&pattern[3],
> +					       &ifa->addr.in6_u.u6_addr8[13],
> +					       3);
> +					mask[0] = 0x003F;
> +					len = 6;
> +					break;
> +				}
> +			}
> +		}

From an architecture point of view, i don't think a PHY driver should
be access these data structure directly. See if ipv6_get_lladdr() does
what you need?

> +	if (wol->wolopts & (WAKE_MAGIC | WAKE_UCAST)) {
> +		const u8 *mac = (const u8 *)ndev->dev_addr;
> +
> +		if (!is_valid_ether_addr(mac))
> +			return -EINVAL;

Is that possible? Does the hardware care?

   Andrew

---
pw-bot: cr

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs.
  2023-05-29 14:48 ` Andrew Lunn
@ 2023-05-31 22:43   ` Tristram.Ha
  2023-05-31 23:10     ` Andrew Lunn
  0 siblings, 1 reply; 11+ messages in thread
From: Tristram.Ha @ 2023-05-31 22:43 UTC (permalink / raw)
  To: andrew; +Cc: davem, f.fainelli, netdev, UNGLinuxDriver

> Subject: Re: [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742
> PHYs.
> 
> > +     if (wol->wolopts & WAKE_ARP) {
> > +             const u8 *ip_addr =
> > +                     ((const u8 *)&((ndev->ip_ptr)->ifa_list)->ifa_address);
> 
> I'm not sure this is safe. What happens when the interface only has an
> IPv6 address? Is ifa_list a NULL pointer? I really think you need to
> be using a core helper to get the IPv4 address.
> 

This will be fixed with in_dev_get() and rcu_dereference().
Indeed the address will disappear when the link is down.
Why is that so?

> > +             const u16 mask[3] = { 0xF03F, 0x003F, 0x03C0 };
> 
> Are there any endianness issues here? I've not looked at how mask is
> used, but if it is indicating which bytes in the pattern should be
> matched on, i guess endian does matter.

These values eventually are programmed into MMD registers.
There are 8 for 128-bit value.  Bit 0 in the last register is byte 0.
I do not see PHY register values are checked for endianness.

> > +             u8 pattern[42] = {
> > +                     0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
> > +                     0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +                     0x08, 0x06,
> > +                     0x00, 0x01, 0x08, 0x00, 0x06, 0x04, 0x00, 0x01,
> > +                     0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +                     0x00, 0x00, 0x00, 0x00,
> > +                     0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +                     0x00, 0x00, 0x00, 0x00 };
> 
> 
> > +     if (wol->wolopts & WAKE_MCAST) {
> > +             u8 pattern[6] = { 0x33, 0x33, 0xFF, 0x00, 0x00, 0x00 };
> > +             u16 mask[1] = { 0x0007 };
> > +             u8 len = 3;
> > +
> > +             /* Try to match IPv6 Neighbor Solicitation. */
> > +             if (ndev->ip6_ptr) {
> > +                     struct list_head *addr_list =
> > +                             &ndev->ip6_ptr->addr_list;
> > +                     struct inet6_ifaddr *ifa;
> > +
> > +                     list_for_each_entry(ifa, addr_list, if_list) {
> > +                             if (ifa->scope == IFA_LINK) {
> > +                                     memcpy(&pattern[3],
> > +                                            &ifa->addr.in6_u.u6_addr8[13],
> > +                                            3);
> > +                                     mask[0] = 0x003F;
> > +                                     len = 6;
> > +                                     break;
> > +                             }
> > +                     }
> > +             }
> 
> From an architecture point of view, i don't think a PHY driver should
> be access these data structure directly. See if ipv6_get_lladdr() does
> what you need?

ipv6_get_lladdr() is not exported so cannot be used when building the
PHY driver as a module.
The WAKE_ARP and WAKE_MCAST code just want a regular IPv4 address
and an IPv6 address as shown by ifconfig command.
They are more like an example to show how the hardware pattern filtering
is done.

> > +     if (wol->wolopts & (WAKE_MAGIC | WAKE_UCAST)) {
> > +             const u8 *mac = (const u8 *)ndev->dev_addr;
> > +
> > +             if (!is_valid_ether_addr(mac))
> > +                     return -EINVAL;
>
> Is that possible? Does the hardware care?
>

Removed.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs.
  2023-05-27 13:56 ` Simon Horman
@ 2023-05-31 22:52   ` Tristram.Ha
  2023-06-01  8:46     ` Simon Horman
  0 siblings, 1 reply; 11+ messages in thread
From: Tristram.Ha @ 2023-05-31 22:52 UTC (permalink / raw)
  To: simon.horman; +Cc: davem, andrew, f.fainelli, netdev, UNGLinuxDriver

> > +     if (wol->wolopts & WAKE_ARP) {
> > +             const u8 *ip_addr =
> > +                     ((const u8 *)&((ndev->ip_ptr)->ifa_list)->ifa_address);
> 
> Hi Tristram,
> 
> Sparse seems unhappy about this:
> 
> .../smsc.c:449:27: warning: cast removes address space '__rcu' of expression
> 

This will be fixed with in_dev_get() and rcu_dereference().

> > +             /* Try to match IPv6 Neighbor Solicitation. */
> > +             if (ndev->ip6_ptr) {
> > +                     struct list_head *addr_list =
> > +                             &ndev->ip6_ptr->addr_list;
> 
> And this:
> 
> .../smsc.c:485:38: warning: incorrect type in initializer (different address spaces)
> .../smsc.c:485:38:    expected struct list_head *addr_list
> .../smsc.c:485:38:    got struct list_head [noderef] __rcu *
> .../smsc.c:449:45: warning: dereference of noderef expression
> 
> Please make sure that patches don't intoduce new warnings with W=1 C=1 builds.

This will be fixed with in6_dev_get().

> > +#define MII_LAN874X_PHY_PME1_SET             (2<<13)
> > +#define MII_LAN874X_PHY_PME2_SET             (2<<11)
> 
> nit: Maybe GENMASK is appropriate here.
>      If not, please consider spaces around '<<'

Will update.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs.
  2023-05-27  1:39 [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs Tristram.Ha
                   ` (3 preceding siblings ...)
  2023-05-29 14:48 ` Andrew Lunn
@ 2023-05-31 23:07 ` Florian Fainelli
  2023-06-01 22:53   ` Tristram.Ha
  4 siblings, 1 reply; 11+ messages in thread
From: Florian Fainelli @ 2023-05-31 23:07 UTC (permalink / raw)
  To: Tristram.Ha, David S. Miller, Andrew Lunn; +Cc: netdev, UNGLinuxDriver

On 5/26/23 18:39, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
> 
> Microchip LAN8740/LAN8742 PHYs support basic unicast, broadcast, and
> Magic Packet WoL.  They have one pattern filter matching up to 128 bytes
> of frame data, which can be used to implement ARP or multicast WoL.
> 
> ARP WoL matches ARP request for IPv4 address of the net device using the
> PHY.
> 
> Multicast WoL matches IPv6 Neighbor Solicitation which is sent when
> somebody wants to talk to the net device using IPv6.  This
> implementation may not be appropriate and can be changed by users later.
> 
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> ---
>   drivers/net/phy/smsc.c  | 297 +++++++++++++++++++++++++++++++++++++++-
>   include/linux/smscphy.h |  34 +++++
>   2 files changed, 329 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> index 692930750215..99c1eb0ab395 100644
> --- a/drivers/net/phy/smsc.c
> +++ b/drivers/net/phy/smsc.c
> @@ -20,6 +20,11 @@
>   #include <linux/of.h>
>   #include <linux/phy.h>
>   #include <linux/netdevice.h>
> +#include <linux/crc16.h>
> +#include <linux/etherdevice.h>
> +#include <linux/inetdevice.h>
> +#include <net/if_inet6.h>
> +#include <net/ipv6.h>
>   #include <linux/smscphy.h>
>   
>   /* Vendor-specific PHY Definitions */
> @@ -51,6 +56,7 @@ struct smsc_phy_priv {
>   	unsigned int edpd_enable:1;
>   	unsigned int edpd_mode_set_by_user:1;
>   	unsigned int edpd_max_wait_ms;
> +	bool wol_arp;
>   };
>   
>   static int smsc_phy_ack_interrupt(struct phy_device *phydev)
> @@ -258,6 +264,285 @@ int lan87xx_read_status(struct phy_device *phydev)
>   }
>   EXPORT_SYMBOL_GPL(lan87xx_read_status);
>   
> +static int lan874x_phy_config_init(struct phy_device *phydev)
> +{
> +	u16 val;
> +	int rc;
> +
> +	/* Setup LED2/nINT/nPME pin to function as nPME.  May need user option
> +	 * to use LED1/nINT/nPME.
> +	 */
> +	val = MII_LAN874X_PHY_PME2_SET;
> +
> +	/* The bits MII_LAN874X_PHY_WOL_PFDA_FR, MII_LAN874X_PHY_WOL_WUFR,
> +	 * MII_LAN874X_PHY_WOL_MPR, and MII_LAN874X_PHY_WOL_BCAST_FR need to
> +	 * be cleared to de-assert PME signal after a WoL event happens, but
> +	 * using PME auto clear gets around that.
> +	 */
> +	val |= MII_LAN874X_PHY_PME_SELF_CLEAR;
> +	rc = phy_write_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR,
> +			   val);
> +	if (rc < 0)
> +		return rc;
> +
> +	/* set nPME self clear delay time */
> +	rc = phy_write_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_MCFGR,
> +			   MII_LAN874X_PHY_PME_SELF_CLEAR_DELAY);
> +	if (rc < 0)
> +		return rc;
> +
> +	return smsc_phy_config_init(phydev);
> +}
> +
> +static void lan874x_get_wol(struct phy_device *phydev,
> +			    struct ethtool_wolinfo *wol)
> +{
> +	struct smsc_phy_priv *priv = phydev->priv;
> +	int rc;
> +
> +	wol->supported = (WAKE_UCAST | WAKE_BCAST | WAKE_MAGIC |
> +			  WAKE_ARP | WAKE_MCAST);
> +	wol->wolopts = 0;
> +
> +	rc = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR);
> +	if (rc < 0)
> +		return;
> +
> +	if (rc & MII_LAN874X_PHY_WOL_PFDAEN)
> +		wol->wolopts |= WAKE_UCAST;
> +
> +	if (rc & MII_LAN874X_PHY_WOL_BCSTEN)
> +		wol->wolopts |= WAKE_BCAST;
> +
> +	if (rc & MII_LAN874X_PHY_WOL_MPEN)
> +		wol->wolopts |= WAKE_MAGIC;
> +
> +	if (rc & MII_LAN874X_PHY_WOL_WUEN) {
> +		if (priv->wol_arp)
> +			wol->wolopts |= WAKE_ARP;
> +		else
> +			wol->wolopts |= WAKE_MCAST;
> +	}
> +}
> +
> +static u16 smsc_crc16(const u8 *buffer, size_t len)
> +{
> +	return bitrev16(crc16(0xFFFF, buffer, len));
> +}
> +
> +static int lan874x_chk_wol_pattern(const u8 pattern[], const u16 *mask,
> +				   u8 len, u8 *data, u8 *datalen)
> +{
> +	size_t i, j, k;
> +	u16 bits;
> +
> +	i = 0;
> +	k = 0;
> +	while (len > 0) {
> +		bits = *mask;
> +		for (j = 0; j < 16; j++, i++, len--) {
> +			/* No more pattern. */
> +			if (!len) {
> +				/* The rest of bitmap is not empty. */
> +				if (bits)
> +					return i + 1;
> +				break;
> +			}
> +			if (bits & 1)
> +				data[k++] = pattern[i];
> +			bits >>= 1;
> +		}
> +		mask++;
> +	}
> +	*datalen = k;
> +	return 0;
> +}
> +
> +static int lan874x_set_wol_pattern(struct phy_device *phydev, u16 val,
> +				   const u8 data[], u8 datalen,
> +				   const u16 *mask, u8 masklen)
> +{
> +	u16 crc, reg;
> +	int rc;
> +
> +	val |= MII_LAN874X_PHY_WOL_FILTER_EN;
> +	rc = phy_write_mmd(phydev, MDIO_MMD_PCS,
> +			   MII_LAN874X_PHY_MMD_WOL_WUF_CFGA, val);
> +	if (rc < 0)
> +		return rc;
> +
> +	crc = smsc_crc16(data, datalen);
> +	rc = phy_write_mmd(phydev, MDIO_MMD_PCS,
> +			   MII_LAN874X_PHY_MMD_WOL_WUF_CFGB, crc);
> +	if (rc < 0)
> +		return rc;
> +
> +	masklen = (masklen + 15) & ~0xf;
> +	reg = MII_LAN874X_PHY_MMD_WOL_WUF_MASK7;
> +	while (masklen >= 16) {
> +		rc = phy_write_mmd(phydev, MDIO_MMD_PCS, reg, *mask);
> +		if (rc < 0)
> +			return rc;
> +		reg--;
> +		mask++;
> +		masklen -= 16;
> +	}
> +
> +	/* Clear out the rest of mask registers. */
> +	while (reg != MII_LAN874X_PHY_MMD_WOL_WUF_MASK0) {
> +		phy_write_mmd(phydev, MDIO_MMD_PCS, reg, 0);
> +		reg--;
> +	}
> +	return rc;
> +}
> +
> +static int lan874x_set_wol(struct phy_device *phydev,
> +			   struct ethtool_wolinfo *wol)
> +{
> +	struct net_device *ndev = phydev->attached_dev;
> +	struct smsc_phy_priv *priv = phydev->priv;
> +	u16 val, val_wucsr;
> +	u8 data[128];
> +	u8 datalen;
> +	int rc;
> +
> +	if (wol->wolopts & WAKE_PHY)
> +		return -EOPNOTSUPP;
> +
> +	/* lan874x has only one WoL filter pattern */
> +	if ((wol->wolopts & (WAKE_ARP | WAKE_MCAST)) ==
> +	    (WAKE_ARP | WAKE_MCAST)) {
> +		phydev_info(phydev,
> +			    "lan874x WoL supports one of ARP|MCAST at a time\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	rc = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR);
> +	if (rc < 0)
> +		return rc;
> +
> +	val_wucsr = rc;

You need to take into account the case where an user wants to disable 
Wake-on-LAN entirely, e.g.:

ethtool -s <iface> wol d

[snip]

> +
> +	if (wol->wolopts & WAKE_UCAST)
> +		val_wucsr |= MII_LAN874X_PHY_WOL_PFDAEN;
> +	else
> +		val_wucsr &= ~MII_LAN874X_PHY_WOL_PFDAEN;
> +
> +	if (wol->wolopts & WAKE_BCAST)
> +		val_wucsr |= MII_LAN874X_PHY_WOL_BCSTEN;
> +	else
> +		val_wucsr &= ~MII_LAN874X_PHY_WOL_BCSTEN;
> +
> +	if (wol->wolopts & WAKE_MAGIC)
> +		val_wucsr |= MII_LAN874X_PHY_WOL_MPEN;
> +	else
> +		val_wucsr &= ~MII_LAN874X_PHY_WOL_MPEN;
> +
> +	/* Need to use pattern matching */
> +	if (wol->wolopts & (WAKE_ARP | WAKE_MCAST))
> +		val_wucsr |= MII_LAN874X_PHY_WOL_WUEN;
> +	else
> +		val_wucsr &= ~MII_LAN874X_PHY_WOL_WUEN;
> +
> +	if (wol->wolopts & WAKE_ARP) {
> +		const u8 *ip_addr =
> +			((const u8 *)&((ndev->ip_ptr)->ifa_list)->ifa_address);
> +		const u16 mask[3] = { 0xF03F, 0x003F, 0x03C0 };
> +		u8 pattern[42] = {
> +			0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
> +			0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +			0x08, 0x06,
> +			0x00, 0x01, 0x08, 0x00, 0x06, 0x04, 0x00, 0x01,
> +			0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +			0x00, 0x00, 0x00, 0x00,
> +			0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +			0x00, 0x00, 0x00, 0x00 };
> +		u8 len = 42;
> +
> +		memcpy(&pattern[38], ip_addr, 4);
> +		rc = lan874x_chk_wol_pattern(pattern, mask, len,
> +					     data, &datalen);
> +		if (rc)
> +			phydev_dbg(phydev, "pattern not valid at %d\n", rc);
> +
> +		/* Need to match broadcast destination address. */
> +		val = MII_LAN874X_PHY_WOL_FILTER_BCSTEN;
> +		rc = lan874x_set_wol_pattern(phydev, val, data, datalen, mask,
> +					     len);
> +		if (rc < 0)
> +			return rc;
> +		priv->wol_arp = true;
> +	}
> +
> +	if (wol->wolopts & WAKE_MCAST) {
> +		u8 pattern[6] = { 0x33, 0x33, 0xFF, 0x00, 0x00, 0x00 };

A multicast Ethernet MAC address is defined by having bit 0 of the first 
byte (in network order) being set, what you are programming here is an 
IPv4 multicast MAC address pattern. Having recently submitted 
Wake-on-LAN for a Broadcom PHY (B50212E), I read WAKE_MAGIC as meaning 
"any multicast" and not specifically IP multicast.

> +		u16 mask[1] = { 0x0007 };
> +		u8 len = 3;
> +
> +		/* Try to match IPv6 Neighbor Solicitation. */
> +		if (ndev->ip6_ptr) {
> +			struct list_head *addr_list =
> +				&ndev->ip6_ptr->addr_list;
> +			struct inet6_ifaddr *ifa;
> +
> +			list_for_each_entry(ifa, addr_list, if_list) {
> +				if (ifa->scope == IFA_LINK) {
> +					memcpy(&pattern[3],
> +					       &ifa->addr.in6_u.u6_addr8[13],
> +					       3);
> +					mask[0] = 0x003F;
> +					len = 6;
> +					break;
> +				}
> +			}
> +		}

That would need to be enclosed within an #if IS_ENABLED(CONFIG_IPV6) 
presumablye, but see my comment above, I don't think you need to do that.

> +		rc = lan874x_chk_wol_pattern(pattern, mask, len,
> +					     data, &datalen);
> +		if (rc)
> +			phydev_dbg(phydev, "pattern not valid at %d\n", rc);
> +
> +		/* Need to match multicast destination address. */
> +		val = MII_LAN874X_PHY_WOL_FILTER_MCASTTEN;
> +		rc = lan874x_set_wol_pattern(phydev, val, data, datalen, mask,
> +					     len);
> +		if (rc < 0)
> +			return rc;
> +		priv->wol_arp = false;

priv->wol_arp is only used for reporting purposes in get_wol, but since 
the same pattern matching hardware is used for WAKE_MCAST and WAKE_ARP, 
you need to make that mutually exclusive with an if (wol->wolopts & 
WAKE_ARP) .. else if (wol->wolopts & WAKE_MCAST) otherwise whichever was 
specified last in the user command will "win".

> +	}
> +
> +	if (wol->wolopts & (WAKE_MAGIC | WAKE_UCAST)) {
> +		const u8 *mac = (const u8 *)ndev->dev_addr;
> +
> +		if (!is_valid_ether_addr(mac))
> +			return -EINVAL;

Same comment as Andrew, I would not care about that particular check.

> +
> +		rc = phy_write_mmd(phydev, MDIO_MMD_PCS,
> +				   MII_LAN874X_PHY_MMD_WOL_RX_ADDRC,
> +				   ((mac[1] << 8) | mac[0]));
> +		if (rc < 0)
> +			return rc;
> +
> +		rc = phy_write_mmd(phydev, MDIO_MMD_PCS,
> +				   MII_LAN874X_PHY_MMD_WOL_RX_ADDRB,
> +				   ((mac[3] << 8) | mac[2]));
> +		if (rc < 0)
> +			return rc;
> +
> +		rc = phy_write_mmd(phydev, MDIO_MMD_PCS,
> +				   MII_LAN874X_PHY_MMD_WOL_RX_ADDRA,
> +				   ((mac[5] << 8) | mac[4]));
> +		if (rc < 0)
> +			return rc;

Can you implement this as a for loop maybe?
-- 
Florian


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs.
  2023-05-31 22:43   ` Tristram.Ha
@ 2023-05-31 23:10     ` Andrew Lunn
  0 siblings, 0 replies; 11+ messages in thread
From: Andrew Lunn @ 2023-05-31 23:10 UTC (permalink / raw)
  To: Tristram.Ha; +Cc: davem, f.fainelli, netdev, UNGLinuxDriver

> > From an architecture point of view, i don't think a PHY driver should
> > be access these data structure directly. See if ipv6_get_lladdr() does
> > what you need?
> 
> ipv6_get_lladdr() is not exported so cannot be used when building the
> PHY driver as a module.

Well that is easy to change. You can propose changes to anything in
Linux, anywhere. And if it makes sense, it will get accepted.

       Andrew

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs.
  2023-05-31 22:52   ` Tristram.Ha
@ 2023-06-01  8:46     ` Simon Horman
  0 siblings, 0 replies; 11+ messages in thread
From: Simon Horman @ 2023-06-01  8:46 UTC (permalink / raw)
  To: Tristram.Ha; +Cc: davem, andrew, f.fainelli, netdev, UNGLinuxDriver

On Wed, May 31, 2023 at 10:52:22PM +0000, Tristram.Ha@microchip.com wrote:
> > > +     if (wol->wolopts & WAKE_ARP) {
> > > +             const u8 *ip_addr =
> > > +                     ((const u8 *)&((ndev->ip_ptr)->ifa_list)->ifa_address);
> > 
> > Hi Tristram,
> > 
> > Sparse seems unhappy about this:
> > 
> > .../smsc.c:449:27: warning: cast removes address space '__rcu' of expression
> > 
> 
> This will be fixed with in_dev_get() and rcu_dereference().
> 
> > > +             /* Try to match IPv6 Neighbor Solicitation. */
> > > +             if (ndev->ip6_ptr) {
> > > +                     struct list_head *addr_list =
> > > +                             &ndev->ip6_ptr->addr_list;
> > 
> > And this:
> > 
> > .../smsc.c:485:38: warning: incorrect type in initializer (different address spaces)
> > .../smsc.c:485:38:    expected struct list_head *addr_list
> > .../smsc.c:485:38:    got struct list_head [noderef] __rcu *
> > .../smsc.c:449:45: warning: dereference of noderef expression
> > 
> > Please make sure that patches don't intoduce new warnings with W=1 C=1 builds.
> 
> This will be fixed with in6_dev_get().
> 
> > > +#define MII_LAN874X_PHY_PME1_SET             (2<<13)
> > > +#define MII_LAN874X_PHY_PME2_SET             (2<<11)
> > 
> > nit: Maybe GENMASK is appropriate here.
> >      If not, please consider spaces around '<<'
> 
> Will update.

Thanks, much appreciated.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs.
  2023-05-31 23:07 ` Florian Fainelli
@ 2023-06-01 22:53   ` Tristram.Ha
  0 siblings, 0 replies; 11+ messages in thread
From: Tristram.Ha @ 2023-06-01 22:53 UTC (permalink / raw)
  To: f.fainelli; +Cc: netdev, UNGLinuxDriver, davem, andrew

> > +     if (wol->wolopts & WAKE_PHY)
> > +             return -EOPNOTSUPP;
> > +
> > +     /* lan874x has only one WoL filter pattern */
> > +     if ((wol->wolopts & (WAKE_ARP | WAKE_MCAST)) ==
> > +         (WAKE_ARP | WAKE_MCAST)) {
> > +             phydev_info(phydev,
> > +                         "lan874x WoL supports one of ARP|MCAST at a time\n");
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> > +     rc = phy_read_mmd(phydev, MDIO_MMD_PCS,
> MII_LAN874X_PHY_MMD_WOL_WUCSR);
> > +     if (rc < 0)
> > +             return rc;
> > +
> > +     val_wucsr = rc;
> 
> You need to take into account the case where an user wants to disable
> Wake-on-LAN entirely, e.g.:
> 
> ethtool -s <iface> wol d
> 
> [snip]
> 
> > +
> > +     if (wol->wolopts & WAKE_UCAST)
> > +             val_wucsr |= MII_LAN874X_PHY_WOL_PFDAEN;
> > +     else
> > +             val_wucsr &= ~MII_LAN874X_PHY_WOL_PFDAEN;
> > +
> > +     if (wol->wolopts & WAKE_BCAST)
> > +             val_wucsr |= MII_LAN874X_PHY_WOL_BCSTEN;
> > +     else
> > +             val_wucsr &= ~MII_LAN874X_PHY_WOL_BCSTEN;
> > +
> > +     if (wol->wolopts & WAKE_MAGIC)
> > +             val_wucsr |= MII_LAN874X_PHY_WOL_MPEN;
> > +     else
> > +             val_wucsr &= ~MII_LAN874X_PHY_WOL_MPEN;
> > +
> > +     /* Need to use pattern matching */
> > +     if (wol->wolopts & (WAKE_ARP | WAKE_MCAST))
> > +             val_wucsr |= MII_LAN874X_PHY_WOL_WUEN;
> > +     else
> > +             val_wucsr &= ~MII_LAN874X_PHY_WOL_WUEN;
> > +

The wolopts parameter contains the bits for WAKE_ARP and others.  When
WoL is disabled the bits are empty.  The driver disables the function
when the bit is not set, so it shall report properly about which WoL
functions are enabled.

> > +     if (wol->wolopts & WAKE_MCAST) {
> > +             u8 pattern[6] = { 0x33, 0x33, 0xFF, 0x00, 0x00, 0x00 };
> 
> A multicast Ethernet MAC address is defined by having bit 0 of the first
> byte (in network order) being set, what you are programming here is an
> IPv4 multicast MAC address pattern. Having recently submitted
> Wake-on-LAN for a Broadcom PHY (B50212E), I read WAKE_MAGIC as meaning
> "any multicast" and not specifically IP multicast.
>

WAKE_MCAST can be implemented in this simple way, but I feel it is not
useful as there are many types of multicast frames.  I settled on
implementing this way similar to WAKE_ARP so that the WoL functions can
be easily tested with ping and ping6.  Actually I do not know what users
really want as hardware WoL implementation is machine dependent.

> > +             /* Try to match IPv6 Neighbor Solicitation. */
> > +             if (ndev->ip6_ptr) {
> > +                     struct list_head *addr_list =
> > +                             &ndev->ip6_ptr->addr_list;
> > +                     struct inet6_ifaddr *ifa;
> > +
> > +                     list_for_each_entry(ifa, addr_list, if_list) {
> > +                             if (ifa->scope == IFA_LINK) {
> > +                                     memcpy(&pattern[3],
> > +                                            &ifa->addr.in6_u.u6_addr8[13],
> > +                                            3);
> > +                                     mask[0] = 0x003F;
> > +                                     len = 6;
> > +                                     break;
> > +                             }
> > +                     }
> > +             }
> 
> That would need to be enclosed within an #if IS_ENABLED(CONFIG_IPV6)
> presumablye, but see my comment above, I don't think you need to do that.
>

Will check on that.
 
> > +             rc = lan874x_set_wol_pattern(phydev, val, data, datalen, mask,
> > +                                          len);
> > +             if (rc < 0)
> > +                     return rc;
> > +             priv->wol_arp = false;
> 
> priv->wol_arp is only used for reporting purposes in get_wol, but since
> the same pattern matching hardware is used for WAKE_MCAST and WAKE_ARP,
> you need to make that mutually exclusive with an if (wol->wolopts &
> WAKE_ARP) .. else if (wol->wolopts & WAKE_MCAST) otherwise whichever was
> specified last in the user command will "win".
> 

The wolopts parameter can contain WAKE_ARP and WAKE_MCAST at the same
time, but this is rejected by the driver when the WoL command is first
processed.  So users can only use WoL command of WAKE_ARP or WAKE_MCAST.
There is no chance of programming the wrong function.

Users can do WoL WAKE_ARP first and then WoL WAKE_MCAST next, but that
cannot be help.  I do not think the WoL commands are accumulated, like
doing WAKE_ARP first and WAKE_MCAST next means both WAKE_ARP and
WAKE_MCAST are enabled.  The WoL functions are specified all at once
as users can mix up the WAKE_ARP, WAKE_UCAST, WAKE_MCAST, WAKE_BCAST,
and WAKE_MAGIC bits whatever they want.

> > +     if (wol->wolopts & (WAKE_MAGIC | WAKE_UCAST)) {
> > +             const u8 *mac = (const u8 *)ndev->dev_addr;
> > +
> > +             if (!is_valid_ether_addr(mac))
> > +                     return -EINVAL;
> 
> Same comment as Andrew, I would not care about that particular check.
>

Will remove.
 
> > +             rc = phy_write_mmd(phydev, MDIO_MMD_PCS,
> > +                                MII_LAN874X_PHY_MMD_WOL_RX_ADDRC,
> > +                                ((mac[1] << 8) | mac[0]));
> > +             if (rc < 0)
> > +                     return rc;
> > +
> > +             rc = phy_write_mmd(phydev, MDIO_MMD_PCS,
> > +                                MII_LAN874X_PHY_MMD_WOL_RX_ADDRB,
> > +                                ((mac[3] << 8) | mac[2]));
> > +             if (rc < 0)
> > +                     return rc;
> > +
> > +             rc = phy_write_mmd(phydev, MDIO_MMD_PCS,
> > +                                MII_LAN874X_PHY_MMD_WOL_RX_ADDRA,
> > +                                ((mac[5] << 8) | mac[4]));
> > +             if (rc < 0)
> > +                     return rc;
> 
> Can you implement this as a for loop maybe?

Will do.


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2023-06-01 22:53 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-27  1:39 [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs Tristram.Ha
2023-05-27 13:56 ` Simon Horman
2023-05-31 22:52   ` Tristram.Ha
2023-06-01  8:46     ` Simon Horman
2023-05-27 17:04 ` kernel test robot
2023-05-28  4:50 ` kernel test robot
2023-05-29 14:48 ` Andrew Lunn
2023-05-31 22:43   ` Tristram.Ha
2023-05-31 23:10     ` Andrew Lunn
2023-05-31 23:07 ` Florian Fainelli
2023-06-01 22:53   ` Tristram.Ha

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).