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

[Oleksij Rempel <o.rempel@pengutronix.de>]

Hi Michal,

On Thu, Apr 30, 2020 at 10:20:20AM +0200, Michal Kubecek wrote:
> 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.

Ok, I rename it all to MASTER_SLAVE prefix. It is the only common name
across the all documentations.

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

good argument. Moved it to other location.

> > 
> > > [...]
> > > > @@ -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?

Hm... after thinking about it, we should keep state and config
separately. The TJA1102 PHY do not provide actual state. Even no error
related to Master/Master conflict. I reworked the code to have
unsupported and unknown values.  In case we know, that state or config
is not supported, it will not be exported to the user space.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |