| Message ID | 20231025173300.1776832-5-florian.fainelli@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | WAKE_FILTER for Broadcom PHY (v2) | expand |
Hi Florian, On Thu. 26 Oct. 2023 at 02:32, Florian Fainelli <florian.fainelli@broadcom.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@broadcom.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)... > + } > + 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 >
On Thu. 26 Oct. 2023 at 10:10, Vincent MAILHOL <mailhol.vincent@wanadoo.fr> wrote: > Hi Florian, > > On Thu. 26 Oct. 2023 at 02:32, Florian Fainelli > <florian.fainelli@broadcom.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@broadcom.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 > >
Hi Vincent, On 10/25/23 19:13, Vincent MAILHOL wrote: [snip] >> >> 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? Because this is consistent with the existing code, though I will keep that suggestion in mind as a subsequent patch. I personally find it clearer expressed that way, but can update. > > 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. You are right, it was not consistent. >> >> Also, did you run parse to check your endianness conversions? I did, though nothing came out with C=1 or C=2, I might have to check that separately. >> >> 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(). Thanks for having taken a look.
On Thu, 26 Oct 2023 10:55:10 -0700 Florian Fainelli wrote: > >> Also, did you run parse to check your endianness conversions? > > I did, though nothing came out with C=1 or C=2, I might have to check > that separately. FWIW drivers/net/phy/bcm-phy-lib.c:1128:42: warning: cast to restricted __be16 drivers/net/phy/bcm-phy-lib.c:1128:40: warning: incorrect type in assignment (different base types) drivers/net/phy/bcm-phy-lib.c:1128:40: expected restricted __be16 [usertype] h_proto drivers/net/phy/bcm-phy-lib.c:1128:40: got unsigned short [usertype] drivers/net/phy/bcm-phy-lib.c:1188:17: warning: incorrect type in assignment (different base types) drivers/net/phy/bcm-phy-lib.c:1188:17: expected restricted __be16 [usertype] h_proto drivers/net/phy/bcm-phy-lib.c:1188:17: got unsigned short [usertype] drivers/net/phy/bcm-phy-lib.c:1190:25: warning: incorrect type in assignment (different base types) drivers/net/phy/bcm-phy-lib.c:1190:25: expected restricted __be16 [usertype] h_proto drivers/net/phy/bcm-phy-lib.c:1190:25: got int drivers/net/phy/bcm-phy-lib.c:1193:33: warning: incorrect type in argument 3 (different base types) drivers/net/phy/bcm-phy-lib.c:1193:33: expected unsigned short [usertype] val drivers/net/phy/bcm-phy-lib.c:1193:33: got restricted __be16 [usertype] h_proto
On 10/26/23 14:56, Jakub Kicinski wrote: > On Thu, 26 Oct 2023 10:55:10 -0700 Florian Fainelli wrote: >>>> Also, did you run parse to check your endianness conversions? >> >> I did, though nothing came out with C=1 or C=2, I might have to check >> that separately. > > FWIW > > drivers/net/phy/bcm-phy-lib.c:1128:42: warning: cast to restricted __be16 > drivers/net/phy/bcm-phy-lib.c:1128:40: warning: incorrect type in assignment (different base types) > drivers/net/phy/bcm-phy-lib.c:1128:40: expected restricted __be16 [usertype] h_proto > drivers/net/phy/bcm-phy-lib.c:1128:40: got unsigned short [usertype] > drivers/net/phy/bcm-phy-lib.c:1188:17: warning: incorrect type in assignment (different base types) > drivers/net/phy/bcm-phy-lib.c:1188:17: expected restricted __be16 [usertype] h_proto > drivers/net/phy/bcm-phy-lib.c:1188:17: got unsigned short [usertype] > drivers/net/phy/bcm-phy-lib.c:1190:25: warning: incorrect type in assignment (different base types) > drivers/net/phy/bcm-phy-lib.c:1190:25: expected restricted __be16 [usertype] h_proto > drivers/net/phy/bcm-phy-lib.c:1190:25: got int > drivers/net/phy/bcm-phy-lib.c:1193:33: warning: incorrect type in argument 3 (different base types) > drivers/net/phy/bcm-phy-lib.c:1193:33: expected unsigned short [usertype] val > drivers/net/phy/bcm-phy-lib.c:1193:33: got restricted __be16 [usertype] h_proto Yep, finally able to see that with an x86 build, not sure yet why sparse did not work with my aarch64 build, now fixed, thanks!
On Fri. 27 Oct. 2023 at 02:55, Florian Fainelli <florian.fainelli@broadcom.com> wrote: > Hi Vincent, > > On 10/25/23 19:13, Vincent MAILHOL wrote: > [snip] > >> > >> 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? > > Because this is consistent with the existing code, though I will keep > that suggestion in mind as a subsequent patch. I personally find it > clearer expressed that way, but can update. Fair enough. I agree that this is not something to be fixed in this series. For your future consideration, I would have done it as: __be16 da[ETH_ALEN / sizeof(__be16)]; /* ... */ da[i] = cpu_to_be16(~ret); da[] can eventually be casted back to u8 * once populated. (...)
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) 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; + } + 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); + + 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,
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 Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.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(-)