lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <27644300-ff4f-4603-9338-bad4aa0e5610@lunn.ch>
Date: Tue, 13 Feb 2024 15:03:01 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc: Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>, davem@...emloft.net,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Jonathan Corbet <corbet@....net>,
	Horatiu Vultur <horatiu.vultur@...rochip.com>,
	Richard Cochran <richardcochran@...il.com>,
	UNGLinuxDriver@...rochip.com, netdev@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	thomas.petazzoni@...tlin.com,
	Köry Maincent <kory.maincent@...tlin.com>
Subject: Re: [PATCH net-next 1/3] net: phy: Add support for inband extensions

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

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

> +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) ?

> +}
> +EXPORT_SYMBOL(phy_inband_ext_available);

If you don't mind, i would prefer EXPORT_SYMBOL_GPL().

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

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

       Andrew

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ