Re: [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS

[Jakub Kicinski]

On Wed, 7 Dec 2022 14:08:51 +0100 Piergiorgio Beruto wrote:
> > > +  ``ETHTOOL_A_PLCA_ENABLED``              u8      PLCA Admin State
> > > +  ``ETHTOOL_A_PLCA_NODE_ID``              u8      PLCA unique local node ID
> > > +  ``ETHTOOL_A_PLCA_NODE_CNT``             u8      Number of PLCA nodes on the
> > > +                                                  netkork, including the  
> > 
> > netkork -> network  
> Got it, thanks.
> 
> > > +                                                  coordinator  
> > 
> > This is 30.16.1.1.3 aPLCANodeCount ? The phrasing of the help is quite
> > different than the standard. Pure count should be max node + 1 (IOW max
> > of 256, which won't fit into u8, hence the question)
> > Or is node 255 reserved?  
> This is indeed aPLCANodeCount. What standard are you referring to
> exactly? This is the excerpt from IEEE802.3cg-2019
> 
> "
> 30.16.1.1.3 aPLCANodeCount
> ATTRIBUTE
> APPROPRIATE SYNTAX:
> INTEGER
> BEHAVIOUR DEFINED AS:
> This value is assigned to define the number of nodes getting a transmit opportunity before a new
> BEACON is generated. Valid range is 0 to 255, inclusive. The default value is 8.;

FWIW this is what I was referring to. To a layperson this description
reads like a beacon interval. While the name and you documentation
sounds like the define max number of endpoints. 

> And this is what I can read in the OPEN Alliance documentation:
> 
> "
> 4.3.1 NCNT
> This field sets the maximum number of PLCA nodes supported on the multidrop
> network. On the node with PLCA ID = 0 (see 4.3.2), this value must be set at
> least to the number of nodes that may be plugged to the network in order for
> PLCA to operate properly. This bit maps to the aPLCANodeCount object in [1]
> Clause 30.
> "
> 
> So the valid range is actually 1..255. A value of 0 does not really mean
> anything. PHYs would just clamp this to 1. So maybe we should set a
> minimum limit in the kernel?

SG, loosing limits is easy. Introducing them once they are in 
a released kernel is almost impossible.

> Please, feel free to ask more questions here, it is important that we
> fully understand what this is. Fortunately, I am the guy who invented
> PLCA and wrote the specs, so I should be able to answer these questions :-D.

I am a little curious why beacon interval == node count here, but don't
want to take up too much of your time for explaining things I could
likely understand by reading the spec, rather than fuzzy-mapping onto
concepts I comprehend :(

This is completely unrelated to the series - do you know if any of
the new 10BASE stuff can easily run over a DC power rail within 
a server rack? :)

> > > +When set, the ``ETHTOOL_A_PLCA_STATUS`` attribute indicates whether the node is
> > > +detecting the presence of the BEACON on the network. This flag is
> > > +corresponding to ``IEEE 802.3cg-2019`` 30.16.1.1.2 aPLCAStatus.  
> > 
> > I noticed some count attributes in the spec, are these statistics?
> > Do any of your devices support them? It'd be good to add support in
> > a fixed format via net/ethtool/stats.c from the start, so that people
> > don't start inventing their own ways of reporting them.
> > 
> > (feel free to ask for more guidance, the stats support is a bit spread
> > out throughout the code)  
> Are you referring to this?
> 
> "
> 45.2.3.68f.1 CorruptedTxCnt (3.2294.15:0)
> Bits 3.2294.15:0 count up at each positive edge of the MII signal COL.
> When the maximum allowed value (65 535) is reached, the count stops until
> this register is cleared by a read operation.
> "
> 
> This is the only one statistic counter I can think of. Although, it is a
> 10BASE-T1S PHY related register, it is not specific to PLCA, even if its
> main purpose is to help the user distinguish between logical and
> physical collisions.
> 
> I would be inclined to add this as a separate feature unrelated to PLCA.
> Please, let me know what you think.

Fair point, I just opened the standard and searched counters.
Indeed outside of the scope here.

> > >   * @start_cable_test: Start a cable test
> > >   * @start_cable_test_tdr: Start a Time Domain Reflectometry cable test
> > >   *
> > > @@ -819,6 +823,13 @@ struct ethtool_phy_ops {
> > >  	int (*get_strings)(struct phy_device *dev, u8 *data);
> > >  	int (*get_stats)(struct phy_device *dev,
> > >  			 struct ethtool_stats *stats, u64 *data);
> > > +	int (*get_plca_cfg)(struct phy_device *dev,
> > > +			    struct phy_plca_cfg *plca_cfg);
> > > +	int (*set_plca_cfg)(struct phy_device *dev,
> > > +			    struct netlink_ext_ack *extack,
> > > +			    const struct phy_plca_cfg *plca_cfg);  
> > 
> > extack is usually the last argument  
> I actually copied from the cable_test_tdr callback below. Do you still
> want me to change the order? Should we do this for cable test as well?
> (as a different patch maybe?). Please, let me know.

Let's not move it in the existing callbacks but yes, cable_test_tdr
is more of an exception than a rule here, I reckon.

> > > +	int (*get_plca_status)(struct phy_device *dev,
> > > +			       struct phy_plca_status *plca_st);  
> > 
> > get status doesn't need exact? I guess..  
> This is my assumption. I would say it is similar to get_config, and I
> would say it is mandatory. I can hardly think of a system that does not
> implement get_status when supporting PLCA.
> Thoughts?

Sounds good.

> > The casts are unnecessary, but if you really really want them they 
> > can stay..  
> Removed it. Sorry, In the past I used to work in an environment where
> thay wanted -unnecessary- casts everywhere. I just need to get used to
> this...

:( Turns out the C language is evolving more than one would have
thought. Or at least what's considered good taste. In the kernel
we don't require casts. Here the helper has type in its name,
so it's very obvious.

> > > +	[ETHTOOL_A_PLCA_NODE_ID]	= { .type = NLA_U8 },  
> > 
> > Does this one also need check against 255 or is 255 allowed?  
> Good question. 255 has a special meaning --> unconfigured.
> So the question is, do we allow the user to set nodeID back to
> unconfigured? My opinion is yes, I don't really see a reson why not and
> I can see cases where I actually want to do this.
> Would you agree?

Just asking because of the tendency to limit inputs where possible
for backward compatibility. I'd think user space should be happy
with just disabling a node, but I defer to your expertise on whether
there's possible cases for giving the access to the full range to 
the user.

> > > +	if (tb[ETHTOOL_A_PLCA_BURST_TMR]) {
> > > +		plca_cfg.burst_tmr = nla_get_u8(tb[ETHTOOL_A_PLCA_BURST_TMR]);
> > > +		mod = true;
> > > +	}  
> > 
> > Could you add a helper for the modifications? A'la ethnl_update_u8, but
> > accounting for the oddness in types (ergo probably locally in this file
> > rather that in the global scope)?  
> I put the helper locally. We can always promote them later if more
> features follow this 'new' approach suggested by Andrew. Makes sense?

SG