Received-SPF: pass (google.com: domain of linux-kernel+bounces-212408-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 12 Jun 2024 17:35:05 -0700
From: Jakub Kicinski <kuba@kernel.org>
To: Justin Lai <justinlai0215@realtek.com>
Cc: <davem@davemloft.net>, <edumazet@google.com>, <pabeni@redhat.com>,
 <linux-kernel@vger.kernel.org>, <netdev@vger.kernel.org>, <andrew@lunn.ch>,
 <jiri@resnulli.us>, <horms@kernel.org>, <rkannoth@marvell.com>,
 <pkshih@realtek.com>, <larry.chiu@realtek.com>
Subject: Re: [PATCH net-next v20 10/13] rtase: Implement ethtool function
Message-ID: <20240612173505.095c4117@kernel.org>
In-Reply-To: <20240607084321.7254-11-justinlai0215@realtek.com>
References: <20240607084321.7254-1-justinlai0215@realtek.com>
	<20240607084321.7254-11-justinlai0215@realtek.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Fri, 7 Jun 2024 16:43:18 +0800 Justin Lai wrote:
> Implement the ethtool function to support users to obtain network card
> information, including obtaining various device settings, Report whether
> physical link is up, Report pause parameters, Set pause parameters,
> Return a set of strings that describe the requested objects, Get number
> of strings that @get_strings will write, Return extended statistics
> about the device.

You don't implement get_strings any more.

> +static void rtase_get_drvinfo(struct net_device *dev,
> +			      struct ethtool_drvinfo *drvinfo)
> +{
> +	const struct rtase_private *tp = netdev_priv(dev);
> +
> +	strscpy(drvinfo->driver, KBUILD_MODNAME, 32);

sizeof(drvinfo->driver) instead of the literal 32?

> +	strscpy(drvinfo->bus_info, pci_name(tp->pdev), 32);

Can you double check that overwriting these fields is actually needed?
I think core will fill this in for you in ethtool_get_drvinfo()

> +	if ((value & (RTASE_FORCE_TXFLOW_EN | RTASE_FORCE_RXFLOW_EN)) ==
> +	    (RTASE_FORCE_TXFLOW_EN | RTASE_FORCE_RXFLOW_EN)) {
> +		pause->rx_pause = 1;
> +		pause->tx_pause = 1;
> +	} else if ((value & RTASE_FORCE_TXFLOW_EN)) {

unnecessary parenthesis

> +		pause->tx_pause = 1;
> +	} else if ((value & RTASE_FORCE_RXFLOW_EN)) {

same here

> +		pause->rx_pause = 1;
> +	}
> +}
> +
> +static int rtase_set_pauseparam(struct net_device *dev,
> +				struct ethtool_pauseparam *pause)
> +{
> +	const struct rtase_private *tp = netdev_priv(dev);
> +	u16 value = rtase_r16(tp, RTASE_CPLUS_CMD);
> +
> +	if (pause->autoneg)
> +		return -EOPNOTSUPP;
> +
> +	value &= ~(RTASE_FORCE_TXFLOW_EN | RTASE_FORCE_RXFLOW_EN);
> +
> +	if (pause->tx_pause)
> +		value |= RTASE_FORCE_TXFLOW_EN;
> +
> +	if (pause->rx_pause)
> +		value |= RTASE_FORCE_RXFLOW_EN;
> +
> +	rtase_w16(tp, RTASE_CPLUS_CMD, value);
> +	return 0;
> +}
> +
> +static void rtase_get_eth_mac_stats(struct net_device *dev,
> +				    struct ethtool_eth_mac_stats *stats)
> +{
> +	struct rtase_private *tp = netdev_priv(dev);
> +	const struct rtase_counters *counters;
> +
> +	counters = tp->tally_vaddr;
> +	if (!counters)

you fail probe if this is NULL, why check if here?

> +		return;
> +
> +	rtase_dump_tally_counter(tp);
> +
> +	stats->FramesTransmittedOK = le64_to_cpu(counters->tx_packets);
> +	stats->SingleCollisionFrames = le32_to_cpu(counters->tx_one_collision);
> +	stats->MultipleCollisionFrames =
> +		le32_to_cpu(counters->tx_multi_collision);
> +	stats->FramesReceivedOK = le64_to_cpu(counters->rx_packets);
> +	stats->FrameCheckSequenceErrors = le32_to_cpu(counters->rx_errors);

You dont report this in rtase_get_stats64() as crc errors, are these
really CRC / FCS errors or other errors?

> +	stats->AlignmentErrors = le16_to_cpu(counters->align_errors);
> +	stats->FramesAbortedDueToXSColls = le16_to_cpu(counters->tx_aborted);
> +	stats->FramesLostDueToIntMACXmitError =
> +		le64_to_cpu(counters->tx_errors);
> +	stats->FramesLostDueToIntMACRcvError =
> +		le16_to_cpu(counters->rx_missed);

Are you sure this is the correct statistic to report as?
What's the definition of rx_missed in the datasheet?

Also is 16 bits enough for a packet counter at 5Gbps?
Don't you have to periodically accumulate this counter so that it
doesn't wrap around?

> +	stats->MulticastFramesReceivedOK = le32_to_cpu(counters->rx_multicast);
> +	stats->BroadcastFramesReceivedOK = le64_to_cpu(counters->rx_broadcast);
> +}
> +
> +static const struct ethtool_ops rtase_ethtool_ops = {
> +	.get_drvinfo = rtase_get_drvinfo,
> +	.get_link = ethtool_op_get_link,
> +	.get_link_ksettings = rtase_get_settings,
> +	.get_pauseparam = rtase_get_pauseparam,
> +	.set_pauseparam = rtase_set_pauseparam,
> +	.get_eth_mac_stats = rtase_get_eth_mac_stats,
> +	.get_ts_info = ethtool_op_get_ts_info,
> +};
> +
>  static void rtase_init_netdev_ops(struct net_device *dev)
>  {
>  	dev->netdev_ops = &rtase_netdev_ops;
> +	dev->ethtool_ops = &rtase_ethtool_ops;
>  }
>  
>  static void rtase_reset_interrupt(struct pci_dev *pdev,