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

[Simon Horman <simon.horman@corigine.com>]

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