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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b20b5d68-2dab-403c-b37b-084218e001bc@lunn.ch>
Date: Thu, 1 Aug 2024 03:10:41 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Jijie Shao <shaojijie@...wei.com>
Cc: yisen.zhuang@...wei.com, salil.mehta@...wei.com, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	horms@...nel.org, shenjian15@...wei.com, wangpeiyang1@...wei.com,
	liuyonglong@...wei.com, sudongming1@...wei.com,
	xujunsheng@...wei.com, shiyongbang@...wei.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH net-next 08/10] net: hibmcge: Implement workqueue and
 some ethtool_ops functions

> +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.

> +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.

> +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() ?

> +	.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.

> @@ -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.

     Andrew