Re: [PATCH net-next 1/3] net: phy: Add support for inband extensions

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hello Andrew,

On Tue, 13 Feb 2024 15:03:01 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > +Inband Extensions
> > +=================
> > +
> > +The USGMII Standard allows the possibility to re-use the full-length 7-bytes
> > +frame preamble to convey meaningful data. This is already partly used by modes
> > +like QSGMII, which passes the port number in the preamble.
> > +
> > +In USGMII, we have a standardized approach to allow the MAC and PHY to pass
> > +such data in the preamble, which looks like this :
> > +
> > +|  0   |  1   |  2  |  3  |  4  |  5  |  6  |  7  |  Frame data
> > +| SoP  |      |      Extension              | CRC |
> > +|     /        \_______________             |     |
> > +|    /                         \            |     |
> > +|   | type | subport | ext type |           |     |
> > +
> > +The preamble in that case uses the Packet Control Header (PCH) format, where
> > +the byte 1 is used as a control field with :
> > +
> > +type - 2 bits :
> > +        - 00 : Packet with PCH
> > +        - 01 : Packet without PCH
> > +        - 10 : Idle Packet, without data
> > +        - 11 : Reserved
> > +
> > +subport - 4 bits : The subport identifier. For QUSGMII, this field ranges from
> > +                   0 to 3, and for OUSGMII, it ranges from 0 to 7.
> > +
> > +ext type - 2 bits : Indicated the type of data conveyed in the extension
> > +        - 00 : Ignore extension
> > +        - 01 : 8 bits reserved + 32 timestamp
> > +        - 10 : Reserved
> > +        - 11 : Reserved  
> 
> Somewhat crystal ball...
> 
> Those two reserved values could be used in the future to indicate
> other extensions. So we could have three in operation at once, but
> only one selected per frame.
> 
> > +A PHY driver can register available modes with::
> > +
> > +  int phy_inband_ext_set_available(struct phy_device *phydev, enum phy_inband_ext ext);
> > +  int phy_inband_ext_set_unavailable(struct phy_device *phydev, enum phy_inband_ext ext);  
> 
> enum phy_inband_ext is just an well defined, but arbitrary number? 0
> is this time stamp value mode, 1 could be used MACSEC, 2 could be a
> QoS indicator when doing rate adaptation? 3 could be ....
> 
> > +It's then up to the MAC driver to enable/disable the extension in the PHY as
> > +needed. This was designed to fit the timestamping configuration model, as it
> > +is the only mode supported so far.
> > +
> > +Enabling/Disabling an extension is done from the MAC driver through::
> > +
> > +  int phy_inband_ext_enable(struct phy_device *phydev, enum phy_inband_ext ext);  
> 
> So maybe this should return the 2 bit ext type value? The MAC can
> request QoS marking, and the PHY replies it expects the bits to be 3 ?
> 
> I'm just trying to ensure we have an API which is extensible in the
> future to make use of those two reserved values.

You are right, that's a much better idea !

> 
> > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > index 3b9531143be1..4b6cf94f51d5 100644
> > --- a/drivers/net/phy/phy.c
> > +++ b/drivers/net/phy/phy.c
> > @@ -1760,3 +1760,89 @@ int phy_ethtool_nway_reset(struct net_device *ndev)
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL(phy_ethtool_nway_reset);
> > +
> > +/**
> > + * PHY modes in the USXGMII family can have extensions, with data transmitted
> > + * in the frame preamble.
> > + * For now, only QUSGMII is supported, but other variants like USGMII and
> > + * OUSGMII can be added in the future.
> > + */
> > +static inline bool phy_interface_has_inband_ext(phy_interface_t interface)  
> 
> No inline functions in .c file please. Let the compiler decide.

My bad this one slept through the cracks...

> 
> > +bool phy_inband_ext_available(struct phy_device *phydev, enum phy_inband_ext ext)
> > +{
> > +	return !!(phydev->inband_ext.available & ext);  
> 
> should this be BIT(ext) ?

Correct indeed

> 
> > +}
> > +EXPORT_SYMBOL(phy_inband_ext_available);  
> 
> If you don't mind, i would prefer EXPORT_SYMBOL_GPL().

I don't mind, I'll fix that

> 
> > +static int phy_set_inband_ext(struct phy_device *phydev,
> > +			      enum phy_inband_ext ext,
> > +			      bool enable)
> > +{
> > +	int ret;
> > +
> > +	if (!phy_interface_has_inband_ext(phydev->interface))
> > +		return -EOPNOTSUPP;
> > +
> > +	if (!phydev->drv->set_inband_ext)
> > +		return -EOPNOTSUPP;  
> 
> That is a driver bug. It should not set phydev->inband_ext.available
> and then not have drv->set_inband_ext. So we should probably test this
> earlier. Maybe define that phydev->inband_ext.available has to be set
> during probe, and the core can validate this after probe and reject
> the device if it is inconsistent?

Good point, I'll add that !

> 
> > +
> > +	mutex_lock(&phydev->lock);
> > +	ret = phydev->drv->set_inband_ext(phydev, ext, enable);
> > +	mutex_unlock(&phydev->lock);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (enable)
> > +		phydev->inband_ext.enabled |= BIT(ext);
> > +	else
> > +		phydev->inband_ext.enabled &= ~BIT(ext);  
> 
> Should these be also protected by the mutex?

I think you are right, it would be better making sure we serialize
accesses to these indeed.

Thanks for the review,

Maxime