Re: [PATCH v2 1/2] ethtool: provide UAPI for PHY master/slave configuration.

From: Andrew Lunn
Date: Mon Apr 20 2020 - 15:45:17 EST


> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -294,7 +294,7 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
> phydev->advertising, autoneg == AUTONEG_ENABLE);
>
> phydev->duplex = duplex;
> -
> + phydev->master_slave_set = cmd->base.master_slave;

Shouldn't you validate what has been passed from userspace it before
setting it?

> phydev->mdix_ctrl = cmd->base.eth_tp_mdix_ctrl;
>
> /* Restart the PHY */
> @@ -313,6 +313,7 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev,
>
> cmd->base.speed = phydev->speed;
> cmd->base.duplex = phydev->duplex;
> + cmd->base.master_slave = phydev->master_slave_get;
> if (phydev->interface == PHY_INTERFACE_MODE_MOCA)
> cmd->base.port = PORT_BNC;
> else

You had me confused for a while. You are packing multiple things into
master_slave_get. Some bits are configuration, some are current
state. I don't like this. I think this is so you can shoe-horn this
into the existing IOCTL API? There are limited spare bytes?

Maybe we should not support this via the IOCTL, only the netlink
ethtool?

> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -185,6 +185,7 @@ enum {
> ETHTOOL_A_LINKMODES_PEER, /* bitset */
> ETHTOOL_A_LINKMODES_SPEED, /* u32 */
> ETHTOOL_A_LINKMODES_DUPLEX, /* u8 */
> + ETHTOOL_A_LINKMODES_MASTER_SLAVE, /* u8 */

We would want two enums here, one for configuration, one for state.

> @@ -119,7 +121,9 @@ static int linkmodes_fill_reply(struct sk_buff *skb,
> }
>
> if (nla_put_u32(skb, ETHTOOL_A_LINKMODES_SPEED, lsettings->speed) ||
> - nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex))
> + nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex) ||
> + nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE,
> + lsettings->master_slave))

Here we return both configuration and current state.

> return -EMSGSIZE;
>
> return 0;
> @@ -248,6 +252,7 @@ linkmodes_set_policy[ETHTOOL_A_LINKMODES_MAX + 1] = {
> [ETHTOOL_A_LINKMODES_PEER] = { .type = NLA_REJECT },
> [ETHTOOL_A_LINKMODES_SPEED] = { .type = NLA_U32 },
> [ETHTOOL_A_LINKMODES_DUPLEX] = { .type = NLA_U8 },
> + [ETHTOOL_A_LINKMODES_MASTER_SLAVE] = { .type = NLA_U8 },
> };
>
> /* Set advertised link modes to all supported modes matching requested speed
> @@ -310,6 +315,8 @@ static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb,
> mod);
> ethnl_update_u8(&lsettings->duplex, tb[ETHTOOL_A_LINKMODES_DUPLEX],
> mod);
> + ethnl_update_u8(&lsettings->master_slave,
> + tb[ETHTOOL_A_LINKMODES_MASTER_SLAVE], mod);

Here we expect just configuration.

There actually is space for two fields in the ethtool_link_settings,
so i don't see why you actually did not use two.

Andrew