Re: [PATCH V2 net] net: hns3: fix speed configuration residue after driver reload

[Jijie Shao <shaojijie@huawei.com>]

on 2026/7/21 19:05, Paolo Abeni wrote:
> From: AI Reviewer <ai@example.com>
>
> 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: fix speed configuration residue after driver reload
>
> This change drops the req_speed overwrite in hclge_init_ae_dev() so
> req_speed keeps the firmware default set during hclge_configure()
> after a driver reload, and only req_autoneg continues to be
> synchronized from mac.autoneg on non-copper media.
>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
>> index fc8587c80813..164c3ecf195c 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
>> @@ -9498,12 +9498,8 @@ 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) {
>> +	if (hdev->hw.mac.media_type != HNAE3_MEDIA_TYPE_COPPER)
>>   		hdev->hw.mac.req_autoneg = hdev->hw.mac.autoneg;
>> -		if (hdev->hw.mac.autoneg == AUTONEG_DISABLE &&
>> -		    hdev->hw.mac.speed != SPEED_UNKNOWN)
>> -			hdev->hw.mac.req_speed = hdev->hw.mac.speed;
>> -	}
> [High]
> Could removing this block reintroduce the scenario that d9d349c4e8a0
> ("net: hns3: differentiate autoneg default values between copper and
> fiber") was written to handle?
>
> The Fixes-tagged commit added this req_speed overwrite specifically for
> optical ports where firmware presets autoneg=DISABLE at a specific
> forced speed (a case its commit message describes as common in data
> centers where the remote switch runs in forced mode).
>
> There appear to be two distinct notions of "firmware default" involved:
>
>    - cfg.default_speed read from flash static parameters via
>      hclge_get_cfg() and stored into req_speed by hclge_configure()
>
>    - the live firmware/SFP state read via hclge_get_sfp_info() into
>      mac.autoneg and mac.speed
>
> On a fiber port where the flash default_speed is 100G but firmware has
> preset the port to forced 25G to match a fixed-speed remote, after this
> change:
>
>      hdev->hw.mac.req_autoneg = hdev->hw.mac.autoneg;  /* = DISABLE */
>      /* req_speed still holds flash default 100G */
>
> hclge_set_autoneg_speed_dup() would then see !req_autoneg and call:
>
>      hclge_cfg_mac_speed_dup_hw(hdev, hdev->hw.mac.req_speed=100G, ...);
>
> forcing 100G against a 25G-only peer.
>
> Is that scenario still handled, or is it expected not to occur in
> practice?

We want driver reload to reset MAC speed to the firmware default.
If that doesn't match a forced-mode remote on an optical port,
link drops and the user reconfigures with ethtool. That's fine.

The req_speed = mac.speed line was added in the V2 of the June
series to avoid that link drop, but it caused the actual reported
bug: speed stays at the last user setting across reload. This
patch drops that line and goes back to the V1 behavior, which
only synced req_autoneg.

   V1:
   https://lore.kernel.org/all/20260617112721.75186-5-shaojijie@huawei.com/

   V2:
   https://lore.kernel.org/all/20260624141319.271439-5-shaojijie@huawei.com/

If we need to cover the forced-mode optical case without
reintroducing the residue, that's a separate problem — a one-line
req_speed = mac.speed can't tell user-configured speed apart
from firmware-preset speed.

>
> [Medium]
> The commit message reasons about mac.speed carrying over user ethtool
> changes across a driver reload:
>
>    "In hclge_init_ae_dev(), hclge_update_port_info() reads mac.speed
>     from hardware, which reflects the last user configuration (e.g.
>     ethtool changes), not the firmware default."
>
> Does the same reasoning apply to mac.autoneg?
>
> hclge_get_sfp_info() sets mac->autoneg from the live firmware state, so
> a prior "ethtool -s ethX autoneg off speed 40000" on a 100G optical
> port would leave firmware reporting autoneg=DISABLE after reload.
>
> After this change, req_speed correctly reverts to the flash default
> 100G, but this line still runs:
>
>      hdev->hw.mac.req_autoneg = hdev->hw.mac.autoneg;
>
> so req_autoneg stays at AUTONEG_DISABLE rather than reverting to the
> AUTONEG_ENABLE default that hclge_configure() would set for fiber.
>
> Why is speed residue fixed while autoneg residue is preserved?

The asymmetry is on purpose. req_autoneg has been inherited from 
mac.autoneg since long before this work; that behavior has always been 
there and is left unchanged to avoid surprising users. req_speed = 
mac.speed was added in June V2 and is what caused the reported residue, 
so dropping it is the fix. I'll send a v3 with the commit message noting 
that link loss on forced-mode optical ports after reload is expected. 
Best, Jijie