Re: [PATCH net 1/5] net: hns3: unify copper port ksettings configuration path

[Simon Horman <horms@kernel.org>]

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: hns3: unify copper port ksettings configuration path

Refactor hns3_set_link_ksettings() and hclge_set_phy_link_ksettings()
to unify the configuration path for copper ports.

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
[ ... ]
> @@ -863,6 +859,8 @@ static int hns3_set_link_ksettings(struct net_device *netdev,
>  	struct hnae3_handle *handle = hns3_get_handle(netdev);
>  	struct hnae3_ae_dev *ae_dev = hns3_get_ae_dev(handle);
>  	const struct hnae3_ae_ops *ops = hns3_get_ops(handle);
> +	u8 module_type = HNAE3_MODULE_TYPE_UNKNOWN;
> +	u8 media_type = HNAE3_MEDIA_TYPE_UNKNOWN;
>  	int ret;
>  
>  	/* Chip don't support this mode. */
> @@ -878,22 +876,20 @@ static int hns3_set_link_ksettings(struct net_device *netdev,
>  		  cmd->base.autoneg, cmd->base.speed, cmd->base.duplex,
>  		  cmd->lanes);
>  
> -	/* Only support ksettings_set for netdev with phy attached for now */
> -	if (netdev->phydev) {
> -		if (cmd->base.speed == SPEED_1000 &&
> -		    cmd->base.autoneg == AUTONEG_DISABLE)
> -			return -EINVAL;

[Severity: Medium]
Does removing this validation allow invalid configurations for native copper
PHYs?

The previous code explicitly rejected SPEED_1000 with AUTONEG_DISABLE because
IEEE 802.3 1000BASE-T strictly requires autonegotiation. By delegating
directly to phy_ethtool_ksettings_set(), which permits this for 1000BASE-X,
the driver might now silently accept an invalid configuration.

> -		return phy_ethtool_ksettings_set(netdev->phydev, cmd);
> -	} else if (test_bit(HNAE3_DEV_SUPPORT_PHY_IMP_B, ae_dev->caps) &&
> -		   ops->set_phy_link_ksettings) {
> +	if (!ops->get_media_type)
> +		return -EOPNOTSUPP;
> +	ops->get_media_type(handle, &media_type, &module_type);
>  
> -		return ops->set_phy_link_ksettings(handle, cmd);
> -	}
> +	if (media_type == HNAE3_MEDIA_TYPE_COPPER) {
> +		if (!ops->set_phy_link_ksettings)
> +			return -EOPNOTSUPP;
> +		return ops->set_phy_link_ksettings(handle, cmd);
> +	}
[ ... ]
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
[ ... ]
> @@ -3400,10 +3400,28 @@ hclge_set_phy_link_ksettings(struct hnae3_handle *handle,
>  		return ret;
>  	}
>  
> +	linkmode_copy(hdev->hw.mac.advertising, cmd->link_modes.advertising);
> +	return 0;
> +}
> +
> +static int
> +hclge_set_phy_link_ksettings(struct hnae3_handle *handle,
> +			     const struct ethtool_link_ksettings *cmd)
> +{
> +	struct hclge_vport *vport = hclge_get_vport(handle);
> +	struct hclge_dev *hdev = vport->back;
> +	int ret;
> +
> +	if (hnae3_dev_phy_imp_supported(hdev))
> +		ret = hclge_ethtool_ksettings_set(handle, cmd);
> +	else
> +		ret = phy_ethtool_ksettings_set(handle->netdev->phydev, cmd);

[Severity: Critical]
Can handle->netdev->phydev be NULL here, leading to a kernel panic when called
from userspace?

A copper port can legitimately have a NULL phydev if the port's PHY address
is configured as PHY_INEXISTENT in the firmware. 

Because the updated hns3_set_link_ksettings() now routes all 
HNAE3_MEDIA_TYPE_COPPER configurations here without the previous 
if (netdev->phydev) guard, phy_ethtool_ksettings_set() will unconditionally
dereference phydev->mdio.addr.

> +	if (ret)
> +		return ret;
> +
>  	hdev->hw.mac.req_autoneg = cmd->base.autoneg;
>  	hdev->hw.mac.req_speed = cmd->base.speed;
>  	hdev->hw.mac.req_duplex = cmd->base.duplex;