Re: [PATCH net-next v9 2/3] net: ethtool: add KSZ87xx low-loss cable PHY tunables
From: Paolo Abeni <pabeni@redhat.com>
Date: 2026-06-04 11:41:42
Also in:
lkml
From: AI Reviewer <redacted> 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.
quoted hunk ↗ jump to hunk
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? [ ... ]
quoted hunk ↗ jump to hunk
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.