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

[Andrew Lunn <andrew@lunn.ch>]

> +	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