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

[Jijie Shao <shaojijie@huawei.com>]

on 2026/6/18 23:33, Simon Horman wrote:
> 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.

[Accepted] Yes, this is indeed an effective bug.

>
>> -		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.

[Accepted] Yes, this is indeed an effective bug.


Thanks,
Jijie Shao