Re: [PATCH net-next v3 1/2] ethtool: provide UAPI for PHY master/slave configuration.

[Michal Kubecek]

On Thu, Apr 30, 2020 at 07:00:58AM +0200, Oleksij Rempel wrote:
> > > diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> > > index 92f737f101178..eb680e3d6bda5 100644
> > > --- a/include/uapi/linux/ethtool.h
> > > +++ b/include/uapi/linux/ethtool.h
> > > @@ -1666,6 +1666,31 @@ static inline int ethtool_validate_duplex(__u8 duplex)
> > >  	return 0;
> > >  }
> > >  
> > > +/* Port mode */
> > > +#define PORT_MODE_CFG_UNKNOWN		0
> > > +#define PORT_MODE_CFG_MASTER_PREFERRED	1
> > > +#define PORT_MODE_CFG_SLAVE_PREFERRED	2
> > > +#define PORT_MODE_CFG_MASTER_FORCE	3
> > > +#define PORT_MODE_CFG_SLAVE_FORCE	4
> > > +#define PORT_MODE_STATE_UNKNOWN		0
> > > +#define PORT_MODE_STATE_MASTER		1
> > > +#define PORT_MODE_STATE_SLAVE		2
> > > +#define PORT_MODE_STATE_ERR		3
> > 
> > You have "MASTER_SLAVE" or "master_slave" everywhere but "PORT_MODE" in
> > these constants which is inconsistent.
> 
> What will be preferred name?

Not sure, that would be rather question for people more familiar with
the hardware. What I wanted to say is that whether we use "master_slave"
or "port_mode", we should use the same everywhere.

> > > +
> > > +static inline int ethtool_validate_master_slave_cfg(__u8 cfg)
> > > +{
> > > +	switch (cfg) {
> > > +	case PORT_MODE_CFG_MASTER_PREFERRED:
> > > +	case PORT_MODE_CFG_SLAVE_PREFERRED:
> > > +	case PORT_MODE_CFG_MASTER_FORCE:
> > > +	case PORT_MODE_CFG_SLAVE_FORCE:
> > > +	case PORT_MODE_CFG_UNKNOWN:
> > > +		return 1;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > 
> > Should we really allow CFG_UNKNOWN in client requests? As far as I can
> > see, this value is handled as no-op which should be rather expressed by
> > absence of the attribute. Allowing the client to request a value,
> > keeping current one and returning 0 (success) is IMHO wrong.
> 
> ok
> 
> > Also, should this function be in UAPI header?
> 
> It is placed together with other validate functions:
> ethtool_validate_duplex
> ethtool_validate_speed
> 
> Doing it in a different place, would be inconsistent.

Those two have been there since "ever". The important question is if we
want this function to be provided to userspace as part of UAPI (which
would also limit our options for future modifications.

> 
> > [...]
> > > @@ -119,7 +123,12 @@ 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_CFG,
> > > +		       lsettings->master_slave_cfg) ||
> > > +	    nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE,
> > > +		       lsettings->master_slave_state))
> > > +
> > >  		return -EMSGSIZE;
> > 
> > From the two handlers you introduced, it seems we only get CFG_UNKNOWN
> > or STATE_UNKNOWN if driver or device does not support the feature at all
> > so it would be IMHO more appropriate to omit the attribute in such case.
> 
> STATE_UNKNOWN is returned if link is not active.

How about distinguishing the two cases then? Omitting both if CFG is
CFG_UNKNOWN (i.e. driver does not support the feature) and sending
STATE=STATE_UNKNOWN to userspace only if we know it's a meaningful value
actually reported by the driver?

Michal