Re: [PATCH net-next v2 13/21] motorcomm:yt6801: Implement some ethtool_ops function

+static const struct fxgmac_stats_desc fxgmac_gstring_stats[] = {
+	/* MMC TX counters */
+	FXGMAC_STAT("tx_bytes", txoctetcount_gb),
+	FXGMAC_STAT("tx_bytes_good", txoctetcount_g),
+	FXGMAC_STAT("tx_packets", txframecount_gb),
+	FXGMAC_STAT("tx_packets_good", txframecount_g),
+	FXGMAC_STAT("tx_unicast_packets", txunicastframes_gb),
+	FXGMAC_STAT("tx_broadcast_packets", txbroadcastframes_gb),
+	FXGMAC_STAT("tx_broadcast_packets_good", txbroadcastframes_g),
+	FXGMAC_STAT("tx_multicast_packets", txmulticastframes_gb),
+	FXGMAC_STAT("tx_multicast_packets_good", txmulticastframes_g),
+	FXGMAC_STAT("tx_vlan_packets_good", txvlanframes_g),
+	FXGMAC_STAT("tx_64_byte_packets", tx64octets_gb),
+	FXGMAC_STAT("tx_65_to_127_byte_packets", tx65to127octets_gb),
+	FXGMAC_STAT("tx_128_to_255_byte_packets", tx128to255octets_gb),
+	FXGMAC_STAT("tx_256_to_511_byte_packets", tx256to511octets_gb),
+	FXGMAC_STAT("tx_512_to_1023_byte_packets", tx512to1023octets_gb),
+	FXGMAC_STAT("tx_1024_to_max_byte_packets", tx1024tomaxoctets_gb),
+	FXGMAC_STAT("tx_underflow_errors", txunderflowerror),
+	FXGMAC_STAT("tx_pause_frames", txpauseframes),
+	FXGMAC_STAT("tx_single_collision", txsinglecollision_g),
+	FXGMAC_STAT("tx_multiple_collision", txmultiplecollision_g),
+	FXGMAC_STAT("tx_deferred_frames", txdeferredframes),
+	FXGMAC_STAT("tx_late_collision_frames", txlatecollisionframes),
+	FXGMAC_STAT("tx_excessive_collision_frames",
+		    txexcessivecollisionframes),
+	FXGMAC_STAT("tx_carrier_error_frames", txcarriererrorframes),
+	FXGMAC_STAT("tx_excessive_deferral_error", txexcessivedeferralerror),
+	FXGMAC_STAT("tx_oversize_frames_good", txoversize_g),

Please look at the rmon group.

+static void fxgmac_ethtool_get_drvinfo(struct net_device *netdev,
+				       struct ethtool_drvinfo *drvinfo)
+{
+	struct fxgmac_pdata *pdata = netdev_priv(netdev);
+	u32 ver = pdata->hw_feat.version;
+	u32 sver, devid, userver;
+
+	strscpy(drvinfo->version, pdata->drv_ver, sizeof(drvinfo->version));

Please leave this blank, so the core fills it with the git hash of the
kernel.

+
+	/* S|SVER: MAC Version
+	 * D|DEVID: Indicates the Device family
+	 * U|USERVER: User-defined Version
+	 */
+	sver = FXGMAC_GET_BITS(ver, MAC_VR_SVER_POS, MAC_VR_SVER_LEN);
+	devid = FXGMAC_GET_BITS(ver, MAC_VR_DEVID_POS, MAC_VR_DEVID_LEN);
+	userver = FXGMAC_GET_BITS(ver, MAC_VR_USERVER_POS, MAC_VR_USERVER_LEN);
+
+	snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
+		 "S.D.U: %x.%x.%x", sver, devid, userver);

You might want to consider devlink, which gives you more flexibility
in reporting versions.

+static void fxgmac_ethtool_set_msglevel(struct net_device *netdev, u32 msglevel)
+{
+	struct fxgmac_pdata *pdata = netdev_priv(netdev);
+
+	yt_dbg(pdata, "set msglvl from %08x to %08x\n", pdata->msg_enable,
+	       msglevel);
+	pdata->msg_enable = msglevel;

This is an example of a yt_dbg() which seems pointless now that you
have debugged it. Maybe you did have a bug in the first version of
this one line function, but it looks reasonably bug free now, so i
don't see the need for the print.

+static void fxgmac_get_regs(struct net_device *netdev,
+			    struct ethtool_regs *regs, void *p)
+{
+	struct fxgmac_pdata *pdata = netdev_priv(netdev);
+	u32 *regs_buff = p;
+
+	memset(p, 0, FXGMAC_PHY_REG_CNT * sizeof(u32));
+	for (u32 i = MII_BMCR; i < FXGMAC_PHY_REG_CNT; i++)
+		regs_buff[i] = phy_read(pdata->phydev, i);

A MAC driver should not be touching the PHY. There are other
standardised ways of dumping PHY registers, which don't involve the
MAC driver.

+static void fxgmac_get_pauseparam(struct net_device *dev,
+				  struct ethtool_pauseparam *data)
+{
+	struct fxgmac_pdata *yt = netdev_priv(dev);

Please be consistent with your naming. The function above calls this
pdata. I don't particularly like pdata, because that often means
platform_data, which you don't have. priv is more common.

+	bool tx_pause, rx_pause;
+
+	phy_get_pause(yt->phydev, &tx_pause, &rx_pause);
+
+	data->autoneg = yt->phydev->autoneg;

This is wrong. You can be doing autoneg in general,
i.e. phydev->autoneg, but not using autoneg for pause. You should
remember the value set in set_pauseparam().

+	data->tx_pause = tx_pause ? 1 : 0;
+	data->rx_pause = rx_pause ? 1 : 0;

Why the ? 1: 0 ?

+
+	yt_dbg(yt, "%s, rx=%d, tx=%d\n", __func__, data->rx_pause,
+	       data->tx_pause);
+}
+
+static int fxgmac_set_pauseparam(struct net_device *dev,
+				 struct ethtool_pauseparam *data)
+{
+	struct fxgmac_pdata *yt = netdev_priv(dev);
+
+	if (dev->mtu > ETH_DATA_LEN)
+		return -EOPNOTSUPP;

A comment about why jumbo packets breaks pause would be good. I also
assume your set_mtu code disables pause when jumbo is enabled?
get_pauseparam should also make it clear pause is disabled.

+	phy_set_asym_pause(yt->phydev, data->rx_pause, data->tx_pause);

You are ignoring ethtool_pauseparam autoneg.

+static void fxgmac_ethtool_get_strings(struct net_device *netdev, u32 stringset,
+				       u8 *data)
+{
+	switch (stringset) {
+	case ETH_SS_STATS:
+		for (u32 i = 0; i < FXGMAC_STATS_COUNT; i++) {
+			memcpy(data, fxgmac_gstring_stats[i].stat_string,
+			       strlen(fxgmac_gstring_stats[i].stat_string));
+			data += ETH_GSTRING_LEN;
+		}
+		break;
+	default:
+		WARN_ON(1);

You need to be careful with WARN_ON() particularly if it can be
triggered from user space. Maybe build cause a WARN to result in a
reboot.

+static int fxgmac_ethtool_reset(struct net_device *netdev, u32 *flag)
+{
+	struct fxgmac_pdata *pdata = netdev_priv(netdev);
+	int ret = 0;
+	u32 val;
+
+	val = *flag & (ETH_RESET_ALL | ETH_RESET_PHY);
+	if (!val) {
+		yt_err(pdata, "Operation not support.\n");
+		return -EOPNOTSUPP;
+	}
+
+	switch (*flag) {
+	case ETH_RESET_ALL:
+		fxgmac_restart(pdata);
+		*flag = 0;
+		break;
+	case ETH_RESET_PHY:
+		/* Power off and on the phy in order to properly
+		 * configure the MAC timing
+		 */
+		ret = phy_modify(pdata->phydev, MII_BMCR, BMCR_PDOWN,
+				 BMCR_PDOWN);
+		if (ret < 0)
+			return ret;
+		fsleep(9000);
+
+		ret = phy_modify(pdata->phydev, MII_BMCR, BMCR_PDOWN, 0);
+		if (ret < 0)
+			return ret;
+		*flag = 0;

This is a bad idea, since depending on the PHY, you have thrown away
all the configuration, and the PHY is now probably dead until you
admin down/admin up the interface. In general a MAC driver should
never directly touch the PHY, it should just use the API phylib
exposes.

    Andrew

---
pw-bot: cr