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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <BYAPR11MB3558A16BDAC76DDE3453B437EC27A@BYAPR11MB3558.namprd11.prod.outlook.com>
Date: Tue, 27 Jun 2023 02:15:27 +0000
From: <Tristram.Ha@...rochip.com>
To: <pabeni@...hat.com>
CC: <netdev@...r.kernel.org>, <UNGLinuxDriver@...rochip.com>,
	<andrew@...n.ch>, <f.fainelli@...il.com>, <davem@...emloft.net>
Subject: RE: [PATCH v3 net-next] net: phy: smsc: add WoL support to
 LAN8740/LAN8742 PHYs

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

Another patch will be submitted to address your issues.