PATCH: ethtool MII helpers

PATCH: ethtool MII helpers

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 476 bytes --]

Initial draft of a helper which uses generic elements present in several
net drivers to implement ethtool ioctl support in a minimum amount of
code.

I have included a sample implementation in the epic100 driver, to
illustrate how these helpers may be used.  This should make it easier to
implement support across 10/100 hardware which uses primarily an MII
phy.

Comments appreciated.

-- 
Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |

[-- Attachment #2: mii.patch --]
[-- Type: text/plain, Size: 12420 bytes --]

Index: linux_2_4/include/linux/ethtool.h
diff -u linux_2_4/include/linux/ethtool.h:1.1.1.4 linux_2_4/include/linux/ethtool.h:1.1.1.4.84.1
--- linux_2_4/include/linux/ethtool.h:1.1.1.4	Thu Apr 19 17:55:36 2001
+++ linux_2_4/include/linux/ethtool.h	Fri Jun  8 21:16:58 2001
@@ -1,4 +1,4 @@
-/* $Id: ethtool.h,v 1.1.1.4 2001/04/20 00:55:36 jgarzik Exp $
+/* $Id: ethtool.h,v 1.1.1.4.84.1 2001/06/09 04:16:58 jgarzik Exp $
  * ethtool.h: Defines for Linux ethtool.
  *
  * Copyright (C) 1998 David S. Miller (davem@redhat.com)
@@ -34,13 +34,15 @@
 	char	bus_info[32];	/* Bus info for this interface.  For PCI
 				 * devices, use pci_dev->slot_name. */
 	char	reserved1[32];
-	char	reserved2[32];
+	char	reserved2[28];
+	u32	regdump_len;	/* Amount of data from ETHTOOL_GREGS */
 };
 
 /* CMDs currently supported */
 #define ETHTOOL_GSET		0x00000001 /* Get settings. */
 #define ETHTOOL_SSET		0x00000002 /* Set settings, privileged. */
 #define ETHTOOL_GDRVINFO	0x00000003 /* Get driver info. */
+#define ETHTOOL_GREGS		0x00000004 /* Get NIC registers, privileged. */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
Index: linux_2_4/include/linux/mii.h
diff -u linux_2_4/include/linux/mii.h:1.1.1.1 linux_2_4/include/linux/mii.h:1.1.1.1.52.1
--- linux_2_4/include/linux/mii.h:1.1.1.1	Fri May 11 16:54:44 2001
+++ linux_2_4/include/linux/mii.h	Sun Jun 10 10:26:44 2001
@@ -126,6 +126,33 @@
 #define CSCONFIG_RESV4          0x4000  /* Unused...                   */
 #define CSCONFIG_NDISABLE       0x8000  /* Disable NRZI                */
 
+
+
+struct ethtool_mii_info {
+	struct net_device *dev;	/* our net interface			*/
+	void *useraddr;		/* userspace addr to which we put data	*/
+	
+	int phy_id;		/* PHY we are addressing		*/
+
+	int bmcr;		/* cached MII register values.		*/
+	int bmsr;		/* -1 means 'undefined', which usually	*/
+	int advertising;	/* means the implementation should read	*/
+	int lpa;		/* the values from hardware instead.	*/
+
+	int autoneg;		/* 0 (disabled), 1 (enabled), -1 (ask hw) */
+	unsigned int ignore;	/* mask of medias we never support,	*/
+				/* such as 100baseT4			*/
+	int speed;		/* 10, 100, 1000 or -1 (ask hw)		*/
+	int full_duplex;	/* 0 (no), 1 (yes), -1 (ask hw)		*/
+	unsigned int port;	/* PORT_xxx from linux/ethtool.h	*/
+	
+	int (*mdio_read) (struct net_device *dev, int phy_id, int location);
+	void (*mdio_write) (struct net_device *dev, int phy_id, int location, int val);
+};
+
+int mii_ethtool_gset (struct ethtool_mii_info *mii);
+int mii_ethtool_sset (struct ethtool_mii_info *mii);
+
 /**
  * mii_nway_result
  * @negotiated: value of MII ANAR and'd with ANLPAR
Index: linux_2_4/drivers/net/mii.c
diff -u /dev/null linux_2_4/drivers/net/mii.c:1.1.2.1
--- /dev/null	Sun Jun 10 10:28:01 2001
+++ linux_2_4/drivers/net/mii.c	Sun Jun 10 10:26:44 2001
@@ -0,0 +1,212 @@
+/*
+ *	linux/drivers/net/mii.c
+ *	Copyright 2001 Jeff Garzik <jgarzik@mandrakesoft.com>
+ *
+ *	This software may be used and distributed according to the terms
+ *	of the GNU General Public License, incorporated herein by reference.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/fs.h>
+#include <linux/mii.h>
+#include <linux/ethtool.h>
+#include <asm/uaccess.h>
+
+
+static void mii_fill_ethtool_cmd (struct net_device *dev,
+				  struct ethtool_mii_info *mii,
+				  struct ethtool_cmd *ecmd)
+{
+	unsigned int bmsr, bmcr, v, autoneg, advertising, lpa;
+	unsigned int negotiated, full_duplex, speed;
+
+	memset(ecmd, 0, sizeof(*ecmd));
+	
+	ecmd->cmd = ETHTOOL_GSET;
+
+	if (mii->bmcr < 0)
+		bmcr = mii->bmcr = mii->mdio_read(dev, mii->phy_id, MII_BMCR);
+	else	bmcr = mii->bmcr;
+
+	if (mii->bmsr < 0)
+		bmsr = mii->bmsr = mii->mdio_read(dev, mii->phy_id, MII_BMSR);
+	else	bmsr = mii->bmsr;
+
+	if (mii->advertising < 0)
+		advertising = mii->advertising =
+			mii->mdio_read(dev, mii->phy_id, MII_ADVERTISE);
+	else	advertising = mii->advertising;
+
+	if (mii->lpa < 0)
+		lpa = mii->lpa = mii->mdio_read(dev, mii->phy_id, MII_LPA);
+	else	lpa = mii->lpa;
+
+	negotiated = advertising & lpa;
+
+	if (mii->autoneg < 0)
+		autoneg = mii->autoneg = (bmcr & BMCR_ANENABLE) ? 1 : 0;
+	else	autoneg = mii->autoneg;
+		
+	if (mii->full_duplex < 0)
+		full_duplex = mii->full_duplex =
+			mii_nway_result(negotiated) & LPA_DUPLEX;
+	else	full_duplex = mii->full_duplex;
+		
+	if (mii->speed < 0) {
+		if (negotiated & LPA_100)
+			speed = mii->speed = 100;
+		else
+			speed = mii->speed = 10;
+	} else
+		speed = mii->speed;
+	
+	ecmd->supported = SUPPORTED_MII;
+	v = bmsr & ~mii->ignore;
+	if (v & BMSR_10HALF)
+		ecmd->supported |= SUPPORTED_10baseT_Half;
+	if (v & BMSR_10FULL)
+		ecmd->supported |= SUPPORTED_10baseT_Full;
+	if (v & BMSR_100HALF)
+		ecmd->supported |= SUPPORTED_100baseT_Half;
+	if (v & BMSR_100FULL)
+		ecmd->supported |= SUPPORTED_100baseT_Full;
+	if (bmsr & BMSR_ANEGCAPABLE)
+		ecmd->supported |= SUPPORTED_Autoneg;
+	else
+		autoneg = mii->autoneg = 0;
+
+	ecmd->advertising = ADVERTISED_MII;
+	v = advertising & ~mii->ignore;
+	if (v & BMSR_10HALF)
+		ecmd->advertising |= ADVERTISED_10baseT_Half;
+	if (v & BMSR_10FULL)
+		ecmd->advertising |= ADVERTISED_10baseT_Full;
+	if (v & BMSR_100HALF)
+		ecmd->advertising |= ADVERTISED_100baseT_Half;
+	if (v & BMSR_100FULL)
+		ecmd->advertising |= ADVERTISED_100baseT_Full;
+	if (autoneg) {
+		ecmd->advertising |= ADVERTISED_Autoneg;
+		ecmd->autoneg = AUTONEG_ENABLE;
+	} else
+		ecmd->autoneg = AUTONEG_DISABLE;
+
+	ecmd->speed = speed == 100 ? SPEED_100 : SPEED_10;
+	ecmd->duplex = full_duplex ? DUPLEX_FULL : DUPLEX_HALF;
+	ecmd->port = PORT_MII;
+	ecmd->phy_address = mii->phy_id;
+	ecmd->transceiver = XCVR_INTERNAL;
+}
+
+int mii_ethtool_gset (struct ethtool_mii_info *mii)
+{
+	struct ethtool_cmd ecmd;
+	
+	if (mii->port != PORT_MII)
+		return -EOPNOTSUPP;
+
+	mii_fill_ethtool_cmd(mii->dev, mii, &ecmd);
+
+	if (copy_to_user(mii->useraddr, &ecmd, sizeof(ecmd)))
+		return -EFAULT;
+
+	return 0;
+}
+
+int mii_ethtool_sset (struct ethtool_mii_info *mii)
+{
+	struct net_device *dev = mii->dev;
+	struct ethtool_cmd in, out;
+	unsigned int advert, bmcr;
+
+	if (copy_from_user (&in, mii->useraddr, sizeof (in)))
+		return -EFAULT;
+	mii_fill_ethtool_cmd (dev, mii, &out);
+	
+	if (in.port != out.port) {
+		if (copy_to_user(mii->useraddr, &in, sizeof(in)))
+			return -EFAULT;
+		mii->port = in.port;
+		return 0;
+	}
+	
+	/* we don't support changing phy address, tranceiver,
+	 * or the interrupt mitigation stuff.
+	 */
+	if ((in.phy_address != out.phy_address) ||
+	    (in.transceiver != XCVR_INTERNAL) ||
+	    (in.maxtxpkt != out.maxtxpkt) ||
+	    (in.maxrxpkt != out.maxrxpkt))
+		return -EOPNOTSUPP;
+
+	advert = mii->advertising & ~ADVERTISE_ALL;
+
+	/* NWAY autonegotiation enabled */
+	if (in.autoneg == AUTONEG_ENABLE) {
+		bmcr = mii->bmcr | BMCR_ANENABLE;
+
+		if (in.advertising & ADVERTISED_10baseT_Half)
+			advert |= ADVERTISE_10HALF;
+		if (in.advertising & ADVERTISED_10baseT_Full)
+			advert |= ADVERTISE_10FULL;
+		if (in.advertising & ADVERTISED_100baseT_Half)
+			advert |= ADVERTISE_100HALF;
+		if (in.advertising & ADVERTISED_100baseT_Full)
+			advert |= ADVERTISE_100FULL;
+		if (advert == (mii->advertising & ~ADVERTISE_ALL))
+			return -EINVAL;
+	}
+
+	/* NWAY autonegotiation disabled */
+	else {
+		bmcr = mii->bmcr & ~BMCR_ANENABLE;
+		
+		if (in.speed == SPEED_100)
+			bmcr |= BMCR_SPEED100;
+		else	bmcr &= ~BMCR_SPEED100;
+
+		if (in.duplex == DUPLEX_FULL)
+			bmcr |= BMCR_FULLDPLX;
+		else	bmcr &= ~BMCR_FULLDPLX;
+
+		if (mii->bmsr & BMSR_10HALF)
+			advert |= ADVERTISE_10HALF;
+		if (mii->bmsr & BMSR_10FULL)
+			advert |= ADVERTISE_10FULL;
+		if (mii->bmsr & BMSR_100HALF)
+			advert |= ADVERTISE_100HALF;
+		if (mii->bmsr & BMSR_100FULL)
+			advert |= ADVERTISE_100FULL;
+	}
+
+	if (advert != mii->advertising) {
+		bmcr |= BMCR_ANRESTART;
+		mii->mdio_write(dev, mii->phy_id, MII_ADVERTISE, advert);
+		mii->advertising = advert;
+	}
+
+	/* some phys need autoneg dis/enabled separately from other settings */
+	if ((bmcr & BMCR_ANENABLE) && (!(mii->bmcr & BMCR_ANENABLE))) {
+		mii->mdio_write(dev, mii->phy_id, MII_BMCR,
+				mii->bmcr | BMCR_ANENABLE | BMCR_ANRESTART);
+		bmcr &= ~BMCR_ANRESTART;
+	} else if ((!(bmcr & BMCR_ANENABLE)) && (mii->bmcr & BMCR_ANENABLE)) {
+		mii->mdio_write(dev, mii->phy_id, MII_BMCR,
+				mii->bmcr & ~BMCR_ANENABLE);
+	}
+
+	if (bmcr != mii->bmcr) {
+		mii->mdio_write(dev, mii->phy_id, MII_BMCR, bmcr);
+		bmcr &= ~BMCR_ANRESTART;
+		mii->bmcr = bmcr;
+	}
+	
+	if (copy_to_user(mii->useraddr, &out, sizeof(out)))
+		return -EFAULT;
+
+	return 0;
+}
+
+EXPORT_SYMBOL(mii_ethtool_gset);
+EXPORT_SYMBOL(mii_ethtool_sset);
Index: linux_2_4/drivers/net/epic100.c
diff -u linux_2_4/drivers/net/epic100.c:1.1.1.35 linux_2_4/drivers/net/epic100.c:1.1.1.35.42.3
--- linux_2_4/drivers/net/epic100.c:1.1.1.35	Sat May 19 18:56:00 2001
+++ linux_2_4/drivers/net/epic100.c	Sun Jun 10 10:26:44 2001
@@ -45,13 +45,16 @@
 	* { fill me in }
 
 	LK1.1.8:
-	* ethtool support (jgarzik)
+	* ethtool driver info support (jgarzik)
 
+	LK1.1.9:
+	* ethtool media get/set support (jgarzik)
+
 */
 
 #define DRV_NAME	"epic100"
-#define DRV_VERSION	"1.11+LK1.1.8"
-#define DRV_RELDATE	"May 18, 2001"
+#define DRV_VERSION	"1.11+LK1.1.9"
+#define DRV_RELDATE	"June 10, 2001"
 
 
 /* The user-configurable values.
@@ -116,6 +119,7 @@
 #include <linux/init.h>
 #include <linux/spinlock.h>
 #include <linux/ethtool.h>
+#include <linux/mii.h>
 #include <asm/bitops.h>
 #include <asm/io.h>
 #include <asm/uaccess.h>
@@ -135,6 +139,11 @@
 MODULE_PARM(rx_copybreak, "i");
 MODULE_PARM(options, "1-" __MODULE_STRING(MAX_UNITS) "i");
 MODULE_PARM(full_duplex, "1-" __MODULE_STRING(MAX_UNITS) "i");
+MODULE_PARM_DESC(debug, "EPIC/100 debug level (0-5)");
+MODULE_PARM_DESC(max_interrupt_work, "EPIC/100 maximum events handled per interrupt");
+MODULE_PARM_DESC(options, "EPIC/100: Bits 0-3: media type, bit 4: full duplex");
+MODULE_PARM_DESC(rx_copybreak, "EPIC/100 copy breakpoint for copy-only-tiny-frames");
+MODULE_PARM_DESC(full_duplex, "EPIC/100 full duplex setting(s) (1)");
 
 /*
 				Theory of Operation
@@ -1169,7 +1178,7 @@
 			if (pkt_len > PKT_BUF_SZ - 4) {
 				printk(KERN_ERR "%s: Oversized Ethernet frame, status %x "
 					   "%d bytes.\n",
-					   dev->name, pkt_len, status);
+					   dev->name, status, pkt_len);
 				pkt_len = 1514;
 			}
 			/* Check if the packet is long enough to accept without copying
@@ -1344,27 +1353,72 @@
 	return;
 }
 
-static int netdev_ethtool_ioctl(struct net_device *dev, void *useraddr)
+static int netdev_ethtool_ioctl (struct net_device *dev, void *useraddr)
 {
 	struct epic_private *np = dev->priv;
 	u32 ethcmd;
-		
-	if (copy_from_user(&ethcmd, useraddr, sizeof(ethcmd)))
+
+	if (copy_from_user (&ethcmd, useraddr, sizeof (ethcmd)))
 		return -EFAULT;
+
+	switch (ethcmd) {
+	case ETHTOOL_GDRVINFO:
+		{
+			struct ethtool_drvinfo info = { ETHTOOL_GDRVINFO };
+			strcpy (info.driver, DRV_NAME);
+			strcpy (info.version, DRV_VERSION);
+			strcpy (info.bus_info, np->pci_dev->slot_name);
+			if (copy_to_user (useraddr, &info, sizeof (info)))
+				return -EFAULT;
+			return 0;
+		}
 
-        switch (ethcmd) {
-        case ETHTOOL_GDRVINFO: {
-		struct ethtool_drvinfo info = {ETHTOOL_GDRVINFO};
-		strcpy(info.driver, DRV_NAME);
-		strcpy(info.version, DRV_VERSION);
-		strcpy(info.bus_info, np->pci_dev->slot_name);
-		if (copy_to_user(useraddr, &info, sizeof(info)))
-			return -EFAULT;
-		return 0;
+	case ETHTOOL_GSET:
+	case ETHTOOL_SSET:
+		{
+			struct ethtool_mii_info info = {
+				dev:		dev,
+				useraddr:	useraddr,
+				phy_id:		np->phys[0],
+				bmcr:		-1,
+				bmsr:		-1,
+				lpa:		-1,
+				advertising:	np->advertising,
+				autoneg:	-1,
+				ignore:		ADVERTISE_100BASE4,
+				speed:		-1,
+				full_duplex:	np->full_duplex,
+				port:		PORT_MII,
+				mdio_read:	mdio_read,
+				mdio_write:	mdio_write,
+			};
+			int rc;
+			unsigned int changed = 0;
+
+			if (ethcmd == ETHTOOL_GSET)
+				rc = mii_ethtool_gset (&info);
+			else
+				rc = mii_ethtool_sset (&info);
+
+			if (np->advertising != info.advertising) {
+				np->advertising = info.advertising;
+				changed = 1;
+			}
+			if (np->full_duplex != info.full_duplex) {
+				np->full_duplex = info.full_duplex;
+				changed = 1;
+			}
+
+			if (changed)
+				check_media (dev);
+
+			return rc;
+		}
+
+	default:
+		break;
 	}
 
-        }
-	
 	return -EOPNOTSUPP;
 }
 

Re: PATCH: ethtool MII helpers

[Pekka Savola]

On Sun, 10 Jun 2001, Jeff Garzik wrote:
> Initial draft of a helper which uses generic elements present in several
> net drivers to implement ethtool ioctl support in a minimum amount of
> code.

In the patch there is:

@@ -135,6 +139,11 @@
 MODULE_PARM(rx_copybreak, "i");
 MODULE_PARM(options, "1-" __MODULE_STRING(MAX_UNITS) "i");
 MODULE_PARM(full_duplex, "1-" __MODULE_STRING(MAX_UNITS) "i");
+MODULE_PARM_DESC(debug, "EPIC/100 debug level (0-5)");
+MODULE_PARM_DESC(max_interrupt_work, "EPIC/100 maximum events handled per interrupt");
+MODULE_PARM_DESC(options, "EPIC/100: Bits 0-3: media type, bit 4: full duplex");
+MODULE_PARM_DESC(rx_copybreak, "EPIC/100 copy breakpoint for copy-only-tiny-frames");
+MODULE_PARM_DESC(full_duplex, "EPIC/100 full duplex setting(s) (1)");

I recall some discussion on a list (can't find it now) that driver
specific comment like "EPIC/100" here notification on all _DESC's would be
removed to a separate MODULE_ to make the comments more generic?

-- 
Pekka Savola                 "Tell me of difficulties surmounted,
Netcore Oy                   not those you stumble over and fall"
Systems. Networks. Security.  -- Robert Jordan: A Crown of Swords

Re: PATCH: ethtool MII helpers

[Keith Owens]

On Mon, 11 Jun 2001 08:59:10 +0300 (EEST), 
Pekka Savola <pekkas@netcore.fi> wrote:
>+MODULE_PARM_DESC(debug, "EPIC/100 debug level (0-5)");
>+MODULE_PARM_DESC(max_interrupt_work, "EPIC/100 maximum events handled per interrupt");
>I recall some discussion on a list (can't find it now) that driver
>specific comment like "EPIC/100" here notification on all _DESC's would be
>removed to a separate MODULE_ to make the comments more generic?

MODULE_DESCRIPTION("EPIC/100 some text") would be better.

Re: PATCH: ethtool MII helpers

[Bogdan Costescu]

On Sun, 10 Jun 2001, Jeff Garzik wrote:

> Comments appreciated.

Some general comments first, the others are spread through the code.

- I don't know what the long-term plan is about ethtool vs. MII ioctl's.
If you do plan to replace completely the MII ioctl's, there should be a
way to access _all_ MII registers provided by the PHY, even if you do this
in a restricted way (i.e. for CAP_NET_ADMIN only). There is also useful
info in other registers than the 4 you have in your implementation.
- You are proposing some caching for the MII registers. I suppose that you
would like to have this code also working with whatever caching will be
done for MII access that was recently discussed. Wouldn't this produce
double caching under some circumstances ?

+	int speed;		/* 10, 100, 1000 or -1 (ask hw)		*/

Please note that the comment specifies 1000, while the code in several
places assumes only 2 possibilities: 10 and 100.

+	if (mii->autoneg < 0)
+		autoneg = mii->autoneg = (bmcr & BMCR_ANENABLE) ? 1 : 0;
+	else	autoneg = mii->autoneg;

You don't read anything from the hardware at this point. Why do you want
caching ?

Not related: I know that this comes from David Miller's older work, but
wouldn't be possible to have a more uniform naming scheme ? You have
BMCR_ANENABLE, but you have BMSR_ANEGCAPABLE...

+	if (mii->full_duplex < 0)
+		full_duplex = mii->full_duplex =
+			mii_nway_result(negotiated) & LPA_DUPLEX;
+	else	full_duplex = mii->full_duplex;

If autoneg. is disabled, I don't think that you always get useful info in
'negotiated'. Applies to the next chunk, too.

+	if (mii->speed < 0) {
+		if (negotiated & LPA_100)
+			speed = mii->speed = 100;
+		else
+			speed = mii->speed = 10;
+	} else
+		speed = mii->speed;

That's one of the places where you don't have 1000...

+	ecmd->speed = speed == 100 ? SPEED_100 : SPEED_10;

... and that's the second.

+	ecmd->transceiver = XCVR_INTERNAL;

I didn't understand what XCVR_INTERNAL should mean as opposed to
XCVR_EXTERNAL or whatever. For example: some older 3Com cards use external
transceivers (not on the chip), while newer ones have NWAY capable MII
transceivers on the chip. So, you can have:
	1. chip + MII
	2. NWAY-chip
	3. NWAY-chip + MII
All MII accesses are done through the serial mdio_* protocol. How should
be this handled w.r.t. XCVR_* or is it completely orthogonal?

+	if ((in.phy_address != out.phy_address) ||
+	    (in.transceiver != XCVR_INTERNAL) ||
+	    (in.maxtxpkt != out.maxtxpkt) ||
+	    (in.maxrxpkt != out.maxrxpkt))
+		return -EOPNOTSUPP;

... and here too.

+	if (advert != mii->advertising) {
+		bmcr |= BMCR_ANRESTART;
+		mii->mdio_write(dev, mii->phy_id, MII_ADVERTISE, advert);
+		mii->advertising = advert;
+	}
+
+	/* some phys need autoneg dis/enabled separately from other settings */
+	if ((bmcr & BMCR_ANENABLE) && (!(mii->bmcr & BMCR_ANENABLE))) {
+		mii->mdio_write(dev, mii->phy_id, MII_BMCR,
+				mii->bmcr | BMCR_ANENABLE | BMCR_ANRESTART);
+		bmcr &= ~BMCR_ANRESTART;
+	} else if ((!(bmcr & BMCR_ANENABLE)) && (mii->bmcr & BMCR_ANENABLE)) {
+		mii->mdio_write(dev, mii->phy_id, MII_BMCR,
+				mii->bmcr & ~BMCR_ANENABLE);
+	}

This is nice, but I would like to able to restart autonegotiation even
without changing any of the advertised capabilities. If I missed this
possibility, please point me to it...

Nice work!

-- 
Bogdan Costescu

IWR - Interdisziplinaeres Zentrum fuer Wissenschaftliches Rechnen
Universitaet Heidelberg, INF 368, D-69120 Heidelberg, GERMANY
Telephone: +49 6221 54 8869, Telefax: +49 6221 54 8868
E-mail: Bogdan.Costescu@IWR.Uni-Heidelberg.De

Re: PATCH: ethtool MII helpers

[Jeff Garzik]

Bogdan Costescu wrote:
> On Sun, 10 Jun 2001, Jeff Garzik wrote:
> - I don't know what the long-term plan is about ethtool vs. MII ioctl's.
> If you do plan to replace completely the MII ioctl's, there should be a
> way to access _all_ MII registers provided by the PHY, even if you do this
> in a restricted way (i.e. for CAP_NET_ADMIN only). There is also useful
> info in other registers than the 4 you have in your implementation.

What are you doing that you need to access all registers from userspace?

On to your larger question, ethtool versus MII ioctls.   That is an
issue that weighs heavily on me.  Right now we have quite a bit of
deployed code using MII ioctls, and there is a gigabit MII standard; so,
Becker's argument is that each driver should provide a set of MII
ioctls, emulating behavior when hardware isn't exactly per spec.  (yes,
right now they are SIOCDEVPRIVATE, but that can be easily changed to
SIOCDEVMIIxxx)

David's argument is for ethtool, which originally comes out of the sparc
port (see include/asm-sparc/ethtool.h in older trees), and has been
around for a while, but doesn't enjoy the massive deployment that the
MII ioctls enjoy.  We have control over the ethtool API, and we can
correct its deficiencies, whereas any MII spec deficiencies must be
worked out inside the driver.  Further, there is the question of "how
much MII to implement" -- currently the MII-ioctl-based net drivers all
implement -basic- MII, but I guarantee that you will find
per-driver(per-chip) differences in the MII implementation... which is a
flaw in the MII ioctl implementation in the driver, regardless of how
the chip is designed.  There are completeness flaws in more than one MII
ioctl implementation.  Several drivers will return zeroes for the MII id
registers, for example.  The ethtool API doesn't have that problem.

For 2.4, my conclusion is:  for drivers that already implement MII
ioctls as SIOCDEVPRIVATE, continue to support those ioctls.  In
addition, support ethtool.  For drivers without support for either, just
add ethtool support.  The patch being discussed will cut down on
duplicate code for both cases.

Further, for the userland ethtool program, support for MII ioctls will
be added soon, so that there will be no need for additional mii-tool or
mii-diag tools.

For 2.5?  I don't know.  I am not a visionary.  I defer that to Linus
and David and Donald and Jamal and Alexey and...  I am mainly a
maintainer and merge monkey, only implementing new APIs when the needs
are blindingly obvious.


> - You are proposing some caching for the MII registers. I suppose that you
> would like to have this code also working with whatever caching will be
> done for MII access that was recently discussed. Wouldn't this produce
> double caching under some circumstances ?

You misunderstood the code.  The "caching" here is whatever is -already-
being done by the driver.  Many Becker-style drivers cache the
advertising value.  If such a driver uses the ethtool MII code, that is
one less MII read that needs to occur.

If the driver author wishes to cache more stuff, they have to do so in
the obvious way.  struct ethtool_mii_info is only used for helper
functions which are only used inside netdrv_ioctl().


> +       int speed;              /* 10, 100, 1000 or -1 (ask hw)         */
> 
> Please note that the comment specifies 1000, while the code in several
> places assumes only 2 possibilities: 10 and 100.

planning for the future :)  Yes, the code only supports 10/100, as I
mentioned in my introductory message.


> +       if (mii->autoneg < 0)
> +               autoneg = mii->autoneg = (bmcr & BMCR_ANENABLE) ? 1 : 0;
> +       else    autoneg = mii->autoneg;
> 
> You don't read anything from the hardware at this point. Why do you want
> caching ?

I don't understand your question.  Of course we have read BMCR from the
hardware at that point, read the code...


> Not related: I know that this comes from David Miller's older work, but
> wouldn't be possible to have a more uniform naming scheme ? You have
> BMCR_ANENABLE, but you have BMSR_ANEGCAPABLE...

capable != enable.. I prefer them different, so I am therefore
unmotivated to change anything ;-)


> +       if (mii->full_duplex < 0)
> +               full_duplex = mii->full_duplex =
> +                       mii_nway_result(negotiated) & LPA_DUPLEX;
> +       else    full_duplex = mii->full_duplex;
> 
> If autoneg. is disabled, I don't think that you always get useful info in
> 'negotiated'. Applies to the next chunk, too.
> 
> +       if (mii->speed < 0) {
> +               if (negotiated & LPA_100)
> +                       speed = mii->speed = 100;
> +               else
> +                       speed = mii->speed = 10;
> +       } else
> +               speed = mii->speed;

interesting point, thanks.


> +       ecmd->transceiver = XCVR_INTERNAL;
> 
> I didn't understand what XCVR_INTERNAL should mean as opposed to
> XCVR_EXTERNAL or whatever.

It is really up to interpretation of the individual driver author (or in
this case mii.c author), because the net core doesn't know nor care
about XCVR_xxx.


> +       if (advert != mii->advertising) {
> +               bmcr |= BMCR_ANRESTART;
> +               mii->mdio_write(dev, mii->phy_id, MII_ADVERTISE, advert);
> +               mii->advertising = advert;
> +       }
> +
> +       /* some phys need autoneg dis/enabled separately from other settings */
> +       if ((bmcr & BMCR_ANENABLE) && (!(mii->bmcr & BMCR_ANENABLE))) {
> +               mii->mdio_write(dev, mii->phy_id, MII_BMCR,
> +                               mii->bmcr | BMCR_ANENABLE | BMCR_ANRESTART);
> +               bmcr &= ~BMCR_ANRESTART;
> +       } else if ((!(bmcr & BMCR_ANENABLE)) && (mii->bmcr & BMCR_ANENABLE)) {
> +               mii->mdio_write(dev, mii->phy_id, MII_BMCR,
> +                               mii->bmcr & ~BMCR_ANENABLE);
> +       }
> 
> This is nice, but I would like to able to restart autonegotiation even
> without changing any of the advertised capabilities. If I missed this
> possibility, please point me to it...

no, that is a capability which needs to be added to ethtool. 
ETHTOOL_RENEG or ETHTOOL_ANRESTART or something.  Basically kick the
link state machine, whether such a state machine is in the driver or in
the MII phy.  That's the one big thing that mii-tool can do that ethtool
cannot, AFAICS.

	Jeff


-- 
Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |

PATCH: ethtool MII helpers (vers 2)

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 419 bytes --]

Here is an updated version of the ethtool generic MII patch.  It fixes a
few bugs, and adds the capability to restart autonegotiation by passing
AUTONEG_RESTART constant into the existing ETHTOOL_SSET.  Note this
isn't implemented in the code, just added to the ethtool header.

Do not apply, still just for comment and testing.

-- 
Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |

[-- Attachment #2: mii.patch --]
[-- Type: text/plain, Size: 12686 bytes --]

Index: linux_2_4/include/linux/mii.h
diff -u linux_2_4/include/linux/mii.h:1.1.1.1 linux_2_4/include/linux/mii.h:1.1.1.1.52.1
--- linux_2_4/include/linux/mii.h:1.1.1.1	Fri May 11 16:54:44 2001
+++ linux_2_4/include/linux/mii.h	Sun Jun 10 10:26:44 2001
@@ -126,6 +126,33 @@
 #define CSCONFIG_RESV4          0x4000  /* Unused...                   */
 #define CSCONFIG_NDISABLE       0x8000  /* Disable NRZI                */
 
+
+
+struct ethtool_mii_info {
+	struct net_device *dev;	/* our net interface			*/
+	void *useraddr;		/* userspace addr to which we put data	*/
+	
+	int phy_id;		/* PHY we are addressing		*/
+
+	int bmcr;		/* cached MII register values.		*/
+	int bmsr;		/* -1 means 'undefined', which usually	*/
+	int advertising;	/* means the implementation should read	*/
+	int lpa;		/* the values from hardware instead.	*/
+
+	int autoneg;		/* 0 (disabled), 1 (enabled), -1 (ask hw) */
+	unsigned int ignore;	/* mask of medias we never support,	*/
+				/* such as 100baseT4			*/
+	int speed;		/* 10, 100, 1000 or -1 (ask hw)		*/
+	int full_duplex;	/* 0 (no), 1 (yes), -1 (ask hw)		*/
+	unsigned int port;	/* PORT_xxx from linux/ethtool.h	*/
+	
+	int (*mdio_read) (struct net_device *dev, int phy_id, int location);
+	void (*mdio_write) (struct net_device *dev, int phy_id, int location, int val);
+};
+
+int mii_ethtool_gset (struct ethtool_mii_info *mii);
+int mii_ethtool_sset (struct ethtool_mii_info *mii);
+
 /**
  * mii_nway_result
  * @negotiated: value of MII ANAR and'd with ANLPAR
Index: linux_2_4/include/linux/ethtool.h
diff -u linux_2_4/include/linux/ethtool.h:1.1.1.4 linux_2_4/include/linux/ethtool.h:1.1.1.4.84.2
--- linux_2_4/include/linux/ethtool.h:1.1.1.4	Thu Apr 19 17:55:36 2001
+++ linux_2_4/include/linux/ethtool.h	Sun Jun 10 10:56:26 2001
@@ -1,4 +1,4 @@
-/* $Id: ethtool.h,v 1.1.1.4 2001/04/20 00:55:36 jgarzik Exp $
+/* $Id: ethtool.h,v 1.1.1.4.84.2 2001/06/10 17:56:26 jgarzik Exp $
  * ethtool.h: Defines for Linux ethtool.
  *
  * Copyright (C) 1998 David S. Miller (davem@redhat.com)
@@ -34,13 +34,15 @@
 	char	bus_info[32];	/* Bus info for this interface.  For PCI
 				 * devices, use pci_dev->slot_name. */
 	char	reserved1[32];
-	char	reserved2[32];
+	char	reserved2[28];
+	u32	regdump_len;	/* Amount of data from ETHTOOL_GREGS */
 };
 
 /* CMDs currently supported */
 #define ETHTOOL_GSET		0x00000001 /* Get settings. */
 #define ETHTOOL_SSET		0x00000002 /* Set settings, privileged. */
 #define ETHTOOL_GDRVINFO	0x00000003 /* Get driver info. */
+#define ETHTOOL_GREGS		0x00000004 /* Get NIC registers, privileged. */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
@@ -106,5 +108,6 @@
  */
 #define AUTONEG_DISABLE		0x00
 #define AUTONEG_ENABLE		0x01
+#define AUTONEG_RESTART		0x02	/* implies AUTONEG_ENABLE */
 
 #endif /* _LINUX_ETHTOOL_H */
Index: linux_2_4/drivers/net/mii.c
diff -u /dev/null linux_2_4/drivers/net/mii.c:1.1.2.4
--- /dev/null	Tue Jun 12 11:03:26 2001
+++ linux_2_4/drivers/net/mii.c	Sun Jun 10 12:19:43 2001
@@ -0,0 +1,228 @@
+/*
+ *	linux/drivers/net/mii.c
+ *	Copyright 2001 Jeff Garzik <jgarzik@mandrakesoft.com>
+ *
+ *	This software may be used and distributed according to the terms
+ *	of the GNU General Public License, incorporated herein by reference.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/fs.h>
+#include <linux/mii.h>
+#include <linux/ethtool.h>
+#include <asm/uaccess.h>
+
+
+static int mii_fill_ethtool_cmd (struct net_device *dev,
+				  struct ethtool_mii_info *mii,
+				  struct ethtool_cmd *ecmd)
+{
+	unsigned int bmsr, bmcr, v, autoneg, advertising, lpa;
+	unsigned int negotiated, full_duplex, speed;
+
+	memset(ecmd, 0, sizeof(*ecmd));
+	
+	ecmd->cmd = ETHTOOL_GSET;
+
+	if (mii->bmcr < 0)
+		bmcr = mii->bmcr = mii->mdio_read(dev, mii->phy_id, MII_BMCR);
+	else	bmcr = mii->bmcr;
+	if (bmcr == 0xffff)
+		return -EIO;
+
+	if (mii->bmsr < 0)
+		bmsr = mii->bmsr = mii->mdio_read(dev, mii->phy_id, MII_BMSR);
+	else	bmsr = mii->bmsr;
+	if (bmsr == 0xffff)
+		return -EIO;
+
+	if (mii->advertising < 0)
+		advertising = mii->advertising =
+			mii->mdio_read(dev, mii->phy_id, MII_ADVERTISE);
+	else	advertising = mii->advertising;
+	if (advertising == 0xffff)
+		return -EIO;
+
+	if (mii->lpa < 0)
+		lpa = mii->lpa = mii->mdio_read(dev, mii->phy_id, MII_LPA);
+	else	lpa = mii->lpa;
+	if (lpa == 0xffff)
+		return -EIO;
+
+	negotiated = advertising & lpa;
+
+	if (mii->autoneg < 0)
+		autoneg = mii->autoneg = (bmcr & BMCR_ANENABLE) ? 1 : 0;
+	else	autoneg = mii->autoneg;
+		
+	if (mii->full_duplex < 0)
+		full_duplex = mii->full_duplex =
+			(mii_nway_result(negotiated) & LPA_DUPLEX) ? 1 : 0;
+	else	full_duplex = mii->full_duplex;
+		
+	if (mii->speed < 0) {
+		if (negotiated & LPA_100)
+			speed = mii->speed = 100;
+		else
+			speed = mii->speed = 10;
+	} else
+		speed = mii->speed;
+	
+	ecmd->supported = SUPPORTED_MII;
+	v = bmsr & ~mii->ignore;
+	if (v & BMSR_10HALF)
+		ecmd->supported |= SUPPORTED_10baseT_Half;
+	if (v & BMSR_10FULL)
+		ecmd->supported |= SUPPORTED_10baseT_Full;
+	if (v & BMSR_100HALF)
+		ecmd->supported |= SUPPORTED_100baseT_Half;
+	if (v & BMSR_100FULL)
+		ecmd->supported |= SUPPORTED_100baseT_Full;
+	if (bmsr & BMSR_ANEGCAPABLE)
+		ecmd->supported |= SUPPORTED_Autoneg;
+	else
+		autoneg = mii->autoneg = 0;
+
+	ecmd->advertising = ADVERTISED_MII;
+	v = advertising & ~mii->ignore;
+	if (v & ADVERTISE_10HALF)
+		ecmd->advertising |= ADVERTISED_10baseT_Half;
+	if (v & ADVERTISE_10FULL)
+		ecmd->advertising |= ADVERTISED_10baseT_Full;
+	if (v & ADVERTISE_100HALF)
+		ecmd->advertising |= ADVERTISED_100baseT_Half;
+	if (v & ADVERTISE_100FULL)
+		ecmd->advertising |= ADVERTISED_100baseT_Full;
+	if (autoneg) {
+		ecmd->advertising |= ADVERTISED_Autoneg;
+		ecmd->autoneg = AUTONEG_ENABLE;
+	} else
+		ecmd->autoneg = AUTONEG_DISABLE;
+
+	ecmd->speed = speed == 100 ? SPEED_100 : SPEED_10;
+	ecmd->duplex = full_duplex ? DUPLEX_FULL : DUPLEX_HALF;
+	ecmd->port = PORT_MII;
+	ecmd->phy_address = mii->phy_id;
+	ecmd->transceiver = XCVR_INTERNAL;
+
+	return 0;
+}
+
+int mii_ethtool_gset (struct ethtool_mii_info *mii)
+{
+	struct ethtool_cmd ecmd;
+	int rc;
+	
+	if (mii->port != PORT_MII)
+		return -EOPNOTSUPP;
+
+	rc = mii_fill_ethtool_cmd(mii->dev, mii, &ecmd);
+	if (rc)
+		return rc;
+
+	if (copy_to_user(mii->useraddr, &ecmd, sizeof(ecmd)))
+		return -EFAULT;
+
+	return 0;
+}
+
+int mii_ethtool_sset (struct ethtool_mii_info *mii)
+{
+	struct net_device *dev = mii->dev;
+	struct ethtool_cmd in, out;
+	unsigned int advert, bmcr;
+	int rc;
+
+	if (copy_from_user (&in, mii->useraddr, sizeof (in)))
+		return -EFAULT;
+	rc = mii_fill_ethtool_cmd (dev, mii, &out);
+	if (rc)
+		return rc;
+	
+	if (in.port != out.port) {
+		if (copy_to_user(mii->useraddr, &in, sizeof(in)))
+			return -EFAULT;
+		mii->port = in.port;
+		return 0;
+	}
+	
+	/* we don't support changing phy address, tranceiver,
+	 * or the interrupt mitigation stuff.
+	 */
+	if ((in.phy_address != out.phy_address) ||
+	    (in.transceiver != XCVR_INTERNAL) ||
+	    (in.maxtxpkt != out.maxtxpkt) ||
+	    (in.maxrxpkt != out.maxrxpkt))
+		return -EOPNOTSUPP;
+
+	advert = mii->advertising & ~ADVERTISE_ALL;
+
+	/* NWAY autonegotiation enabled */
+	if (in.autoneg == AUTONEG_ENABLE) {
+		bmcr = mii->bmcr | BMCR_ANENABLE;
+
+		if (in.advertising & ADVERTISED_10baseT_Half)
+			advert |= ADVERTISE_10HALF;
+		if (in.advertising & ADVERTISED_10baseT_Full)
+			advert |= ADVERTISE_10FULL;
+		if (in.advertising & ADVERTISED_100baseT_Half)
+			advert |= ADVERTISE_100HALF;
+		if (in.advertising & ADVERTISED_100baseT_Full)
+			advert |= ADVERTISE_100FULL;
+		if (advert == (mii->advertising & ~ADVERTISE_ALL))
+			return -EINVAL;
+	}
+
+	/* NWAY autonegotiation disabled */
+	else {
+		bmcr = mii->bmcr & ~BMCR_ANENABLE;
+		
+		if (in.speed == SPEED_100)
+			bmcr |= BMCR_SPEED100;
+		else	bmcr &= ~BMCR_SPEED100;
+
+		if (in.duplex == DUPLEX_FULL)
+			bmcr |= BMCR_FULLDPLX;
+		else	bmcr &= ~BMCR_FULLDPLX;
+
+		if (mii->bmsr & BMSR_10HALF)
+			advert |= ADVERTISE_10HALF;
+		if (mii->bmsr & BMSR_10FULL)
+			advert |= ADVERTISE_10FULL;
+		if (mii->bmsr & BMSR_100HALF)
+			advert |= ADVERTISE_100HALF;
+		if (mii->bmsr & BMSR_100FULL)
+			advert |= ADVERTISE_100FULL;
+	}
+
+	if (advert != mii->advertising) {
+		bmcr |= BMCR_ANRESTART;
+		mii->mdio_write(dev, mii->phy_id, MII_ADVERTISE, advert);
+		mii->advertising = advert;
+	}
+
+	/* some phys need autoneg dis/enabled separately from other settings */
+	if ((bmcr & BMCR_ANENABLE) && (!(mii->bmcr & BMCR_ANENABLE))) {
+		mii->mdio_write(dev, mii->phy_id, MII_BMCR,
+				mii->bmcr | BMCR_ANENABLE | BMCR_ANRESTART);
+		bmcr &= ~BMCR_ANRESTART;
+	} else if ((!(bmcr & BMCR_ANENABLE)) && (mii->bmcr & BMCR_ANENABLE)) {
+		mii->mdio_write(dev, mii->phy_id, MII_BMCR,
+				mii->bmcr & ~BMCR_ANENABLE);
+	}
+
+	if (bmcr != mii->bmcr) {
+		mii->mdio_write(dev, mii->phy_id, MII_BMCR, bmcr);
+		bmcr &= ~BMCR_ANRESTART;
+		mii->bmcr = bmcr;
+	}
+	
+	if (copy_to_user(mii->useraddr, &out, sizeof(out)))
+		return -EFAULT;
+
+	return 0;
+}
+
+EXPORT_SYMBOL(mii_ethtool_gset);
+EXPORT_SYMBOL(mii_ethtool_sset);
Index: linux_2_4/drivers/net/epic100.c
diff -u linux_2_4/drivers/net/epic100.c:1.1.1.35 linux_2_4/drivers/net/epic100.c:1.1.1.35.42.4
--- linux_2_4/drivers/net/epic100.c:1.1.1.35	Sat May 19 18:56:00 2001
+++ linux_2_4/drivers/net/epic100.c	Sun Jun 10 12:42:37 2001
@@ -45,13 +45,16 @@
 	* { fill me in }
 
 	LK1.1.8:
-	* ethtool support (jgarzik)
+	* ethtool driver info support (jgarzik)
 
+	LK1.1.9:
+	* ethtool media get/set support (jgarzik)
+
 */
 
 #define DRV_NAME	"epic100"
-#define DRV_VERSION	"1.11+LK1.1.8"
-#define DRV_RELDATE	"May 18, 2001"
+#define DRV_VERSION	"1.11+LK1.1.9"
+#define DRV_RELDATE	"June 10, 2001"
 
 
 /* The user-configurable values.
@@ -116,6 +119,7 @@
 #include <linux/init.h>
 #include <linux/spinlock.h>
 #include <linux/ethtool.h>
+#include <linux/mii.h>
 #include <asm/bitops.h>
 #include <asm/io.h>
 #include <asm/uaccess.h>
@@ -135,6 +139,11 @@
 MODULE_PARM(rx_copybreak, "i");
 MODULE_PARM(options, "1-" __MODULE_STRING(MAX_UNITS) "i");
 MODULE_PARM(full_duplex, "1-" __MODULE_STRING(MAX_UNITS) "i");
+MODULE_PARM_DESC(debug, "EPIC/100 debug level (0-5)");
+MODULE_PARM_DESC(max_interrupt_work, "EPIC/100 maximum events handled per interrupt");
+MODULE_PARM_DESC(options, "EPIC/100: Bits 0-3: media type, bit 4: full duplex");
+MODULE_PARM_DESC(rx_copybreak, "EPIC/100 copy breakpoint for copy-only-tiny-frames");
+MODULE_PARM_DESC(full_duplex, "EPIC/100 full duplex setting(s) (1)");
 
 /*
 				Theory of Operation
@@ -1169,7 +1178,7 @@
 			if (pkt_len > PKT_BUF_SZ - 4) {
 				printk(KERN_ERR "%s: Oversized Ethernet frame, status %x "
 					   "%d bytes.\n",
-					   dev->name, pkt_len, status);
+					   dev->name, status, pkt_len);
 				pkt_len = 1514;
 			}
 			/* Check if the packet is long enough to accept without copying
@@ -1344,27 +1353,64 @@
 	return;
 }
 
-static int netdev_ethtool_ioctl(struct net_device *dev, void *useraddr)
+static int netdev_ethtool_ioctl (struct net_device *dev, void *useraddr)
 {
 	struct epic_private *np = dev->priv;
 	u32 ethcmd;
-		
-	if (copy_from_user(&ethcmd, useraddr, sizeof(ethcmd)))
+
+	if (copy_from_user (&ethcmd, useraddr, sizeof (ethcmd)))
 		return -EFAULT;
+
+	switch (ethcmd) {
+	case ETHTOOL_GDRVINFO:
+		{
+			struct ethtool_drvinfo info = { ETHTOOL_GDRVINFO };
+			strcpy (info.driver, DRV_NAME);
+			strcpy (info.version, DRV_VERSION);
+			strcpy (info.bus_info, np->pci_dev->slot_name);
+			if (copy_to_user (useraddr, &info, sizeof (info)))
+				return -EFAULT;
+			return 0;
+		}
+
+	case ETHTOOL_GSET:
+	case ETHTOOL_SSET:
+		{
+			struct ethtool_mii_info info = {
+				dev:		dev,
+				useraddr:	useraddr,
+				phy_id:		np->phys[0],
+				bmcr:		-1,
+				bmsr:		-1,
+				lpa:		-1,
+				advertising:	np->advertising,
+				autoneg:	-1,
+				ignore:		ADVERTISE_100BASE4,
+				speed:		-1,
+				full_duplex:	np->full_duplex,
+				port:		PORT_MII,
+				mdio_read:	mdio_read,
+				mdio_write:	mdio_write,
+			};
+			int rc;
+
+			if (ethcmd == ETHTOOL_GSET)
+				rc = mii_ethtool_gset (&info);
+			else
+				rc = mii_ethtool_sset (&info);
 
-        switch (ethcmd) {
-        case ETHTOOL_GDRVINFO: {
-		struct ethtool_drvinfo info = {ETHTOOL_GDRVINFO};
-		strcpy(info.driver, DRV_NAME);
-		strcpy(info.version, DRV_VERSION);
-		strcpy(info.bus_info, np->pci_dev->slot_name);
-		if (copy_to_user(useraddr, &info, sizeof(info)))
-			return -EFAULT;
-		return 0;
+			np->advertising = info.advertising;
+			np->full_duplex = info.full_duplex;
+
+			check_media (dev);
+
+			return rc;
+		}
+
+	default:
+		break;
 	}
 
-        }
-	
 	return -EOPNOTSUPP;
 }
 

Re: PATCH: ethtool MII helpers

[Bogdan Costescu]

On Tue, 12 Jun 2001, Jeff Garzik wrote:

> What are you doing that you need to access all registers from userspace?

The main purpose is debugging. Some registers give you more detailed info
about what's going on; you don't need this for normal functioning, but
when you're looking for "what's wrong" it might give you additional
details (next page, transceiver manufacturer/model for hardware bugs,
etc.). That's why I proposed restricted access, a normal user shouldn't
need this info, but the sysadmin might.
Probably the same info can be taken from the card inside the kernel,
convert it to some user parsable form and give it to user through /proc or
something similar. But I think that the implementation would be much more
complicated than allowing direct access to MII registers.

One other argument: mii-diag currently allows this. ethtool would mean a
step backwards 8-)

> Right now we have quite a bit of
> deployed code using MII ioctls, and there is a gigabit MII standard; so,
> Becker's argument is that each driver should provide a set of MII
> ioctls, emulating behavior when hardware isn't exactly per spec.

I'm more-or-less supporting this oppinion. But I use Don's mii-diag
heavily in debugging media-related problems for 3c59x, so I might be
biased 8-)

> We have control over the ethtool API, and we can
> correct its deficiencies, whereas any MII spec deficiencies must be
> worked out inside the driver.

I agree that this is a problem. Even more, if you start emulating MII for
non-MII NICs, you might get into trouble when presenting the available
info in a MII-compatible way (f.e. how would you emulate "link partner
ability" ?). But the NIC specific deficiencies should be worked out inside
the driver, isn't this always the case with a common API ?

> Further, there is the question of "how much MII to implement" --
> currently the MII-ioctl-based net drivers all implement -basic- MII, but
> I guarantee that you will find per-driver(per-chip) differences in the
> MII implementation... which is a flaw in the MII ioctl implementation in
> the driver, regardless of how the chip is designed.

I take that you mean by implementing basic MII that the drivers don't take
advantage of the additional info when dealing with media settings. The
drivers do allow user access to all MII registers when available.
Per-driver or per-chip differences mean that the driver author didn't do a
good job at emulating MII 8-)

> There are completeness flaws in more than one MII ioctl implementation.

IMHO, this is only because there is no agreed-upon standard. But this can
be corrected. What prevents ethtool implementations from being flawed ?

> The ethtool API doesn't have that problem.

Well, IMHO you can't directly compare the two. MII has hardware support,
so for a MII-capable NIC, you usually just handle access to the registers.
ethtool is software only and you emulate everything; if you would
also partly emulate MII (where you need to), you are in the same
situation.

> For drivers without support for either, just add ethtool support.

Well, that's my point. You need to write code in both cases. So why do you
choose ethtool ?

> For 2.5?  I don't know.  I am not a visionary.  I defer that to Linus
> and David and Donald and Jamal and Alexey and...  I am mainly a
> maintainer and merge monkey, only implementing new APIs when the needs
> are blindingly obvious.

I don't want to push anything. But when oppinions start to diverge, there
will always be (from all sides!) something like: "my version can do this,
but yours can't". So I'm all for _one_ way of doing things.

> You misunderstood the code.  The "caching" here is whatever is -already-
> being done by the driver.  Many Becker-style drivers cache the
> advertising value.  If such a driver uses the ethtool MII code, that is
> one less MII read that needs to occur.

No, I was talking mainly about 'bmcr' and 'bmsr'. I'm not aware of any
driver that caches these values currently.

> > +       if (mii->autoneg < 0)
> > +               autoneg = mii->autoneg = (bmcr & BMCR_ANENABLE) ? 1 : 0;
> > +       else    autoneg = mii->autoneg;
> >
> > You don't read anything from the hardware at this point. Why do you want
> > caching ?
>
> I don't understand your question.  Of course we have read BMCR from the
> hardware at that point, read the code...

My question was directly related to caching of 'autoneg'. You need to read
'bmcr' before, sure, but why not directly "computing" autoneg instead of
the "if" ? What do you achieve by setting autoneg to potentially something
else than the actual BMCR setting ?

> It is really up to interpretation of the individual driver author (or in
> this case mii.c author), because the net core doesn't know nor care
> about XCVR_xxx.

Yes, but it might make a difference for debugging too. For the example
that I gave, it really helps knowing which of the 2 MII transceivers on
the card is used. So, this info might need to be propagated as exactly
as possible even to user space. And probably needs to be driver-specific,
not in mii.c, anyway.

Sincerely,

Bogdan Costescu

IWR - Interdisziplinaeres Zentrum fuer Wissenschaftliches Rechnen
Universitaet Heidelberg, INF 368, D-69120 Heidelberg, GERMANY
Telephone: +49 6221 54 8869, Telefax: +49 6221 54 8868
E-mail: Bogdan.Costescu@IWR.Uni-Heidelberg.De

Re: PATCH: ethtool MII helpers

[Donald Becker]

I was on vacation, and thus didn't have the opportunity to comment earlier.

This message covers
   Why caching MII values doesn't work.
   Why extended MII values are useful.

On Tue, 12 Jun 2001, Jeff Garzik wrote:

> > - You are proposing some caching for the MII registers. I suppose that you
> > would like to have this code also working with whatever caching will be
> > done for MII access that was recently discussed. Wouldn't this produce
> > double caching under some circumstances ?
> 
> You misunderstood the code.  The "caching" here is whatever is -already-
> being done by the driver.  Many Becker-style drivers cache the
> advertising value.  If such a driver uses the ethtool MII code, that is
> one less MII read that needs to occur.

That's not the way I read the code.  It appears to cache various MII
management registers.

Caching almost any MII register, except the ID registers, may be
invalid.  My drivers record values written to MII registers (note 1), but
always does an actual read.

Here is a quick summary of the basic mode registers

 MII Reg   Function     When it changes
  0  Control register -- May return the current autonegotiated status.
  1  Status register  -- Changes with link status and other events.
 2&3 ID registers     -- Should never change
  4 Advertised value  -- may change with a transceiver reset
  5 Link partner      -- changes with negotiation, and "next page" feature

(1) The drivers record the autonegotiation advertised value, and
recently have been updated to allow writes to the control register to
force the speed and duplex.

Caching and ioctl() rate-limiting are both a problem for a program I use
frequently.  It monitors the transceiver to report the timing and state
transitions of autonegotiation.  It internally handles polling rate
limiting by backing off the poll rate when nothing is happening.  But
when something happens, it polls every timer tick for the next 30
ticks.

> Bogdan Costescu wrote:
> > On Sun, 10 Jun 2001, Jeff Garzik wrote:
> > - I don't know what the long-term plan is about ethtool vs. MII ioctl's.
> > If you do plan to replace completely the MII ioctl's, there should be a
> > way to access _all_ MII registers provided by the PHY, even if you do this
> > in a restricted way (i.e. for CAP_NET_ADMIN only). There is also useful
> > info in other registers than the 4 you have in your implementation.
> 
> What are you doing that you need to access all registers from userspace?

That's an easy one: "next page" information, diagnostics, status
reports, and extended configuration.

Much useful information is reported by certain MII transceivers.  People
that care select transceivers that provide the extended information.

Diagnostic reports
   The approximate distance to the first major impedence mismatch on the
   cable.
Signal status reports.
   Signal level -- estimate if the cable is to long or flawed.
   Signal to noise -- estimate the reliability of the link.
   Near-end cross-talk level.
   Reversed receive polarity.

Operational errors
   Symbol coding error count
   Symbol sequence error count
   Decoder/PLL slip indication.

Some examples of extended configuration are
   Increasing or decreasing the transmit level.
   Setting a lowered recieve threshold to allow marginal non-noisy links
     to work.
   Using symbol coding over fiber.
   Changing the information reported on the LED outputs

> Becker's argument is that each driver should provide a set of MII
> ioctls, emulating behavior when hardware isn't exactly per spec.  (yes,
> right now they are SIOCDEVPRIVATE, but that can be easily changed to
> SIOCDEVMIIxxx)

My driver sources converted to using specific names, which are currently
mapped as follows
#ifndef SIOCGMIIPHY
#define SIOCGMIIPHY (SIOCDEVPRIVATE)	/* Get the PHY in use. */
#define SIOCGMIIREG (SIOCDEVPRIVATE+1)	/* Read a PHY register. */
#define SIOCSMIIREG (SIOCDEVPRIVATE+2)	/* Write a PHY register. */
#define SIOCGPARAMS (SIOCDEVPRIVATE+3)	/* Read operational parameters. */
#define SIOCSPARAMS (SIOCDEVPRIVATE+4)	/* Set operational parameters. */


> David's argument is for ethtool, which originally comes out of the sparc
> port (see include/asm-sparc/ethtool.h in older trees), and has been
> around for a while, but doesn't enjoy the massive deployment that the
> MII ioctls enjoy.  We have control over the ethtool API, and we can
> correct its deficiencies, whereas any MII spec deficiencies must be
> worked out inside the driver.

You should first understand what MII management registers provide before
deciding that you can do better.  There are some design uglinesses,
but it was put together by people that lived and breathed transceivers.
It has been proven over six or seven years or use with no incompatible
changes to the original software interface definition.

>...
> the chip is designed.  There are completeness flaws in more than one MII
> ioctl implementation.  Several drivers will return zeroes for the MII id
> registers, for example.  The ethtool API doesn't have that problem.

Returning zeros for the MII ID registers is accepted industry practice
for integrated transceivers.  We could have the driver substitute a
specific ID, but this isn't an actual problem.

> Further, for the userland ethtool program, support for MII ioctls will
> be added soon, so that there will be no need for additional mii-tool or
> mii-diag tools.

This could be easily reversed: the additional ethtool program was not
needed in the first place.

> > This is nice, but I would like to able to restart autonegotiation even
> > without changing any of the advertised capabilities. If I missed this
> > possibility, please point me to it...
> 
> no, that is a capability which needs to be added to ethtool. 
> ETHTOOL_RENEG or ETHTOOL_ANRESTART or something.  Basically kick the
> link state machine, whether such a state machine is in the driver or in
> the MII phy.  That's the one big thing that mii-tool can do that ethtool
> cannot, AFAICS.

An additional capability of the MII ioctl() is that it permits sending
"next page" extended information to the link partner.

Donald Becker				becker@scyld.com
Scyld Computing Corporation		http://www.scyld.com
410 Severn Ave. Suite 210		Second Generation Beowulf Clusters
Annapolis MD 21403			410-990-9993

Re: PATCH: ethtool MII helpers

[Jeff Garzik]

Donald Becker wrote:
> I was on vacation, and thus didn't have the opportunity to comment earlier.

Thanks a bunch for your comments here.


> On Tue, 12 Jun 2001, Jeff Garzik wrote:
> 
> > > - You are proposing some caching for the MII registers. I suppose that you
> > > would like to have this code also working with whatever caching will be
> > > done for MII access that was recently discussed. Wouldn't this produce
> > > double caching under some circumstances ?
> >
> > You misunderstood the code.  The "caching" here is whatever is -already-
> > being done by the driver.  Many Becker-style drivers cache the
> > advertising value.  If such a driver uses the ethtool MII code, that is
> > one less MII read that needs to occur.
> 
> That's not the way I read the code.  It appears to cache various MII
> management registers.

I still think there is a misunderstanding here, brought about my short
explanation and lack of docs..

The key here is the lifetime of the cache.  Without extra work on the
part of the driver author, the data in struct ethtool_mii_info only
exists for a single ioctl call.  ethtool_mii_info is a container, not a
data cache.  So, if you already have MII register cached somewhere, like
advertising, or you perform MII reads before calling
ethtool_mii_[gs]set, then those values are "cached" in the sense that
mii.c will not re-read the register values.

Since MII reads are not the quickest operations in the world, I
preferred to be flexible in allowing what will occur before and after
the ethtool_mii_[gs]set call.


> Caching almost any MII register, except the ID registers, may be
> invalid.

Agreed.  I even said this in an MII thread on lkml a couple weeks ago
;-)


> Caching and ioctl() rate-limiting are both a problem for a program I use
> frequently.  It monitors the transceiver to report the timing and state
> transitions of autonegotiation.  It internally handles polling rate
> limiting by backing off the poll rate when nothing is happening.  But
> when something happens, it polls every timer tick for the next 30
> ticks.

Unfortunately that is at loggerheads with the potential for a bunch of
people to soak the system with unpriveleged MII reads via ioctl.  That
is the core problem, and caching or rate-limiting is only a suggested
solution.

I could forget about rate-limiting if we required CAP_NET_ADMIN and/or
CAP_RAW_IO for all these ioctls, but that might cause complaints too..


> > David's argument is for ethtool, which originally comes out of the sparc
> > port (see include/asm-sparc/ethtool.h in older trees), and has been
> > around for a while, but doesn't enjoy the massive deployment that the
> > MII ioctls enjoy.  We have control over the ethtool API, and we can
> > correct its deficiencies, whereas any MII spec deficiencies must be
> > worked out inside the driver.
> 
> You should first understand what MII management registers provide before
> deciding that you can do better.  There are some design uglinesses,
> but it was put together by people that lived and breathed transceivers.
> It has been proven over six or seven years or use with no incompatible
> changes to the original software interface definition.
> 
> >...
> > Further, for the userland ethtool program, support for MII ioctls will
> > be added soon, so that there will be no need for additional mii-tool or
> > mii-diag tools.
> 
> This could be easily reversed: the additional ethtool program was not
> needed in the first place.
> 
> > > This is nice, but I would like to able to restart autonegotiation even
> > > without changing any of the advertised capabilities. If I missed this
> > > possibility, please point me to it...
> >
> > no, that is a capability which needs to be added to ethtool.
> > ETHTOOL_RENEG or ETHTOOL_ANRESTART or something.  Basically kick the
> > link state machine, whether such a state machine is in the driver or in
> > the MII phy.  That's the one big thing that mii-tool can do that ethtool
> > cannot, AFAICS.
> 
> An additional capability of the MII ioctl() is that it permits sending
> "next page" extended information to the link partner.

[move this down here]
> This message covers
>    Why caching MII values doesn't work.
[responded above]
>    Why extended MII values are useful.
Ok, thanks, agreed.

About the larger issue of why ethtool exists, I wonder about things
like:  how do the MII ioctls cover things like switching transceivers? 
supporting aui/10b2?  supporting sym phys?

ethtool is not just about 10/100 media.  It's a general software
diagnostics utility and tuning tool for your ethernet driver.  The same
kernel interface and the same userland program will allow me to
associate an ethernet interface with a driver and bus location, adjust
media settings, adjust interrupt mitigation settings, or perhaps even
perform a driver-specific duty.

I am very much convinced that the extended MII ioctls are useful, and
would even support codifying them in sockios.h, using the SIOCMII* names
you are already using.

However I see the MII ioctls as a tuning tool for a specific (though
large) subset of hardware.  I am still not comfortable with considering
the MII ioctls as the standard for communication between the kernel and
userland...

Tangent, to close on a more concrete technical note.  The MII ioctls in
their current form are not completely portable.  For DaveM and others
doing 32-bit userland on 64-bit kernel, you have to pass through ioctl
translation layer.  Not only will the SIOCMIIxxx ioctls need to be made
official, but the structure which has so far been implicitly defined
(u16* data) in the ioctls would need to be explicitly defined, in some
central location.

Regards,

	Jeff


-- 
Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |

Re: PATCH: ethtool MII helpers

[Bogdan Costescu]

On Wed, 13 Jun 2001, Jeff Garzik wrote:

> > Caching and ioctl() rate-limiting are both a problem for a program I use
> > frequently.
>
> Unfortunately that is at loggerheads with the potential for a bunch of
> people to soak the system with unpriveleged MII reads via ioctl.
>
> I could forget about rate-limiting if we required CAP_NET_ADMIN and/or
> CAP_RAW_IO for all these ioctls, but that might cause complaints too..

In the last thread, I proposed that caching/rate-limiting should apply
only to unpriviledged users. This way applications like Don's would still
run (but require to be run as root) and normal users would not DoS it.
I was thinking of something like this (caching applied to 3c59x):

static int vortex_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
{
...
        switch(cmd) {
        case SIOCDEVPRIVATE:            /* Get the address of the PHY in use. */
                data[0] = phy;
		break;
        case SIOCDEVPRIVATE+1:          /* Read the specified MII register. */
		reg = data[1] & 0x1f;
		if (capable(CAP_NET_ADMIN) || (vp->cache.to[reg] + MII_CACHE_TIMEOUT < jiffies)) {
	                EL3WINDOW(4);
	                data[3] = mdio_read(dev, data[0] & 0x1f, reg);
			vp->cache.val[reg] = data[3];
			vp->cache.to[reg] = jiffies;
		} else {
			data[3] = vp->cache.val[reg];
		}
                retval = 0;
                break;
	case ...

where MII_CACHE_TIMEOUT is a constant here, but should be something
modifiable through /proc. I've choosen a resolution of HZ for cached reads
as it's easily accesible. The example is obviously simplified, it only
handles one transceiver; if there can be more active at the same time,
each should have it's own cache.

> About the larger issue of why ethtool exists, I wonder about things
> like:  how do the MII ioctls cover things like switching transceivers?
> supporting aui/10b2?  supporting sym phys?

AFAIK, MII ioctl's are right now only allowing access to MII registers.
Switching transceivers is a tough job, that's why it's generally done in
init()/open(). If you mean by "switching" just a "set this transceiver for
use", this might be possible to do, but if you want to check if a
transceiver is available and then "set for use", this can't be done
easily. Some transceivers can't return any info and the general way of
probing them is by trying to send/receive something - which opens the
window for nice races with the Tx/Rx parts which can be active at that
time. That's why my impression is that changing transceivers can only
be safely done at init()/open().
AUI cannot be probed, so you have to send/receive something. Furthemore, I
don't think that the AUI interface tells you anything about what's
connected to it, so you have to blindly activate it and hope that it
works. 10base2 might give link beat, but otherwise AUI considerations
apply. AFAIK, Sym phys are easier to emulate (isn't tulip already doing
this ?).

>  The same
> kernel interface and the same userland program will allow me to
> associate an ethernet interface with a driver and bus location, adjust
> media settings, adjust interrupt mitigation settings, or perhaps even
> perform a driver-specific duty.

So far, Don's work proved very well thought. There's a mii-diag which
deals with media settings for MII-capable NICs and there are diag tools
for each chipset (vortex-diag, tulip-diag, etc.) which deal with
driver-specific duties.

[ I don't have any relationship with Don. In fact, you can see on the
vortex list that we disagreed many times. ]

I wouldn't object to making mii-diag able to more generally deal with
media settings (probably by provinding MII-like interfaces for NICs that
don't have MII). But the other way around of trying to do driver-specific
duties from _one_ tool seems a bit to hard for me.
AFAIK, associating ethernet interfaces with drivers and bus locations has
no standard right now, but I agree that is a need. So, if everybody
agrees, the ethtool way can be stated as the standard.
Again AFAIK, interrupt mitigation has no standard right now, in fact only
few drivers support it. So the fact that is available from ethtool is not
really relevant to me (before you ask, 3c59x supported hardware doesn't
have hardware Rx interrupt mitigation). If Jamal's work for general
interrupt mitigation will be included, then I surely see the need for a
tool to control it. As it will be a core functionality, one tool will do
it, there's no driver dependency.

> However I see the MII ioctls as a tuning tool for a specific (though
> large) subset of hardware.  I am still not comfortable with considering
> the MII ioctls as the standard for communication between the kernel and
> userland...

The low level stuff should be the same in both cases (MII-like and
ethtool). Is what you add on top of it that makes it MII-like or ethtool.
Or am I missing something ?

Sincerely,

Bogdan Costescu

IWR - Interdisziplinaeres Zentrum fuer Wissenschaftliches Rechnen
Universitaet Heidelberg, INF 368, D-69120 Heidelberg, GERMANY
Telephone: +49 6221 54 8869, Telefax: +49 6221 54 8868
E-mail: Bogdan.Costescu@IWR.Uni-Heidelberg.De

Re: PATCH: ethtool MII helpers

[Chris Wedgwood]

On Sun, Jun 10, 2001 at 01:34:59PM -0400, Jeff Garzik wrote:

    Initial draft of a helper which uses generic elements present in several
    net drivers to implement ethtool ioctl support in a minimum amount of
    code.
    
    I have included a sample implementation in the epic100 driver, to
    illustrate how these helpers may be used.  This should make it easier to
    implement support across 10/100 hardware which uses primarily an MII
    phy.
    
    Comments appreciated.

Can someone explain to me why we have ethtool and mii-tool? Can we
not extend ethtool for the mii-tool stuff, even if only at userland?



  --cw

Re: PATCH: ethtool MII helpers

[Jeff Garzik]

Chris Wedgwood wrote:
> Can we
> not extend ethtool for the mii-tool stuff, even if only at userland?

Sure, and that's planned.  Wanna send me a patch for it?  :)

It will definitely fall back on the MII ioctls if ethtool media support
for the desired command doesn't exist.

-- 
Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |

Re: PATCH: ethtool MII helpers

[Chris Wedgwood]

On Fri, Jun 22, 2001 at 01:24:36AM -0400, Jeff Garzik wrote:

    Sure, and that's planned.  Wanna send me a patch for it?  :)

Possibly, but I wonder if this is a kernel-space problem or not. Why
not put all the smarts into userland for it?

    It will definitely fall back on the MII ioctls if ethtool media
    support for the desired command doesn't exist.

Well, that is more or less as much as needs to be done. That, and
some kind of super-set API to be defined for all new stuff, having
two slightly different APIs for the same things sucks.



   --cw

Re: PATCH: ethtool MII helpers

[Jeff Garzik]

Chris Wedgwood wrote:
> 
> On Fri, Jun 22, 2001 at 01:24:36AM -0400, Jeff Garzik wrote:
> 
>     Sure, and that's planned.  Wanna send me a patch for it?  :)
> 
> Possibly, but I wonder if this is a kernel-space problem or not. Why
> not put all the smarts into userland for it?

I meant, send me a patch for userland ethtool, to do exactly what you
described.


>     It will definitely fall back on the MII ioctls if ethtool media
>     support for the desired command doesn't exist.
> 
> Well, that is more or less as much as needs to be done. That, and
> some kind of super-set API to be defined for all new stuff, having
> two slightly different APIs for the same things sucks.

Both APIs do different things but have a common subset, yes.

The MII ioctls only do their thing for MII-like hardware.  ethtool can
be applied to any hardware.  Old ISA drivers that don't do MII, or do it
in a really nonstandard way.  For example I have ethtool code locally
which allows ne2k-pci to do media selection via ioctl, for two popular
ne2k cards, something its never been able to do before.  Emulating media
selection support for things like 10base2<->10baseT<->AUI just isn't
possible with the MII ioctls.

MII is a standard and incredibly popular, thus mii-tool works most
popular PCI NICs, for the most popular media types.  But it's still
basically a hardware interface.  I am not convinced its a good idea for
make the [G]MII ioctls the Linux software media interface for all
network hardware.

I see ethtool as the interface for tuning your NIC, that works across
all hardware.
I see mii-diag as the way to do advance MII-specific hardware stuff,
like next page or HA monitoring or whatever.

	Jeff


-- 
Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |