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

[<Tristram.Ha@microchip.com>]

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