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

[Paolo Abeni <pabeni@redhat.com>]

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?

[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?

>  
>  	ret = hclge_set_autoneg_speed_dup(hdev);
>  	if (ret) {
-- 
This is an AI-generated review.