| 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: <5051f1e2-4f8e-a021-df6c-d4066938422f@ti.com>
Date: Thu, 10 Sep 2020 12:34:09 -0500
From: Dan Murphy <dmurphy@...com>
To: Jakub Kicinski <kuba@...nel.org>
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
Jakub
Thanks for the review
On 9/5/20 1:34 PM, Jakub Kicinski wrote:
> 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.
I will add it back
>
>> + 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
OK
>
>> + 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?
Unicast and broadcast are the ways to send the magic packet.
Magic secure is programmed below into the SOP (secure on pass) registers
>
>> +
>> + 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?
Enhanced MAC - Enables enhanced RX features. This bit should be set when
using
wakeup abilities, CRC check or RX 1588 indication
>
> 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.
A helper is a bit overkill see below.
>
>> +}
>> +
>> 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?
Factor what out? Yes the DP83867 and DP83869 are very similar in
registers and bitmaps. They just differ in their feature sets.
The WoL code was copied and pasted to the 869 and I would like to keep
the two files as similar as I can as it will be easier to fix and find bugs.
Powered by blists - more mailing lists