Re: [PATCH net 1/5] net: hns3: unify copper port ksettings configuration path

[Jijie Shao]

on 2026/6/18 23:33, Simon Horman wrote:
> 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.
>
> > 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
> [ ... ]
> > @@ -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.

[Accepted] Yes, this is indeed an effective bug.

> > -		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);
> > +	}
> [ ... ]
> > 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
> [ ... ]
> > @@ -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.

[Accepted] Yes, this is indeed an effective bug.


Thanks,
Jijie Shao