Re: [PATCH] [v2] net: qcom/emac: add ethtool support

[Timur Tabi]

Timur Tabi wrote:

+static void emac_get_pauseparam(struct net_device *netdev,
+   struct ethtool_pauseparam *pause)
+{
+   struct phy_device *phydev = netdev->phydev;
+
+   if (phydev) {
+   if (phydev->autoneg)
+   pause->autoneg = 1;
+   if (phydev->pause)
+   pause->rx_pause = 1;
+   if (phydev->pause != phydev->asym_pause)
+   pause->tx_pause = 1;
+   }
+}


I finally figured out why this code was bothering me.

This function works, as long as I do NOT implement set_pauseparam. 
That's because the driver always matches the pause frame support in the 
MAC with whatever the PHY is doing.  Since the MAC and PHY are always in 
sync, I can use the PHY settings for get_pauseparam.


However, technically this is not correct.  get_pauseparam is supposed to 
return the setting of the MAC, not the PHY.  If I also implement 
set_pauseparam, which can force the MAC to ignore the PHY and 
enable/disable pause frames arbitrarily, then the above function is wrong.


Do I finally understand this correctly?

--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH] [v2] net: qcom/emac: add ethtool support

[David Miller]

From: Timur Tabi 
Date: Fri,  6 Jan 2017 15:43:01 -0600

> Add support for some ethtool methods: get/set link settings, get/set
> message level, get statistics, get link status, get ring params, get
> pause params, and restart autonegotiation.
> 
> The code to collect the hardware statistics is moved into its own
> function so that it can be used by "get statistics" method.
> 
> Signed-off-by: Timur Tabi 
> ---
> 
> Notes:
> I don't trust my implementation of emac_get_pauseparam.  I feel like
> I'm missing something.
> 
> v2: added emac_get_pauseparam and emac_get_ringparam

This doesn't apply cleanly to net-next, please respin.

[PATCH] [v2] net: qcom/emac: add ethtool support

[Timur Tabi]

Add support for some ethtool methods: get/set link settings, get/set
message level, get statistics, get link status, get ring params, get
pause params, and restart autonegotiation.

The code to collect the hardware statistics is moved into its own
function so that it can be used by "get statistics" method.

Signed-off-by: Timur Tabi 
---

Notes:
I don't trust my implementation of emac_get_pauseparam.  I feel like
I'm missing something.

v2: added emac_get_pauseparam and emac_get_ringparam

 drivers/net/ethernet/qualcomm/emac/Makefile   |   2 +-
 drivers/net/ethernet/qualcomm/emac/emac-ethtool.c | 185 ++
 drivers/net/ethernet/qualcomm/emac/emac.c |  51 +++---
 drivers/net/ethernet/qualcomm/emac/emac.h |   3 +
 4 files changed, 220 insertions(+), 21 deletions(-)
 create mode 100644 drivers/net/ethernet/qualcomm/emac/emac-ethtool.c

diff --git a/drivers/net/ethernet/qualcomm/emac/Makefile 
b/drivers/net/ethernet/qualcomm/emac/Makefile
index 7a66879..fc57ced 100644
--- a/drivers/net/ethernet/qualcomm/emac/Makefile
+++ b/drivers/net/ethernet/qualcomm/emac/Makefile
@@ -4,6 +4,6 @@
 
 obj-$(CONFIG_QCOM_EMAC) += qcom-emac.o
 
-qcom-emac-objs := emac.o emac-mac.o emac-phy.o emac-sgmii.o \
+qcom-emac-objs := emac.o emac-mac.o emac-phy.o emac-sgmii.o emac-ethtool.o \
  emac-sgmii-fsm9900.o emac-sgmii-qdf2432.o \
  emac-sgmii-qdf2400.o
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c 
b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
new file mode 100644
index 000..cfc57d2
--- /dev/null
+++ b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
@@ -0,0 +1,185 @@
+/* Copyright (c) 2016, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+
+#include "emac.h"
+
+static const char * const emac_ethtool_stat_strings[] = {
+   "rx_ok",
+   "rx_bcast",
+   "rx_mcast",
+   "rx_pause",
+   "rx_ctrl",
+   "rx_fcs_err",
+   "rx_len_err",
+   "rx_byte_cnt",
+   "rx_runt",
+   "rx_frag",
+   "rx_sz_64",
+   "rx_sz_65_127",
+   "rx_sz_128_255",
+   "rx_sz_256_511",
+   "rx_sz_512_1023",
+   "rx_sz_1024_1518",
+   "rx_sz_1519_max",
+   "rx_sz_ov",
+   "rx_rxf_ov",
+   "rx_align_err",
+   "rx_bcast_byte_cnt",
+   "rx_mcast_byte_cnt",
+   "rx_err_addr",
+   "rx_crc_align",
+   "rx_jabbers",
+   "tx_ok",
+   "tx_bcast",
+   "tx_mcast",
+   "tx_pause",
+   "tx_exc_defer",
+   "tx_ctrl",
+   "tx_defer",
+   "tx_byte_cnt",
+   "tx_sz_64",
+   "tx_sz_65_127",
+   "tx_sz_128_255",
+   "tx_sz_256_511",
+   "tx_sz_512_1023",
+   "tx_sz_1024_1518",
+   "tx_sz_1519_max",
+   "tx_1_col",
+   "tx_2_col",
+   "tx_late_col",
+   "tx_abort_col",
+   "tx_underrun",
+   "tx_rd_eop",
+   "tx_len_err",
+   "tx_trunc",
+   "tx_bcast_byte",
+   "tx_mcast_byte",
+   "tx_col",
+};
+
+#define EMAC_STATS_LEN ARRAY_SIZE(emac_ethtool_stat_strings)
+
+static u32 emac_get_msglevel(struct net_device *netdev)
+{
+   struct emac_adapter *adpt = netdev_priv(netdev);
+
+   return adpt->msg_enable;
+}
+
+static void emac_set_msglevel(struct net_device *netdev, u32 data)
+{
+   struct emac_adapter *adpt = netdev_priv(netdev);
+
+   adpt->msg_enable = data;
+}
+
+static int emac_get_sset_count(struct net_device *netdev, int sset)
+{
+   switch (sset) {
+   case ETH_SS_STATS:
+   return EMAC_STATS_LEN;
+   default:
+   return -EOPNOTSUPP;
+   }
+}
+
+static void emac_get_strings(struct net_device *netdev, u32 stringset, u8 
*data)
+{
+   unsigned int i;
+
+   switch (stringset) {
+   case ETH_SS_STATS:
+   for (i = 0; i < EMAC_STATS_LEN; i++) {
+   strlcpy(data, emac_ethtool_stat_strings[i],
+   ETH_GSTRING_LEN);
+   data += ETH_GSTRING_LEN;
+   }
+   break;
+   }
+}
+
+static void emac_get_ethtool_stats(struct net_device *netdev,
+  struct ethtool_stats *stats,
+  u64 *data)
+{
+   struct emac_adapter *adpt = netdev_priv(netdev);
+
+   spin_lock(>stats.lock);
+
+   emac_update_hw_stats(adpt);
+   memcpy(data, >stats, EMAC_STATS_LEN * sizeof(u64));
+
+   spin_unlock(>stats.lock);
+}
+
+static int