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

[Andrew Lunn]

> --- 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