Re: [PATCH net-next v20 10/13] rtase: Implement ethtool function

[Jakub Kicinski]

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,