Re: [PATCH net 1/5] net: hns3: unify copper port ksettings configuration path
From: Simon Horman <horms@kernel.org>
Date: 2026-06-18 15:34:03
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://sashiko.dev --- net: hns3: unify copper port ksettings configuration path Refactor hns3_set_link_ksettings() and hclge_set_phy_link_ksettings() to unify the configuration path for copper ports.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c --- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -863,6 +859,8 @@ static int hns3_set_link_ksettings(struct net_device *netdev, struct hnae3_handle *handle = hns3_get_handle(netdev); struct hnae3_ae_dev *ae_dev = hns3_get_ae_dev(handle); const struct hnae3_ae_ops *ops = hns3_get_ops(handle); + u8 module_type = HNAE3_MODULE_TYPE_UNKNOWN; + u8 media_type = HNAE3_MEDIA_TYPE_UNKNOWN; int ret; /* Chip don't support this mode. */@@ -878,22 +876,20 @@ static int hns3_set_link_ksettings(struct net_device *netdev, cmd->base.autoneg, cmd->base.speed, cmd->base.duplex, cmd->lanes); - /* Only support ksettings_set for netdev with phy attached for now */ - if (netdev->phydev) { - if (cmd->base.speed == SPEED_1000 && - cmd->base.autoneg == AUTONEG_DISABLE) - return -EINVAL;
[Severity: Medium] Does removing this validation allow invalid configurations for native copper PHYs? The previous code explicitly rejected SPEED_1000 with AUTONEG_DISABLE because IEEE 802.3 1000BASE-T strictly requires autonegotiation. By delegating directly to phy_ethtool_ksettings_set(), which permits this for 1000BASE-X, the driver might now silently accept an invalid configuration.
- return phy_ethtool_ksettings_set(netdev->phydev, cmd);
- } else if (test_bit(HNAE3_DEV_SUPPORT_PHY_IMP_B, ae_dev->caps) &&
- ops->set_phy_link_ksettings) {
+ if (!ops->get_media_type)
+ return -EOPNOTSUPP;
+ ops->get_media_type(handle, &media_type, &module_type);
- return ops->set_phy_link_ksettings(handle, cmd);
- }
+ if (media_type == HNAE3_MEDIA_TYPE_COPPER) {
+ if (!ops->set_phy_link_ksettings)
+ return -EOPNOTSUPP;
+ return ops->set_phy_link_ksettings(handle, cmd);
+ }[ ... ]
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 --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -3400,10 +3400,28 @@ hclge_set_phy_link_ksettings(struct hnae3_handle *handle, return ret; } + linkmode_copy(hdev->hw.mac.advertising, cmd->link_modes.advertising); + return 0; +} + +static int +hclge_set_phy_link_ksettings(struct hnae3_handle *handle, + const struct ethtool_link_ksettings *cmd) +{ + struct hclge_vport *vport = hclge_get_vport(handle); + struct hclge_dev *hdev = vport->back; + int ret; + + if (hnae3_dev_phy_imp_supported(hdev)) + ret = hclge_ethtool_ksettings_set(handle, cmd); + else + ret = phy_ethtool_ksettings_set(handle->netdev->phydev, cmd);
[Severity: Critical] Can handle->netdev->phydev be NULL here, leading to a kernel panic when called from userspace? A copper port can legitimately have a NULL phydev if the port's PHY address is configured as PHY_INEXISTENT in the firmware. Because the updated hns3_set_link_ksettings() now routes all HNAE3_MEDIA_TYPE_COPPER configurations here without the previous if (netdev->phydev) guard, phy_ethtool_ksettings_set() will unconditionally dereference phydev->mdio.addr.
+ if (ret) + return ret; + hdev->hw.mac.req_autoneg = cmd->base.autoneg; hdev->hw.mac.req_speed = cmd->base.speed; hdev->hw.mac.req_duplex = cmd->base.duplex;