Messages in this thread | | | Date | Mon, 20 Apr 2020 21:45:04 +0200 | From | Andrew Lunn <> | Subject | Re: [PATCH v2 1/2] ethtool: provide UAPI for PHY master/slave configuration. |
| |
> --- 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
| |