Hello Andrew,
On Tue, 13 Feb 2024 15:03:01 +0100
Andrew Lunn <andrew@xxxxxxx> 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
