Re: [PATCH V4 net-next 7/8] net: hns3: Add Ethtool support to HNS3 driver

From: Florian Fainelli
Date: Sun Jul 23 2017 - 13:05:12 EST




On 07/22/2017 03:09 PM, Salil Mehta wrote:
> This patch adds the support of the Ethtool interface to
> the HNS3 Ethernet driver. Various commands to read the
> statistics, configure the offloading, loopback selftest etc.
> are supported.
>
> Signed-off-by: Daode Huang <huangdaode@xxxxxxxxxxxxx>
> Signed-off-by: lipeng <lipeng321@xxxxxxxxxx>
> Signed-off-by: Salil Mehta <salil.mehta@xxxxxxxxxx>
> Signed-off-by: Yisen Zhuang <yisen.zhuang@xxxxxxxxxx>
> ---
> Patch V4: addressed below comments
> 1. Andrew Lunn
> Removed the support of loop PHY back for now
> Patch V3: Address below comments
> 1. Stephen Hemminger
> https://lkml.org/lkml/2017/6/13/974
> 2. Andrew Lunn
> https://lkml.org/lkml/2017/6/13/1037
> Patch V2: No change
> Patch V1: Initial Submit
> ---
> .../ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c | 543 +++++++++++++++++++++
> 1 file changed, 543 insertions(+)
> create mode 100644 drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c
>
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c
> new file mode 100644
> index 000000000000..82b0d4d829f8
> --- /dev/null
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c
> @@ -0,0 +1,543 @@
> +/*
> + * Copyright (c) 2016~2017 Hisilicon Limited.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/etherdevice.h>
> +#include "hns3_enet.h"
> +
> +struct hns3_stats {
> + char stats_string[ETH_GSTRING_LEN];
> + int stats_size;
> + int stats_offset;
> +};
> +
> +/* netdev related stats */
> +#define HNS3_NETDEV_STAT(_string, _member) \
> + { _string, \
> + FIELD_SIZEOF(struct rtnl_link_stats64, _member), \
> + offsetof(struct rtnl_link_stats64, _member), \
> + }

Can you make this macro use named initializers?

> +
> +static const struct hns3_stats hns3_netdev_stats[] = {
> + /* misc. Rx/Tx statistics */
> + HNS3_NETDEV_STAT("rx_packets", rx_packets),
> + HNS3_NETDEV_STAT("tx_packets", tx_packets),
> + HNS3_NETDEV_STAT("rx_bytes", rx_bytes),
> + HNS3_NETDEV_STAT("tx_bytes", tx_bytes),
> + HNS3_NETDEV_STAT("rx_errors", rx_errors),
> + HNS3_NETDEV_STAT("tx_errors", tx_errors),
> + HNS3_NETDEV_STAT("rx_dropped", rx_dropped),
> + HNS3_NETDEV_STAT("tx_dropped", tx_dropped),
> + HNS3_NETDEV_STAT("multicast", multicast),
> + HNS3_NETDEV_STAT("collisions", collisions),
> +
> + /* detailed Rx errors */
> + HNS3_NETDEV_STAT("rx_length_errors", rx_length_errors),
> + HNS3_NETDEV_STAT("rx_over_errors", rx_over_errors),
> + HNS3_NETDEV_STAT("rx_crc_errors", rx_crc_errors),
> + HNS3_NETDEV_STAT("rx_frame_errors", rx_frame_errors),
> + HNS3_NETDEV_STAT("rx_fifo_errors", rx_fifo_errors),
> + HNS3_NETDEV_STAT("rx_missed_errors", rx_missed_errors),
> +
> + /* detailed Tx errors */
> + HNS3_NETDEV_STAT("tx_aborted_errors", tx_aborted_errors),
> + HNS3_NETDEV_STAT("tx_carrier_errors", tx_carrier_errors),
> + HNS3_NETDEV_STAT("tx_fifo_errors", tx_fifo_errors),
> + HNS3_NETDEV_STAT("tx_heartbeat_errors", tx_heartbeat_errors),
> + HNS3_NETDEV_STAT("tx_window_errors", tx_window_errors),
> +
> + /* for cslip etc */
> + HNS3_NETDEV_STAT("rx_compressed", rx_compressed),
> + HNS3_NETDEV_STAT("tx_compressed", tx_compressed),
> +};
> +
> +#define HNS3_NETDEV_STATS_COUNT ARRAY_SIZE(hns3_netdev_stats)
> +
> +/* tqp related stats */
> +#define HNS3_TQP_STAT(_string, _member) \
> + { _string, \
> + FIELD_SIZEOF(struct ring_stats, _member), \
> + offsetof(struct hns3_enet_ring, stats), \
> + }
> +

Same here.

> +static const struct hns3_stats hns3_txq_stats[] = {
> + /* Tx per-queue statistics */
> + HNS3_TQP_STAT("tx_io_err_cnt", io_err_cnt),
> + HNS3_TQP_STAT("tx_sw_err_cnt", sw_err_cnt),
> + HNS3_TQP_STAT("tx_seg_pkt_cnt", seg_pkt_cnt),
> + HNS3_TQP_STAT("tx_pkts", tx_pkts),
> + HNS3_TQP_STAT("tx_bytes", tx_bytes),
> + HNS3_TQP_STAT("tx_err_cnt", tx_err_cnt),
> + HNS3_TQP_STAT("tx_restart_queue", restart_queue),
> + HNS3_TQP_STAT("tx_busy", tx_busy),
> +};
> +
> +#define HNS3_TXQ_STATS_COUNT ARRAY_SIZE(hns3_txq_stats)
> +
> +static const struct hns3_stats hns3_rxq_stats[] = {
> + /* Rx per-queue statistics */
> + HNS3_TQP_STAT("rx_io_err_cnt", io_err_cnt),
> + HNS3_TQP_STAT("rx_sw_err_cnt", sw_err_cnt),
> + HNS3_TQP_STAT("rx_seg_pkt_cnt", seg_pkt_cnt),
> + HNS3_TQP_STAT("rx_pkts", rx_pkts),
> + HNS3_TQP_STAT("rx_bytes", rx_bytes),
> + HNS3_TQP_STAT("rx_err_cnt", rx_err_cnt),
> + HNS3_TQP_STAT("rx_reuse_pg_cnt", reuse_pg_cnt),
> + HNS3_TQP_STAT("rx_err_pkt_len", err_pkt_len),
> + HNS3_TQP_STAT("rx_non_vld_descs", non_vld_descs),
> + HNS3_TQP_STAT("rx_err_bd_num", err_bd_num),
> + HNS3_TQP_STAT("rx_l2_err", l2_err),
> + HNS3_TQP_STAT("rx_l3l4_csum_err", l3l4_csum_err),
> +};
> +
> +#define HNS3_RXQ_STATS_COUNT ARRAY_SIZE(hns3_rxq_stats)
> +
> +#define HNS3_TQP_STATS_COUNT (HNS3_TXQ_STATS_COUNT + HNS3_RXQ_STATS_COUNT)
> +
> +struct hns3_link_mode_mapping {
> + u32 hns3_link_mode;
> + u32 ethtool_link_mode;
> +};
> +
> +static const struct hns3_link_mode_mapping hns3_lm_map[] = {
> + {HNS3_LM_FIBRE_BIT, ETHTOOL_LINK_MODE_FIBRE_BIT},
> + {HNS3_LM_AUTONEG_BIT, ETHTOOL_LINK_MODE_Autoneg_BIT},
> + {HNS3_LM_TP_BIT, ETHTOOL_LINK_MODE_TP_BIT},
> + {HNS3_LM_PAUSE_BIT, ETHTOOL_LINK_MODE_Pause_BIT},
> + {HNS3_LM_BACKPLANE_BIT, ETHTOOL_LINK_MODE_Backplane_BIT},
> + {HNS3_LM_10BASET_HALF_BIT, ETHTOOL_LINK_MODE_10baseT_Half_BIT},
> + {HNS3_LM_10BASET_FULL_BIT, ETHTOOL_LINK_MODE_10baseT_Full_BIT},
> + {HNS3_LM_100BASET_HALF_BIT, ETHTOOL_LINK_MODE_100baseT_Half_BIT},
> + {HNS3_LM_100BASET_FULL_BIT, ETHTOOL_LINK_MODE_100baseT_Full_BIT},
> + {HNS3_LM_1000BASET_FULL_BIT, ETHTOOL_LINK_MODE_1000baseT_Full_BIT},
> +};
> +
> +#define HNS3_DRV_TO_ETHTOOL_CAPS(caps, lk_ksettings, name) \
> +{ \
> + int i; \
> + \
> + for (i = 0; i < ARRAY_SIZE(hns3_lm_map); i++) { \
> + if ((caps) & hns3_lm_map[i].hns3_link_mode) \
> + __set_bit(hns3_lm_map[i].ethtool_link_mode,\
> + (lk_ksettings)->link_modes.name); \
> + } \
> +}

How about making this an inline function such that you would get proper
type checking?

> +
> +static int hns3_get_sset_count(struct net_device *netdev, int stringset)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(netdev);
> + struct hnae3_handle *h = priv->ae_handle;
> + const struct hnae3_ae_ops *ops = h->ae_algo->ops;
> +
> + if (!ops->get_sset_count) {
> + netdev_err(netdev, "could not get string set count\n");
> + return -EOPNOTSUPP;
> + }

Is it really worth the error message? This might be called several times
during the driver's lifecycle.

> +
> + switch (stringset) {
> + case ETH_SS_STATS:
> + return (HNS3_NETDEV_STATS_COUNT +
> + (HNS3_TQP_STATS_COUNT * h->kinfo.num_tqps) +
> + ops->get_sset_count(h, stringset));
> +
> + case ETH_SS_TEST:
> + return ops->get_sset_count(h, stringset);
> + }
> +
> + return 0;
> +}
> +
> +static u8 *hns3_get_strings_netdev(u8 *data)
> +{
> + int i;

unsigned int i.

> +
> + for (i = 0; i < HNS3_NETDEV_STATS_COUNT; i++) {
> + memcpy(data, hns3_netdev_stats[i].stats_string,
> + ETH_GSTRING_LEN);
> + data += ETH_GSTRING_LEN;
> + }
> +
> + return data;
> +}
> +
> +static u8 *hns3_get_strings_tqps(struct hnae3_handle *handle, u8 *data)
> +{
> + struct hnae3_knic_private_info *kinfo = &handle->kinfo;
> + int i, j;

Likewise, unsigned int i, j.

> +
> + /* get strings for Tx */
> + for (i = 0; i < kinfo->num_tqps; i++) {
> + for (j = 0; j < HNS3_TXQ_STATS_COUNT; j++) {
> + u8 gstr[ETH_GSTRING_LEN];

char gstr[ETH_GSTRING_LEN] and you can move it out of the loop since it
is just a temporary buffer for both loops here.

> +
> + sprintf(gstr, "rcb_q%d_", i);

snprintf() just to be on the safe side.

> + strcat(gstr, hns3_txq_stats[j].stats_string);
> +
> + memcpy(data, gstr, ETH_GSTRING_LEN);

What ensures that you are NULL terminating this string?

> + data += ETH_GSTRING_LEN;
> + }
> + }
> +
> + /* get strings for Rx */
> + for (i = 0; i < kinfo->num_tqps; i++) {
> + for (j = 0; j < HNS3_RXQ_STATS_COUNT; j++) {
> + u8 gstr[ETH_GSTRING_LEN];
> +
> + sprintf(gstr, "rcb_q%d_", i);
> + strcat(gstr, hns3_rxq_stats[j].stats_string);
> +
> + memcpy(data, gstr, ETH_GSTRING_LEN);
> + data += ETH_GSTRING_LEN;
> + }
> + }
> +
> + return data;
> +}
> +
> +static void hns3_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(netdev);
> + struct hnae3_handle *h = priv->ae_handle;
> + const struct hnae3_ae_ops *ops = h->ae_algo->ops;
> + char *buff = (char *)data;
> +
> + if (!ops->get_strings) {
> + netdev_err(netdev, "could not get strings!\n");
> + return;
> + }

Same here, does not sound like something you would want to print more
than once?

> +
> + switch (stringset) {
> + case ETH_SS_STATS:
> + buff = hns3_get_strings_netdev(buff);
> + buff = hns3_get_strings_tqps(h, buff);
> + h->ae_algo->ops->get_strings(h, stringset, (u8 *)buff);
> + break;
> + case ETH_SS_TEST:
> + ops->get_strings(h, stringset, data);
> + break;
> + }
> +}
> +
> +static u64 *hns3_get_stats_netdev(struct net_device *netdev, u64 *data)
> +{

You should implement the 64-bit version of this.

> + const struct rtnl_link_stats64 *net_stats;
> + struct rtnl_link_stats64 temp;
> + u8 *stat;
> + int i;

unsigned int i

> +
> + net_stats = dev_get_stats(netdev, &temp);
> +
> + for (i = 0; i < HNS3_NETDEV_STATS_COUNT; i++) {
> + stat = (u8 *)net_stats + hns3_netdev_stats[i].stats_offset;
> + *data++ = *(u64 *)stat;
> + }
> +
> + return data;
> +}
> +
> +static u64 *hns3_get_stats_tqps(struct hnae3_handle *handle, u64 *data)
> +{
> + struct hns3_nic_priv *nic_priv = (struct hns3_nic_priv *)handle->priv;
> + struct hnae3_knic_private_info *kinfo = &handle->kinfo;
> + struct hns3_enet_ring *ring;
> + u8 *stat;
> + int i;

Same here.

> +
> + /* get stats for Tx */
> + for (i = 0; i < kinfo->num_tqps; i++) {
> + ring = nic_priv->ring_data[i].ring;
> + for (i = 0; i < HNS3_TXQ_STATS_COUNT; i++) {
> + stat = (u8 *)ring + hns3_txq_stats[i].stats_offset;
> + *data++ = *(u64 *)stat;
> + }
> + }
> +
> + /* get stats for Rx */
> + for (i = 0; i < kinfo->num_tqps; i++) {
> + ring = nic_priv->ring_data[i + kinfo->num_tqps].ring;
> + for (i = 0; i < HNS3_RXQ_STATS_COUNT; i++) {
> + stat = (u8 *)ring + hns3_rxq_stats[i].stats_offset;
> + *data++ = *(u64 *)stat;
> + }
> + }
> +
> + return data;
> +}
> +
> +/* hns3_get_stats - get detail statistics.
> + * @netdev: net device
> + * @stats: statistics info.
> + * @data: statistics data.
> + */
> +void hns3_get_stats(struct net_device *netdev, struct ethtool_stats *stats,
> + u64 *data)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(netdev);
> + struct hnae3_handle *h = priv->ae_handle;
> + u64 *p = data;
> +
> + if (!h->ae_algo->ops->get_stats || !h->ae_algo->ops->update_stats) {
> + netdev_err(netdev, "could not get any statistics\n");
> + return;
> + }
> +
> + h->ae_algo->ops->update_stats(h, &netdev->stats);
> +
> + /* get netdev related stats */
> + p = hns3_get_stats_netdev(netdev, p);
> +
> + /* get per-queue stats */
> + p = hns3_get_stats_tqps(h, p);
> +
> + /* get MAC & other misc hardware stats */
> + h->ae_algo->ops->get_stats(h, p);
> +}
> +
> +static void hns3_get_drvinfo(struct net_device *netdev,
> + struct ethtool_drvinfo *drvinfo)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(netdev);
> + struct hnae3_handle *h = priv->ae_handle;
> +
> + strncpy(drvinfo->version, HNAE_DRIVER_VERSION,
> + sizeof(drvinfo->version));
> + drvinfo->version[sizeof(drvinfo->version) - 1] = '\0';

strlcpy() would probably do that for you.

> +
> + strncpy(drvinfo->driver, HNAE_DRIVER_NAME, sizeof(drvinfo->driver));
> + drvinfo->driver[sizeof(drvinfo->driver) - 1] = '\0';

Same here

> +
> + strncpy(drvinfo->bus_info, priv->dev->bus->name,
> + sizeof(drvinfo->bus_info));> + drvinfo->bus_info[ETHTOOL_BUSINFO_LEN - 1] = '\0';

And here.

The rest looked fine.
--
Florian