Re: [PATCH net 2/5] net: hns3: refactor MAC autoneg and speed configuration

[Simon Horman]

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://netdev-ai.bots.linux.dev/sashiko/
---
net: hns3: refactor MAC autoneg and speed configuration

This refactor extracts the MAC autoneg and speed/duplex/lane configuration
out of hclge_mac_init() into a new helper hclge_set_autoneg_speed_dup(),
and invokes it from both hclge_init_ae_dev() and hclge_reset_ae_dev().

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> index 5a00797d9252..2c74675b149f 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> @@ -2957,20 +2957,6 @@ static int hclge_mac_init(struct hclge_dev *hdev)
>  	if (!test_bit(HCLGE_STATE_RST_HANDLING, &hdev->state))
>  		hdev->hw.mac.duplex = HCLGE_MAC_FULL;
>  
> -	if (hdev->hw.mac.support_autoneg) {
> -		ret = hclge_set_autoneg_en(hdev, hdev->hw.mac.autoneg);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	if (!hdev->hw.mac.autoneg) {
> -		ret = hclge_cfg_mac_speed_dup_hw(hdev, hdev->hw.mac.req_speed,
> -						 hdev->hw.mac.req_duplex,
> -						 hdev->hw.mac.lane_num);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	mac->link = 0;

[ ... ]

> @@ -11901,6 +11908,13 @@ static int hclge_init_ae_dev(struct hnae3_ae_dev *ae_dev)
>  	if (ret)
>  		goto err_ptp_uninit;
>  
> +	ret = hclge_set_autoneg_speed_dup(hdev);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"failed to set autoneg speed duplex, ret = %d\n", ret);
> +		goto err_ptp_uninit;
> +	}
> +

[High]

Has the relocation of the autoneg/speed configuration in hclge_init_ae_dev()
been considered against the surrounding init order?

Previously the equivalent calls ran inside hclge_mac_init() before
hclge_update_port_info(). After this patch the helper runs after
hclge_update_port_info(), and hclge_update_port_info() ->
hclge_get_sfp_info() unconditionally overwrites hdev->hw.mac.autoneg,
hdev->hw.mac.support_autoneg and hdev->hw.mac.lane_num from firmware
whenever resp->speed_ability is non-zero.

The new helper then feeds the firmware-supplied lane_num back through:

    if (!hdev->hw.mac.autoneg) {
        ret = hclge_cfg_mac_speed_dup_hw(hdev, hdev->hw.mac.req_speed,
                                         hdev->hw.mac.req_duplex,
                                         hdev->hw.mac.lane_num);

On a non-copper port where firmware reports forced mode (autoneg=0) and a
stale lane_num inherited from a previous link lifecycle (for example
lane_num=4 from a prior 100G state), the (req_speed, lane_num) pair given
to hclge_cfg_mac_speed_dup_hw() can be inconsistent (for example 25G with
4 lanes), and firmware rejects it with -EINVAL, which propagates back to
hclge_init_ae_dev() and unwinds via err_ptp_uninit, failing the entire
probe.

The follow-up commit a7e52c16ecbf "net: hns3: fix init failure caused by
lane_num contamination" later in this series fixes this by introducing a
separate req_lane_num=0 and passing that to hclge_cfg_mac_speed_dup_hw()
instead of mac.lane_num. Should the fix be folded into this patch so the
series does not introduce a probe-failure regression at this point and
then repair it?

[Medium]

After this change, the last write to MAC speed/duplex on the init path is
hclge_cfg_mac_speed_dup_hw() inside the new helper, which programs
hardware but does not update the cached hdev->hw.mac.speed and
hdev->hw.mac.duplex. Previously the last writer was
hclge_cfg_mac_speed_dup() inside hclge_update_port_info(), which updates
both hardware and the cached fields.

When req_speed differs from the firmware-reported mac.speed (which can
happen if the active SFP speed differs from cfg.default_speed parsed in
hclge_configure()), is hardware left programmed with req_speed while
mac.speed retains the firmware-reported value, leaving the cache out of
sync with the actual hardware configuration until the periodic service
task refreshes mac.speed?

> @@ -12231,6 +12245,13 @@ static int hclge_reset_ae_dev(struct hnae3_ae_dev *ae_dev)
>  		return ret;
>  	}
>  
> +	ret = hclge_set_autoneg_speed_dup(hdev);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"failed to set autoneg speed duplex, ret = %d\n", ret);
> +		return ret;
> +	}
> +
>  	ret = hclge_tp_port_init(hdev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to init tp port, ret = %d\n",

[Low]

Could the commit message be tightened to match the resulting code?

The message says the helper "will also be invoked during the hardware
reset recovery path to re-apply link settings without repeating
unnecessary buffer or MTU initializations." However hclge_reset_ae_dev()
still calls hclge_mac_init() immediately before hclge_set_autoneg_speed_dup(),
and hclge_mac_init() still performs hclge_set_mac_mtu(),
hclge_set_default_loopback() and hclge_buffer_alloc(), so the avoidance
of MTU/buffer re-initialization described in the changelog is not
realized here.

The commit message is also framed as a pure readability/cyclomatic-
complexity refactor, but the autoneg/speed configuration is not just
extracted: in hclge_init_ae_dev() it is moved to a materially later
point (after hclge_update_port_info(), TSO, GRO, VLAN, TM, RSS, FD and
PTP setup), versus the original code where it was the very first
hardware-touching step inside hclge_mac_init(). The reset path keeps the
original relative ordering (helper runs immediately after
hclge_mac_init()), so the init and reset paths are now asymmetric in
placement, and the rationale for the late placement on the init path is
not stated.