Re: [RFC PATCH net-next 08/10] net: hibmcge: Implement workqueue and some ethtool_ops functions

[Jijie Shao <shaojijie@huawei.com>]

on 2024/8/1 9:10, Andrew Lunn wrote:
>> +static void hbg_ethtool_get_drvinfo(struct net_device *netdev,
>> +				    struct ethtool_drvinfo *drvinfo)
>> +{
>> +	strscpy(drvinfo->version, HBG_MOD_VERSION, sizeof(drvinfo->version));
>> +	drvinfo->version[sizeof(drvinfo->version) - 1] = '\0';
> A version is pointless, it tells you nothing useful. If you don't
> touch version, the core will fill it with the uname, so you know
> exactly what kernel this is, which is useful.
>
>> +static u32 hbg_ethtool_get_link(struct net_device *netdev)
>> +{
>> +	struct hbg_priv *priv = netdev_priv(netdev);
>> +
>> +	return priv->mac.link_status;
>> +}
>> +
>> +static int hbg_ethtool_get_ksettings(struct net_device *netdev,
>> +				     struct ethtool_link_ksettings *ksettings)
>> +{
>> +	struct hbg_priv *priv = netdev_priv(netdev);
>> +
>> +	phy_ethtool_ksettings_get(priv->mac.phydev, ksettings);
> You can avoid this wrapper since phy_attach_direct sets netdev->phydev
> to phydev. You can then call phy_ethtool_get_link_ksettings() etc.

Yes, It`s ok

>
>> +static int hbg_ethtool_set_ksettings(struct net_device *netdev,
>> +				     const struct ethtool_link_ksettings *cmd)
>> +{
>> +	struct hbg_priv *priv = netdev_priv(netdev);
>> +
>> +	if (cmd->base.speed == SPEED_1000 && cmd->base.duplex == DUPLEX_HALF)
>> +		return -EINVAL;
> So long as you have told phylib about not supporting 1000Base/Half,
> phy_ethtool_set_link_ksettings() will return an error for you.

okay,

>
>> +static const struct ethtool_ops hbg_ethtool_ops = {
>> +	.get_drvinfo		= hbg_ethtool_get_drvinfo,
>> +	.get_link		= hbg_ethtool_get_link,
> Why not ethtool_op_get_link() ?

It`s a good idea

>
>> +	.get_link_ksettings	= hbg_ethtool_get_ksettings,
>> +	.set_link_ksettings	= hbg_ethtool_set_ksettings,
>> +};
>> +static void hbg_update_link_status(struct hbg_priv *priv)
>> +{
>> +	u32 link;
>> +
>> +	link = hbg_get_link_status(priv);
>> +	if (link == priv->mac.link_status)
>> +		return;
>> +
>> +	priv->mac.link_status = link;
>> +	if (link == HBG_LINK_DOWN) {
>> +		netif_carrier_off(priv->netdev);
>> +		netif_tx_stop_all_queues(priv->netdev);
>> +		dev_info(&priv->pdev->dev, "link down!");
>> +	} else {
>> +		netif_tx_wake_all_queues(priv->netdev);
>> +		netif_carrier_on(priv->netdev);
>> +		dev_info(&priv->pdev->dev, "link up!");
>> +	}
>> +}
> Why do you need this? phylib will poll the PHY once per second and
> call the adjust_link callback whenever the link changes state.

However, we hope that the network port can be linked only when
the PHY and MAC are linked.
The adjust_link callback can ensure that the PHY status is normal,
but cannot ensure that the MAC address is linked.

so, in hbg_get_link_status:
+/* include phy link and mac link */
+u32 hbg_get_link_status(struct hbg_priv *priv)
+{
+	struct phy_device *phydev = priv->mac.phydev;
+	int ret;
+
+	if (!phydev)
+		return HBG_LINK_DOWN;
+
+	phy_read_status(phydev);
+	if ((phydev->state != PHY_UP && phydev->state != PHY_RUNNING) ||
+	    !phydev->link)
+		return HBG_LINK_DOWN;
+
+	ret = hbg_hw_sgmii_autoneg(priv);
+	if (ret)
+		return HBG_LINK_DOWN;
+
+	return HBG_LINK_UP;
+}

>
>> @@ -177,12 +226,17 @@ static int hbg_init(struct net_device *netdev)
>>   	ret = hbg_irq_init(priv);
>>   	if (ret)
>>   		return ret;
>> -
>>   	ret = devm_add_action_or_reset(&priv->pdev->dev, hbg_irq_uninit, priv);
> This white space change does not belong here.

Yes, I'll fix that in V2

Thanks again,

Jijie Shao