Re: [PATCH net-next v9 3/3] net: phy: micrel: expose KSZ87xx low-loss cable 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: phy: micrel: expose KSZ87xx low-loss cable tunables

This commit adds support for the KSZ87xx low-loss cable PHY tunables in the
Micrel PHY driver. It implements the get_tunable and set_tunable callbacks
to control receiver behavior. These callbacks are forwarded via standard
phy_read() and phy_write() operations.

> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index e211a523c2584..bb8f3054e2574 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c

[ ... ]

> @@ -6961,6 +7013,8 @@ static struct phy_driver ksphy_driver[] = {
>  	/* PHY_BASIC_FEATURES */
>  	.config_init	= kszphy_config_init,
>  	.match_phy_device = ksz8795_match_phy_device,
> +	.get_tunable	= ksz8795_get_tunable,
> +	.set_tunable	= ksz8795_set_tunable,

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the addition
of these callbacks, I noticed a pre-existing issue in the ethtool core.

In net/ethtool/ioctl.c:set_phy_tunable(), the capability check wrongly examines
phydev->drv->get_tunable instead of set_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);

Since the capability check looks at get_tunable but unconditionally calls
set_tunable, could this lead to a NULL pointer dereference if a PHY driver
were to implement only read-only tunables by providing just get_tunable?

>  	.suspend	= genphy_suspend,
>  	.resume		= genphy_resume,
>  }, {
-- 
This is an AI-generated review.