Re: [PATCH net-next v9 2/3] net: ethtool: add KSZ87xx low-loss cable PHY tunables

[Paolo Abeni]

From: AI Reviewer <ai@xxxxxxxxxxx>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/
---
net: ethtool: add KSZ87xx low-loss cable PHY tunables

Introduce vendor-specific PHY tunable identifiers to control the
KSZ87xx low-loss cable erratum handling through the ethtool PHY
tunable interface.

> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 1cdfb8341df24..a2091d4e00f3f 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -291,6 +291,9 @@ enum phy_tunable_id {
>  	ETHTOOL_PHY_DOWNSHIFT,
>  	ETHTOOL_PHY_FAST_LINK_DOWN,
>  	ETHTOOL_PHY_EDPD,
> +	ETHTOOL_PHY_SHORT_CABLE_PRESET,
> +	ETHTOOL_PHY_LPF_BW,
> +	ETHTOOL_PHY_DSP_EQ_INIT_VALUE,

[Severity: High]
Are these vendor-specific hardware erratum tunables appropriate for the 
generic global ethtool UAPI? 

These tunables use generic names for device-specific KSZ87xx workarounds. 
Since UAPI is permanent, adding hardware-tied erratum parameters without a 
vendor prefix or standard abstraction could lead to namespace pollution. Would 
devlink param or device tree properties be a better fit for this?

[ ... ]

> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index bd97f9b9bf182..fc25424228246 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -3114,6 +3114,9 @@ static int ethtool_phy_tunable_valid(const struct ethtool_tunable *tuna)
>  	switch (tuna->id) {
>  	case ETHTOOL_PHY_DOWNSHIFT:
>  	case ETHTOOL_PHY_FAST_LINK_DOWN:
> +	case ETHTOOL_PHY_SHORT_CABLE_PRESET:
> +	case ETHTOOL_PHY_LPF_BW:
> +	case ETHTOOL_PHY_DSP_EQ_INIT_VALUE:
>  		if (tuna->len != sizeof(u8) ||
>  		    tuna->type_id != ETHTOOL_TUNABLE_U8)
>  			return -EINVAL;

[Severity: High]
Is it intentional to enforce an 8-bit type constraint for bandwidth and DSP
equalizer tunables?

This heavily couples the global API to the 8-bit register layout of the
KSZ87xx hardware without defining generic units like Hz for bandwidth. 

Also, in the same file:

> net/ethtool/ioctl.c:set_phy_tunable() {
> ...
> 	phy_drv_tunable = phydev && phydev->drv && phydev->drv->get_tunable;
> 	if (!phy_drv_tunable && !dev->ethtool_ops->set_phy_tunable)
> 		return -EOPNOTSUPP;
> ...
> 	if (phy_drv_tunable) {
> 		mutex_lock(&phydev->lock);
> 		ret = phydev->drv->set_tunable(phydev, &tuna, data);
> 		mutex_unlock(&phydev->lock);
> 	}
> ...
> }

[Severity: High]
This is a pre-existing issue, but does this code unconditionally dereference
set_tunable if get_tunable is present?

In set_phy_tunable(), phy_drv_tunable is determined by checking
phydev->drv->get_tunable instead of set_tunable. If a driver implements
get_tunable but leaves set_tunable as NULL, the capability check passes, which
would result in a NULL pointer dereference when set_tunable is called.
-- 
This is an AI-generated review.