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

[Jijie Shao]

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