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: <CAMZ6Rq+iBazJ+fM5yd5Tfa8==DEGV93iD-XojU=f1m3ScSGEww@mail.gmail.com> Date: Thu, 26 Oct 2023 11:13:24 +0900 From: Vincent MAILHOL <mailhol.vincent@...adoo.fr> To: Florian Fainelli <florian.fainelli@...adcom.com> Cc: netdev@...r.kernel.org, Doug Berger <opendmb@...il.com>, Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>, Russell King <linux@...linux.org.uk>, Vladimir Oltean <vladimir.oltean@....com>, Tariq Toukan <tariqt@...dia.com>, Gal Pressman <gal@...dia.com>, Willem de Bruijn <willemb@...gle.com>, Daniil Tatianin <d-tatianin@...dex-team.ru>, Simon Horman <horms@...nel.org>, Justin Chen <justin.chen@...adcom.com>, Ratheesh Kannoth <rkannoth@...vell.com>, Joe Damato <jdamato@...tly.com>, Jiri Pirko <jiri@...nulli.us>, open list <linux-kernel@...r.kernel.org> Subject: Re: [PATCH net-next 4/5] net: phy: broadcom: Add support for WAKE_FILTER On Thu. 26 Oct. 2023 at 10:10, Vincent MAILHOL <mailhol.vincent@...adoo.fr> wrote: > Hi Florian, > > On Thu. 26 Oct. 2023 at 02:32, Florian Fainelli > <florian.fainelli@...adcom.com> wrote: > > Since the PHY is capable of matching any arbitrary Ethernet MAC > > destination as a programmable wake-up pattern, add support for doing > > that using the WAKE_FILTER and ethtool::rxnfc API. For instance, in > > order to wake-up from the Ethernet MAC address corresponding to the IPv4 > > multicast IP address of 224.0.0.251 (e.g.: multicast DNS), one could do: > > > > ethtool -N eth0 flow-type ether dst 01:00:5e:00:00:fb loc 0 action -2 > > ethtool -n eth0 > > Total 1 rules > > > > Filter: 0 > > Flow Type: Raw Ethernet > > Src MAC addr: 00:00:00:00:00:00 mask: FF:FF:FF:FF:FF:FF > > Dest MAC addr: 01:00:5E:00:00:FB mask: 00:00:00:00:00:00 > > Ethertype: 0x0 mask: 0xFFFF > > Action: Wake-on-LAN > > ethtool -s eth0 wol f > > Nit: indent the commands and their output with two spaces. > > > Signed-off-by: Florian Fainelli <florian.fainelli@...adcom.com> > > --- > > drivers/net/phy/bcm-phy-lib.c | 195 +++++++++++++++++++++++++++++++++- > > drivers/net/phy/bcm-phy-lib.h | 5 + > > drivers/net/phy/broadcom.c | 2 + > > 3 files changed, 201 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c > > index 876f28fd8256..cfbeedc5ee81 100644 > > --- a/drivers/net/phy/bcm-phy-lib.c > > +++ b/drivers/net/phy/bcm-phy-lib.c > > @@ -827,7 +827,8 @@ EXPORT_SYMBOL_GPL(bcm_phy_cable_test_get_status_rdb); > > WAKE_MCAST | \ > > WAKE_BCAST | \ > > WAKE_MAGIC | \ > > - WAKE_MAGICSECURE) > > + WAKE_MAGICSECURE | \ > > + WAKE_FILTER) > > Nit: you may want to have the closing bracket on a newline to have a > cleaner diff for new future additions. > > > int bcm_phy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) > > { > > @@ -881,6 +882,12 @@ int bcm_phy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) > > ctl &= ~BCM54XX_WOL_DIR_PKT_EN; > > ctl &= ~(BCM54XX_WOL_SECKEY_OPT_MASK << BCM54XX_WOL_SECKEY_OPT_SHIFT); > > > > + /* For WAKE_FILTER, we have already programmed the desired MAC DA > > + * and associated mask by the time we get there. > > + */ > > + if (wol->wolopts & WAKE_FILTER) > > + goto program_ctl; > > + > > /* When using WAKE_MAGIC, we program the magic pattern filter to match > > * the device's MAC address and we accept any MAC DA in the Ethernet > > * frame. > > @@ -935,6 +942,7 @@ int bcm_phy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) > > return ret; > > } > > > > +program_ctl: > > if (wol->wolopts & WAKE_MAGICSECURE) { > > ctl |= BCM54XX_WOL_SECKEY_OPT_6B << > > BCM54XX_WOL_SECKEY_OPT_SHIFT; > > @@ -999,6 +1007,16 @@ void bcm_phy_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) > > if (!(ctl & BCM54XX_WOL_EN)) > > return; > > > > + ret = bcm_phy_read_exp(phydev, BCM54XX_WOL_SEC_KEY_8B); > > + if (ret < 0) > > + return; > > + > > + /* Mutualy exclusive with other modes */ > > + if (ret) { > > + wol->wolopts |= WAKE_FILTER; > > + return; > > + } > > + > > for (i = 0; i < sizeof(da) / 2; i++) { > > ret = bcm_phy_read_exp(phydev, > > BCM54XX_WOL_MPD_DATA2(2 - i)); > > @@ -1066,6 +1084,181 @@ int bcm_phy_led_brightness_set(struct phy_device *phydev, > > } > > EXPORT_SYMBOL_GPL(bcm_phy_led_brightness_set); > > > > +static int bcm_phy_get_rule(struct phy_device *phydev, > > + struct ethtool_rxnfc *nfc, > > + int loc) > > +{ > > + u8 da[ETH_ALEN]; > > + unsigned int i; > > + int ret; > > + > > + if (loc != 0) > > + return -EINVAL; > > + > > + memset(nfc, 0, sizeof(*nfc)); > > + nfc->flow_type = ETHER_FLOW; > > + nfc->fs.flow_type = ETHER_FLOW; > > + > > + for (i = 0; i < sizeof(da) / 2; i++) { > > + ret = bcm_phy_read_exp(phydev, > > + BCM54XX_WOL_MPD_DATA2(2 - i)); > > + if (ret < 0) > > + return ret; > > + > > + da[i * 2] = ret >> 8; > > + da[i * 2 + 1] = ret & 0xff; > > This looks like an endianness conversion (I can not tell if this is > big to little or the opposite)... Oopsy! On second look, this is an open coded cpu to big endian conversion. So the question I should have asked is: why not use the put_unaligned_be16() helper here? Below comments still remain. > > + } > > + ether_addr_copy(nfc->fs.h_u.ether_spec.h_dest, da); > > + > > + for (i = 0; i < sizeof(da) / 2; i++) { > > + ret = bcm_phy_read_exp(phydev, > > + BCM54XX_WOL_MASK(2 - i)); > > + if (ret < 0) > > + return ret; > > + > > + da[i * 2] = ~(ret >> 8); > > + da[i * 2 + 1] = ~(ret & 0xff); > > + } > > + ether_addr_copy(nfc->fs.m_u.ether_spec.h_dest, da); > > + > > + ret = bcm_phy_read_exp(phydev, BCM54XX_WOL_INNER_PROTO); > > + if (ret < 0) > > + return ret; > > + > > + nfc->fs.h_u.ether_spec.h_proto = be16_to_cpu(ret); > > ... but here it is big endian to cpu endian? It does not look coherent. > > Also, did you run parse to check your endianness conversions? > > https://www.kernel.org/doc/html/latest/dev-tools/sparse.html > > For example, I would have expected htons() (a.k.a. cpu_to_be16()) > instead of be16_to_cpu(). > > > + nfc->fs.ring_cookie = RX_CLS_FLOW_WAKE; > > + nfc->fs.location = 0; > > + > > + return 0; > > +} > > + > > +static int bcm_phy_set_rule(struct phy_device *phydev, > > + struct ethtool_rxnfc *nfc) > > +{ > > + int ret = -EOPNOTSUPP; > > + unsigned int i; > > + __be16 h_proto; > > + const u8 *da; > > + > > + /* We support only matching on the MAC DA with a custom mask and > > + * optionally with a specific Ethernet type, reject anything else. > > + */ > > + if (nfc->fs.ring_cookie != RX_CLS_FLOW_WAKE || > > + (nfc->fs.location != 0 && > > + nfc->fs.location != RX_CLS_LOC_ANY && > > + nfc->fs.location != RX_CLS_LOC_FIRST) || > > + nfc->fs.flow_type != ETHER_FLOW || > > + !is_zero_ether_addr(nfc->fs.h_u.ether_spec.h_source) || > > + !is_zero_ether_addr(nfc->fs.m_u.ether_spec.h_source)) > > + return ret; > > + > > + ret = bcm_phy_read_exp(phydev, BCM54XX_WOL_SEC_KEY_8B); > > + if (ret < 0) > > + return ret; > > + > > + if (ret) > > + return -EBUSY; > > + > > + if (nfc->fs.location == RX_CLS_LOC_ANY || > > + nfc->fs.location == RX_CLS_LOC_FIRST) > > + nfc->fs.location = 0; > > + > > + da = nfc->fs.h_u.ether_spec.h_dest; > > + for (i = 0; i < ETH_ALEN / 2; i++) { > > + ret = bcm_phy_write_exp(phydev, > > + BCM54XX_WOL_MPD_DATA2(2 - i), > > + da[i * 2] << 8 | da[i * 2 + 1]); > > + if (ret < 0) > > + return ret; > > + } > > + > > + da = nfc->fs.m_u.ether_spec.h_dest; > > + for (i = 0; i < ETH_ALEN / 2; i++) { > > + ret = bcm_phy_write_exp(phydev, > > + BCM54XX_WOL_MASK(2 - i), > > + da[i * 2] << 8 | da[i * 2 + 1]); > > + if (ret < 0) > > + return ret; > > + } > > + > > + /* Restore default inner protocol field unless overridden by the flow > > + * specification. > > + */ > > + h_proto = be16_to_cpu(nfc->fs.h_u.ether_spec.h_proto); > > + if (!h_proto) > > + h_proto = ETH_P_8021Q; > > + > > + ret = bcm_phy_write_exp(phydev, BCM54XX_WOL_INNER_PROTO, > > + h_proto); > > + if (ret) > > + return ret; > > + > > + /* Use BCM54XX_WOL_SEC_KEY_8B as a scratch register to record > > + * that we installed a filter rule. > > + */ > > + return bcm_phy_write_exp(phydev, BCM54XX_WOL_SEC_KEY_8B, 1); > > +} > > + > > +int bcm_phy_get_rxnfc(struct phy_device *phydev, > > + struct ethtool_rxnfc *cmd, u32 *rule_locs) > > +{ > > + int err = 0, rule_cnt = 0; > > + > > + err = bcm_phy_read_exp(phydev, BCM54XX_WOL_SEC_KEY_8B); > > + if (err < 0) > > + return err; > > + > > + rule_cnt = err; > > + err = 0; > > + > > + switch (cmd->cmd) { > > + case ETHTOOL_GRXCLSRLCNT: > > + cmd->rule_cnt = rule_cnt; > > + cmd->data = 1 | RX_CLS_LOC_SPECIAL; > > + break; > > + case ETHTOOL_GRXCLSRULE: > > + err = bcm_phy_get_rule(phydev, cmd, cmd->fs.location); > > + break; > > + case ETHTOOL_GRXCLSRLALL: > > + if (rule_cnt) > > + rule_locs[0] = 0; > > + cmd->rule_cnt = rule_cnt; > > + cmd->data = 1; > > + break; > > + default: > > + err = -EOPNOTSUPP; > > + break; > > + } > > + > > + return err; > > +} > > +EXPORT_SYMBOL_GPL(bcm_phy_get_rxnfc); > > + > > +int bcm_phy_set_rxnfc(struct phy_device *phydev, > > + struct ethtool_rxnfc *cmd) > > +{ > > + int err = 0; > > + > > + switch (cmd->cmd) { > > + case ETHTOOL_SRXCLSRLINS: > > + err = bcm_phy_set_rule(phydev, cmd); > > + break; > > + case ETHTOOL_SRXCLSRLDEL: > > + if (cmd->fs.location != 0) > > + return err; > > + > > + err = bcm_phy_write_exp(phydev, BCM54XX_WOL_SEC_KEY_8B, 0); > > + break; > > + default: > > + err = -EOPNOTSUPP; > > + break; > > + } > > + > > + return err; > > +} > > +EXPORT_SYMBOL_GPL(bcm_phy_set_rxnfc); > > + > > MODULE_DESCRIPTION("Broadcom PHY Library"); > > MODULE_LICENSE("GPL v2"); > > MODULE_AUTHOR("Broadcom Corporation"); > > diff --git a/drivers/net/phy/bcm-phy-lib.h b/drivers/net/phy/bcm-phy-lib.h > > index b52189e45a84..7081edcec06b 100644 > > --- a/drivers/net/phy/bcm-phy-lib.h > > +++ b/drivers/net/phy/bcm-phy-lib.h > > @@ -121,4 +121,9 @@ irqreturn_t bcm_phy_wol_isr(int irq, void *dev_id); > > int bcm_phy_led_brightness_set(struct phy_device *phydev, > > u8 index, enum led_brightness value); > > > > +int bcm_phy_get_rxnfc(struct phy_device *phydev, > > + struct ethtool_rxnfc *nfc, u32 *rule_locs); > > +int bcm_phy_set_rxnfc(struct phy_device *phydev, > > + struct ethtool_rxnfc *nfc); > > + > > #endif /* _LINUX_BCM_PHY_LIB_H */ > > diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c > > index 3a627105675a..6c2212bd2779 100644 > > --- a/drivers/net/phy/broadcom.c > > +++ b/drivers/net/phy/broadcom.c > > @@ -1107,6 +1107,8 @@ static struct phy_driver broadcom_drivers[] = { > > .get_wol = bcm54xx_phy_get_wol, > > .set_wol = bcm54xx_phy_set_wol, > > .led_brightness_set = bcm_phy_led_brightness_set, > > + .get_rxnfc = bcm_phy_get_rxnfc, > > + .set_rxnfc = bcm_phy_set_rxnfc, > > }, { > > .phy_id = PHY_ID_BCM5461, > > .phy_id_mask = 0xfffffff0, > > -- > > 2.34.1 > >
Powered by blists - more mailing lists