netdev - Re: [RFC PATCH net-next 1/2] net: phy: Add phy latency adjustment support in phy framework.

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: <Yl6r2mOlszx1I+fv@lunn.ch>
Date:   Tue, 19 Apr 2022 14:32:26 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Horatiu Vultur <horatiu.vultur@...rochip.com>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        hkallweit1@...il.com, linux@...linux.org.uk, davem@...emloft.net,
        kuba@...nel.org, pabeni@...hat.com, UNGLinuxDriver@...rochip.com,
        richardcochran@...il.com
Subject: Re: [RFC PATCH net-next 1/2] net: phy: Add phy latency adjustment
 support in phy framework.

On Tue, Apr 19, 2022 at 10:37:03AM +0200, Horatiu Vultur wrote:
> Add adjustment support for latency for the phy using sysfs.
> This is used to adjust the latency of the phy based on link mode
> and direction.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@...rochip.com>
> ---
>  .../ABI/testing/sysfs-class-net-phydev        | 10 ++++
>  drivers/net/phy/phy_device.c                  | 58 +++++++++++++++++++
>  include/linux/phy.h                           |  9 +++
>  3 files changed, 77 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-net-phydev b/Documentation/ABI/testing/sysfs-class-net-phydev
> index ac722dd5e694..a99bbfeddb6f 100644
> --- a/Documentation/ABI/testing/sysfs-class-net-phydev
> +++ b/Documentation/ABI/testing/sysfs-class-net-phydev
> @@ -63,3 +63,13 @@ Description:
>  		only used internally by the kernel and their placement are
>  		not meant to be stable across kernel versions. This is intended
>  		for facilitating the debugging of PHY drivers.
> +
> +What:		/sys/class/mdio_bus/<bus>/<device>/adjust_latency
> +Date:		April 2022
> +KernelVersion:	5.19
> +Contact:	netdev@...r.kernel.org
> +Description:
> +		This file adjusts the latency in the PHY. To set value,
> +		write three integers into the file: interface mode, RX latency,
> +		TX latency. When the file is read, it returns the supported
> +		interface modes and the latency values.

https://lwn.net/Articles/378884/

	The key design goal relating to attribute files is the
	stipulation - almost a mantra - of "one file, one value" or
	sometimes "one item per file". The idea here is that each
	attribute file should contain precisely one value. If multiple
	values are needed, then multiple files should be used.

You also need to specify units in the documentation.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 8406ac739def..80bf04ca0e02 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -529,6 +529,48 @@ static ssize_t phy_dev_flags_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(phy_dev_flags);
>  
> +static ssize_t adjust_latency_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct phy_device *phydev = to_phy_device(dev);
> +	ssize_t count = 0;
> +	int err, i;
> +	s32 rx, tx;
> +
> +	for (i = 0; i < __ETHTOOL_LINK_MODE_MASK_NBITS; ++i) {
> +		err = phydev->drv->get_adj_latency(phydev, i, &rx, &tx);
> +		if (err == -EINVAL)

-EOPNOTSUPP seems more likley than EINVAL.

> +			continue;
> +
> +		count += sprintf(&buf[count], "%d rx: %d, tx: %d\n", i, rx, tx);

Link modes as a number? Can you tell me off the top of you head what
link mode 42 is?

Also, if phydev->drv->get_adj_latency() returns any other error, it is
likely rx and tx contain rubbish, yet you still add them to the
output.

> @@ -932,6 +932,15 @@ struct phy_driver {
>  	int (*get_sqi)(struct phy_device *dev);
>  	/** @get_sqi_max: Get the maximum signal quality indication */
>  	int (*get_sqi_max)(struct phy_device *dev);
> +	/** @set_adj_latency: Set the latency values of the PHY */
> +	int (*set_adj_latency)(struct phy_device *dev,
> +			       enum ethtool_link_mode_bit_indices link_mode,
> +			       s32 rx, s32 tx);
> +	/** @get_latency: Get the latency values of the PHY */
> +	int (*get_adj_latency)(struct phy_device *dev,
> +			       enum ethtool_link_mode_bit_indices link_mode,
> +			       s32 *rx, s32 *tx);

You need to clearly document the return value here, that -EINVAL (or
maybe -EOPNOTUSPP) has special meaning. I would also document the
units, and what is supposed to happen if the stamper already has a
hard coded offset.

     Andrew