Skip to contentSkip to conversation

PATCH: ethtool MII helpers

PATCH Jeff Garzik 6 participants 14 messages Jun 10, 2001 – Jun 22, 2001

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

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.

Signature · 3 lines
Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |

Index: linux_2_4/include/linux/ethtool.h

include/linux/ethtool.h renamed from include/linux/ethtool.h:1.1.1.4 +4 -2
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
@@ -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 ([email protected])
@@ -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

include/linux/mii.h renamed from include/linux/mii.h:1.1.1.1 +27 -0
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
@@ -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

drivers/net/mii.c new file +212 -0

212 lines not on this page · raw message

Index: linux_2_4/drivers/net/epic100.c

drivers/net/epic100.c renamed from drivers/net/epic100.c:1.1.1.35 +72 -18

131 lines not on this page · raw message

#1 lore raw
Quoted 3 lines · Jeff Garzik

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?

Signature · 3 lines
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
#2 lore raw
Quoted 5 lines · Pekka Savola
drivers/net/epic100.c renamed from drivers/net/epic100.c:1.1.1.35 +2 -0
+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.

#3 lore raw
In reply to Jeff Garzik
Quoted 1 line · Jeff Garzik

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!

Signature · 6 lines
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: [email protected]
#4 lore raw
Quoted 6 lines · Bogdan Costescu

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.

Quoted 4 lines

- 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().

Quoted 4 lines

+ 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.

Quoted 6 lines
drivers/net/mii.c new file +3 -0
+       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...

Quoted 3 lines

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 ;-)

Quoted 8 lines
drivers/net/mii.c new file +4 -0
+       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.

drivers/net/mii.c line 62

Quoted 7 lines
drivers/net/mii.c new file +7 -0
+       if (mii->speed < 0) {
+               if (negotiated & LPA_100)
+                       speed = mii->speed = 100;
+               else
+                       speed = mii->speed = 10;
+       } else
+               speed = mii->speed;

interesting point, thanks.

Quoted 4 lines

+ 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.

Quoted 16 lines
drivers/net/mii.c new file +15 -0
+       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);
+       }
Quoted 3 lines

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

Signature · 3 lines
Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |
#5 lore raw
Quoted 1 line · Jeff Garzik

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-)

Quoted 4 lines

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-)

Quoted 3 lines

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 ?

Quoted 5 lines

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-)

Quoted 1 line

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 ?

Quoted 1 line

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.

Quoted 1 line

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 ?

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 9 lines
Quoted 6 lines
drivers/net/mii.c new file +3 -0
+       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 ?

Quoted 3 lines

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: [email protected]

#6 lore raw
In reply to Jeff Garzik

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.

Quoted 9 lines · Jeff Garzik
Quoted 4 lines

- 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.

Quoted 9 lines
Quoted 6 lines · Bogdan Costescu

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

Quoted 4 lines

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. */

Quoted 6 lines

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.

Quoted 4 lines

...
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.

Quoted 3 lines

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.

Quoted 9 lines
Quoted 3 lines

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 [email protected]
Scyld Computing Corporation http://www.scyld.com
410 Severn Ave. Suite 210 Second Generation Beowulf Clusters
Annapolis MD 21403 410-990-9993

#7 lore raw
Quoted 1 line · Donald Becker

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

Thanks a bunch for your comments here.

Quoted 7 lines · Jeff Garzik
Quoted 6 lines
Quoted 5 lines

- 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 ?

Quoted 7 lines
Quoted 4 lines

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.

Quoted 2 lines

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 ;-)

Quoted 6 lines

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..

Quoted 25 lines
Quoted 6 lines

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.

Quoted 4 lines

...
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.

Quoted 4 lines
Quoted 3 lines

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...

Quoted 8 lines
Quoted 5 lines

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]

Quoted 2 lines

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

[responded above]

Quoted 1 line

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

Signature · 3 lines
Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |
#8 lore raw
Quoted 8 lines · Jeff Garzik
Quoted 2 lines

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.

Quoted 3 lines

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 ?).

Quoted 5 lines

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.

Quoted 4 lines

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: [email protected]

#9 lore raw

PATCH: ethtool MII helpers (vers 2)

In reply to Bogdan Costescu

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.

Signature · 3 lines
Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |

Index: linux_2_4/include/linux/mii.h

include/linux/mii.h renamed from include/linux/mii.h:1.1.1.1 +27 -0
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
@@ -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

include/linux/ethtool.h renamed from include/linux/ethtool.h:1.1.1.4 +5 -2
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
@@ -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 ([email protected])
@@ -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

drivers/net/mii.c new file +228 -0

228 lines not on this page · raw message

Index: linux_2_4/drivers/net/epic100.c

drivers/net/epic100.c renamed from drivers/net/epic100.c:1.1.1.35 +64 -18

123 lines not on this page · raw message

#10 lore raw
In reply to Jeff Garzik

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

#11 lore raw
In reply to Jeff Garzik
Quoted 2 lines · Chris Wedgwood

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.

Signature · 3 lines
Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |
#12 lore raw
In reply to Jeff Garzik

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

#13 lore raw
In reply to Jeff Garzik
Quoted 7 lines · 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?

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

Quoted 6 lines

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

Signature · 3 lines
Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |