On 10/25/23 10:33, Florian Fainelli wrote:
> Determine whether the PHY can support waking up from the user programmed
> network filter, and if it can utilize it.
>
> Signed-off-by: Florian Fainelli <[email protected]>>
> ---
[snip]
> diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
> index cfbeedc5ee81..569aeab68f97 100644
> --- a/drivers/net/phy/bcm-phy-lib.c
> +++ b/drivers/net/phy/bcm-phy-lib.c
This hunk belongs in the previous patch, I will send a v2 after
collecting feedback.
--
Florian
Hi Florian,
On Thu. 26 Oct. 2023 at 02:32, Florian Fainelli
<[email protected]> 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 <[email protected]>
> ---
> 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
<[email protected]> wrote:
> Hi Florian,
>
> On Thu. 26 Oct. 2023 at 02:32, Florian Fainelli
> <[email protected]> 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 <[email protected]>
> > ---
> > 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.
--
Florian
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
--
pw-bot: cr
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!
--
Florian
On Fri. 27 Oct. 2023 at 02:55, Florian Fainelli
<[email protected]> 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.
(...)