Re: [PATCH net 4/5] net: hns3: differentiate autoneg default values between copper and fiber

[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: differentiate autoneg default values between copper and fiber

This commit addresses a link loss issue during driver initialization on
optical ports connected to forced-mode remote switches. It modifies
hclge_init_ae_dev to set autonegotiation based on media type. Copper ports
retain the AUTONEG_ENABLE default, while optical ports inherit the firmware
pre-configured autoneg setting.

> 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
> @@ -11916,6 +11916,9 @@ static int hclge_init_ae_dev(struct hnae3_ae_dev *ae_dev)
>  	if (ret)
>  		goto err_ptp_uninit;
>  
> +	if (hdev->hw.mac.media_type != HNAE3_MEDIA_TYPE_COPPER)
> +		hdev->hw.mac.req_autoneg = hdev->hw.mac.autoneg;
> +

[Severity: High]
Does this apply the wrong speed when autonegotiation is disabled on fiber
ports?

During driver initialization, hclge_update_port_info() dynamically probes
the SFP module and updates mac->speed, but does not update mac->req_speed,
which retains its statically configured default capability.

When hdev->hw.mac.autoneg is 0 (forced-mode), req_autoneg becomes 0, and
then hclge_set_autoneg_speed_dup() uses req_speed:

drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c:hclge_set_autoneg_speed_dup() {
...
    if (!hdev->hw.mac.req_autoneg) {
        ret = hclge_cfg_mac_speed_dup_hw(hdev, hdev->hw.mac.req_speed,
...
}

Could this override the dynamically probed SFP speed with the static default
speed?

[Severity: High]
Does this unconditionally disable autonegotiation for V1 optical hardware?

Looking at hclge_update_port_info(), it only queries SFP information via
hclge_get_sfp_info() for V2 hardware. For V1 hardware, it falls back to
hclge_get_sfp_speed(), which leaves mac->autoneg uninitialized (0):

drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c:hclge_update_port_info() {
...
    if (hdev->ae_dev->dev_version >= HNAE3_DEVICE_VERSION_V2) {
        speed = mac->speed;
        ret = hclge_get_sfp_info(hdev, mac);
    } else {
        speed = HCLGE_MAC_SPEED_UNKNOWN;
        ret = hclge_get_sfp_speed(hdev, &speed);
    }
...
}

If mac->autoneg is left as 0, wouldn't req_autoneg unconditionally be set
to 0 here, overriding the speed and breaking connectivity for V1 devices?

>  	ret = hclge_set_autoneg_speed_dup(hdev);
>  	if (ret) {
>  		dev_err(&pdev->dev,