Re: [PATCH net 2/5] net: hns3: refactor MAC autoneg and speed configuration
From: Simon Horman <horms@kernel.org>
Date: 2026-06-18 15:34:43
Also in:
lkml
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().
quoted hunk ↗ jump to hunk
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;
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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?
quoted hunk ↗ jump to hunk
@@ -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.