| 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
| ||
|
Message-ID: <20200905113428.5bd7dc95@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date: Sat, 5 Sep 2020 11:34:28 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Dan Murphy <dmurphy@...com>
Cc: <davem@...emloft.net>, <andrew@...n.ch>, <f.fainelli@...il.com>,
<hkallweit1@...il.com>, <netdev@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v3 2/3] net: phy: dp83869: support Wake on LAN
On Thu, 3 Sep 2020 06:42:58 -0500 Dan Murphy wrote:
> This adds WoL support on TI DP83869 for magic, magic secure, unicast and
> broadcast.
>
> Signed-off-by: Dan Murphy <dmurphy@...com>
> ---
> drivers/net/phy/dp83869.c | 128 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 128 insertions(+)
>
> diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
> index 48a68474f89c..5045df9515a5 100644
> --- a/drivers/net/phy/dp83869.c
> +++ b/drivers/net/phy/dp83869.c
> @@ -4,6 +4,7 @@
> */
>
> #include <linux/ethtool.h>
> +#include <linux/etherdevice.h>
> #include <linux/kernel.h>
> #include <linux/mii.h>
> #include <linux/module.h>
> @@ -27,6 +28,13 @@
> #define DP83869_RGMIICTL 0x0032
> #define DP83869_STRAP_STS1 0x006e
> #define DP83869_RGMIIDCTL 0x0086
> +#define DP83869_RXFCFG 0x0134
> +#define DP83869_RXFPMD1 0x0136
> +#define DP83869_RXFPMD2 0x0137
> +#define DP83869_RXFPMD3 0x0138
> +#define DP83869_RXFSOP1 0x0139
> +#define DP83869_RXFSOP2 0x013A
> +#define DP83869_RXFSOP3 0x013B
> #define DP83869_IO_MUX_CFG 0x0170
> #define DP83869_OP_MODE 0x01df
> #define DP83869_FX_CTRL 0x0c00
> @@ -105,6 +113,14 @@
> #define DP83869_OP_MODE_MII BIT(5)
> #define DP83869_SGMII_RGMII_BRIDGE BIT(6)
>
> +/* RXFCFG bits*/
> +#define DP83869_WOL_MAGIC_EN BIT(0)
> +#define DP83869_WOL_PATTERN_EN BIT(1)
> +#define DP83869_WOL_BCAST_EN BIT(2)
> +#define DP83869_WOL_UCAST_EN BIT(4)
> +#define DP83869_WOL_SEC_EN BIT(5)
> +#define DP83869_WOL_ENH_MAC BIT(7)
> +
> enum {
> DP83869_PORT_MIRRORING_KEEP,
> DP83869_PORT_MIRRORING_EN,
> @@ -156,6 +172,115 @@ static int dp83869_config_intr(struct phy_device *phydev)
> return phy_write(phydev, MII_DP83869_MICR, micr_status);
> }
>
> +static int dp83869_set_wol(struct phy_device *phydev,
> + struct ethtool_wolinfo *wol)
> +{
> + struct net_device *ndev = phydev->attached_dev;
> + u16 val_rxcfg, val_micr;
> + u8 *mac;
> +
> + val_rxcfg = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_RXFCFG);
> + val_micr = phy_read(phydev, MII_DP83869_MICR);
In the previous patch you checked if phy_read() failed, here you don't.
> + if (wol->wolopts & (WAKE_MAGIC | WAKE_MAGICSECURE | WAKE_UCAST |
> + WAKE_BCAST)) {
> + val_rxcfg |= DP83869_WOL_ENH_MAC;
> + val_micr |= MII_DP83869_MICR_WOL_INT_EN;
> +
> + if (wol->wolopts & WAKE_MAGIC) {
> + mac = (u8 *)ndev->dev_addr;
> +
> + if (!is_valid_ether_addr(mac))
> + return -EINVAL;
> +
> + phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RXFPMD1,
> + (mac[1] << 8 | mac[0]));
parenthesis unnecessary
> + phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RXFPMD2,
> + (mac[3] << 8 | mac[2]));
> + phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RXFPMD3,
> + (mac[5] << 8 | mac[4]));
Why only program mac addr for wake_magic, does magic_secure or unicast
not require it?
> +
> + val_rxcfg |= DP83869_WOL_MAGIC_EN;
> + } else {
> + val_rxcfg &= ~DP83869_WOL_MAGIC_EN;
> + }
> +
> + if (wol->wolopts & WAKE_MAGICSECURE) {
> + phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RXFSOP1,
> + (wol->sopass[1] << 8) | wol->sopass[0]);
> + phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RXFSOP2,
> + (wol->sopass[3] << 8) | wol->sopass[2]);
> + phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RXFSOP3,
> + (wol->sopass[5] << 8) | wol->sopass[4]);
> +
> + val_rxcfg |= DP83869_WOL_SEC_EN;
> + } else {
> + val_rxcfg &= ~DP83869_WOL_SEC_EN;
> + }
> +
> + if (wol->wolopts & WAKE_UCAST)
> + val_rxcfg |= DP83869_WOL_UCAST_EN;
> + else
> + val_rxcfg &= ~DP83869_WOL_UCAST_EN;
> +
> + if (wol->wolopts & WAKE_BCAST)
> + val_rxcfg |= DP83869_WOL_BCAST_EN;
> + else
> + val_rxcfg &= ~DP83869_WOL_BCAST_EN;
> + } else {
> + val_rxcfg &= ~DP83869_WOL_ENH_MAC;
> + val_micr &= ~MII_DP83869_MICR_WOL_INT_EN;
> + }
> +
> + phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RXFCFG, val_rxcfg);
> + phy_write(phydev, MII_DP83869_MICR, val_micr);
> +
> + return 0;
> +}
> +
> +static void dp83869_get_wol(struct phy_device *phydev,
> + struct ethtool_wolinfo *wol)
> +{
> + u16 value, sopass_val;
> +
> + wol->supported = (WAKE_UCAST | WAKE_BCAST | WAKE_MAGIC |
> + WAKE_MAGICSECURE);
> + wol->wolopts = 0;
> +
> + value = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_RXFCFG);
> +
> + if (value & DP83869_WOL_UCAST_EN)
> + wol->wolopts |= WAKE_UCAST;
> +
> + if (value & DP83869_WOL_BCAST_EN)
> + wol->wolopts |= WAKE_BCAST;
> +
> + if (value & DP83869_WOL_MAGIC_EN)
> + wol->wolopts |= WAKE_MAGIC;
> +
> + if (value & DP83869_WOL_SEC_EN) {
> + sopass_val = phy_read_mmd(phydev, DP83869_DEVADDR,
> + DP83869_RXFSOP1);
> + wol->sopass[0] = (sopass_val & 0xff);
> + wol->sopass[1] = (sopass_val >> 8);
> +
> + sopass_val = phy_read_mmd(phydev, DP83869_DEVADDR,
> + DP83869_RXFSOP2);
> + wol->sopass[2] = (sopass_val & 0xff);
> + wol->sopass[3] = (sopass_val >> 8);
> +
> + sopass_val = phy_read_mmd(phydev, DP83869_DEVADDR,
> + DP83869_RXFSOP3);
> + wol->sopass[4] = (sopass_val & 0xff);
> + wol->sopass[5] = (sopass_val >> 8);
> +
> + wol->wolopts |= WAKE_MAGICSECURE;
> + }
> +
> + if (!(value & DP83869_WOL_ENH_MAC))
> + wol->wolopts = 0;
What does ENH stand for?
Perhaps it would be cleaner to make a helper like this:
u32 helper(u16 rxfsop1)
{
u32 wolopts;
if (!(value & DP83869_WOL_ENH_MAC))
return 0;
if (value & DP83869_WOL_UCAST_EN)
wolopts |= WAKE_UCAST;
if (value & DP83869_WOL_BCAST_EN)
wolopts |= WAKE_BCAST;
if (value & DP83869_WOL_MAGIC_EN)
wolopts |= WAKE_MAGIC;
if (value & DP83869_WOL_SEC_EN)
wolopts |= WAKE_MAGICSECURE;
return wolopts;
}
wol->wolopts = helper(value);
setting the bits and then clearing the value looks strange.
> +}
> +
> static int dp83869_config_port_mirroring(struct phy_device *phydev)
> {
> struct dp83869_private *dp83869 = phydev->priv;
Overall this code looks quite similar to dp83867, is there no way to
factor this out?
Powered by blists - more mailing lists