netdev - Re: [PATCH v3 net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs

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]

Date: Fri, 23 Jun 2023 12:47:04 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Tristram.Ha@...rochip.com, Andrew Lunn <andrew@...n.ch>, Florian
 Fainelli <f.fainelli@...il.com>, "David S. Miller" <davem@...emloft.net>
Cc: netdev@...r.kernel.org, UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH v3 net-next] net: phy: smsc: add WoL support to
 LAN8740/LAN8742 PHYs

On Wed, 2023-06-21 at 16:00 -0700, Tristram.Ha@...rochip.com wrote:
> @@ -258,6 +265,249 @@ int lan87xx_read_status(struct phy_device *phydev)
[...]
> +static int lan874x_chk_wol_pattern(const u8 pattern[], const u16 *mask,
> +				   u8 len, u8 *data, u8 *datalen)

I think it would be nice adding some comments here describing the
implemented logic, for future memory. 


> +{
> +	size_t i, j, k;
> +	int ret = 0;
> +	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)
> +					ret = i + 1;
> +				break;
> +			}
> +			if (bits & 1)
> +				data[k++] = pattern[i];
> +			bits >>= 1;
> +		}
> +		mask++;
> +	}
> +	*datalen = k;
> +	return ret;
> +}

[...]

> +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) {
> +		u8 pattern[14] = {
> +			0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
> +			0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +			0x08, 0x06 };
> +		u16 mask[1] = { 0x303F };
> +		u8 len = 14;

'len' is never changed, you could use instead some macro with a
meaningful name.

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

Same here, but now 'len' is 0, which makes the following
lan874x_chk_wol_pattern() a no-op. Was '3' in the initial revision, is
the above an unintentional change?!? Otherwise it would be clearer
avoid the lan874x_set_wol_pattern() call. 

Thanks!

Paolo