Re: [PATCH v2 1/2] ethtool: provide UAPI for PHY master/slave configuration.
From: Andrew Lunn <andrew@lunn.ch>
Date: 2020-04-20 19:45:16
Also in:
lkml
quoted hunk ↗ jump to hunk
--- 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?
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
--- 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.
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
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