lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <40c2010e-337f-6bc1-5080-ab710fc4f991@gmail.com>
Date: Wed, 31 May 2023 16:07:10 -0700
From: Florian Fainelli <f.fainelli@...il.com>
To: Tristram.Ha@...rochip.com, "David S. Miller" <davem@...emloft.net>,
 Andrew Lunn <andrew@...n.ch>
Cc: netdev@...r.kernel.org, UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH net-next] net: phy: smsc: add WoL support to
 LAN8740/LAN8742 PHYs.

On 5/26/23 18:39, Tristram.Ha@...rochip.com wrote:
> From: Tristram Ha <Tristram.Ha@...rochip.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@...rochip.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


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ