RE: [PATCH 08/26] e1000: Deprecate mii-tool SIOCMIIREG ioctl

RE: [PATCH 08/26] e1000: Deprecate mii-tool SIOCMIIREG ioctl

[Brandeburg, Jesse]

Jeff Garzik wrote:
> Kok, Auke wrote:
>> Deprecate mii-tool SIOCMIIREG ioctl. This ioctl is broken in e1000
>> and ethtool has this functionality in working order.
>> 
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
> 
> This doesn't "deprecated" anything, it changes the ABI on a
> user's machine by _removing_ ioctl handling code that used to be
> there. 
> 
> What's broken, and what is needed to fix?  Perhaps e1000
> should instead be limited to 10/100 networks/speeds?

mii-tool is broken because it doesn't understand GMII phys.  We ended up
having a mess of code trying to implement the shim layer to support the
PHY write ioctls, among others.
mii-tool always reports 100 Mbps link when you actually have 1Gbps link.
Using mii-tool to set options didn't work very well to begin with, so
rather than continue to dump time into a dead end, just remove the
incomplete/non-working code and force the user to use ethtool if they
want to do set operations. 

Mii-tool is already marked in its man page to be obsolete

<quote from RHEL4>
NOTE
       This program is obsolete. Valid  media  are  only  100baseT4,
100baseTx-FD,100baseTx-HD,  10baseT-FD  and
       10baseT-HD ethernet cards. For replacement check eth-tool.
</quote>

We intentionally left the read code in to not break any user space
programs that are using that IOCTL to read link status etc.  With some
recent improvements to ethtool, it has the exact same functionality as
mii-tool.

While I appreciate the "don't break user space" position, in this case
it never really worked right, and when it did work the user was just
lucky because they probably weren't getting what they wanted.

Jesse

Re: [PATCH 08/26] e1000: Deprecate mii-tool SIOCMIIREG ioctl

[Auke Kok]

Brandeburg, Jesse wrote:
> Jeff Garzik wrote:
>> Kok, Auke wrote:
>>> Deprecate mii-tool SIOCMIIREG ioctl. This ioctl is broken in e1000
>>> and ethtool has this functionality in working order.
>>>
>>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>>> Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
>> This doesn't "deprecated" anything, it changes the ABI on a
>> user's machine by _removing_ ioctl handling code that used to be
>> there. 
>>
>> What's broken, and what is needed to fix?  Perhaps e1000
>> should instead be limited to 10/100 networks/speeds?
> 
> mii-tool is broken because it doesn't understand GMII phys.  We ended up
> having a mess of code trying to implement the shim layer to support the
> PHY write ioctls, among others.
> mii-tool always reports 100 Mbps link when you actually have 1Gbps link.
> Using mii-tool to set options didn't work very well to begin with, so
> rather than continue to dump time into a dead end, just remove the
> incomplete/non-working code and force the user to use ethtool if they
> want to do set operations. 
> 
> Mii-tool is already marked in its man page to be obsolete
> 
> <quote from RHEL4>
> NOTE
>        This program is obsolete. Valid  media  are  only  100baseT4,
> 100baseTx-FD,100baseTx-HD,  10baseT-FD  and
>        10baseT-HD ethernet cards. For replacement check eth-tool.
> </quote>
> 
> We intentionally left the read code in to not break any user space
> programs that are using that IOCTL to read link status etc.  With some
> recent improvements to ethtool, it has the exact same functionality as
> mii-tool.
> 
> While I appreciate the "don't break user space" position, in this case
> it never really worked right, and when it did work the user was just
> lucky because they probably weren't getting what they wanted.

I'm dropping this patch from our stack, it seems (irony) that we might have 
some use for this ioctl after all and we need to re-evaluate what to do with 
it. It doesn't hurt (much) to leave it in there for now.

Jeff, I'll repost a summary of the changes I made to the queue on our gitserver 
in a second.

Cheers,

Auke

[PATCH 00/26] e100, e1000, ixgb updates

[Kok, Auke]

Hi,

Here are updates targeted to branch #upstream from netdev-2.6,
including updates to e100, e1000 and ixgb.

These changes are available through git.

git pull git://lost.foo-projects.org/~ahkok/git/netdev-2.6 upstream

and against
 netdev-2.6#upstream 699a71238856b19091503c671bac8abb1e3f9a3a


Summary:

Vasily Averin <vvs@sw.ru>
     e1000: IRQ resources cleanup
     e1000: e1000_probe resources cleanup
     e1000: ring buffers resources cleanup

Jesse Brandeburg <jesse.brandeburg@intel.com>
     e1000: unify WoL capability detection code
     e1000: Add PCI ID 0x10a4 for our new 4-port PCI-Express device
     ixgb: Set a constant blink rate for ixgb adapter identify (1sec on, 1sec off)
     ixgb: Cache-align all TX components of the adapter struct.
     ixgb: Add buffer_info and test like e1000 has.

Christoph Hellwig <hch@lst.de>
     e1000: clean up skb allocation code, patch submitted by Christoph

Jeff Kirsher <jeffrey.t.kirsher@intel.com>
     e1000: Deprecate mii-tool SIOCMIIREG ioctl

Auke Kok <auke-jan.h.kok@intel.com>
     e1000: Whitespace cleanup, cosmetic changes
     e1000: error out if we cannot enable PCI device on resume
     e1000: remove unused part_num reading code
     e1000: Use module param array code
     e1000: Increment driver version to 7.2.7-k2
     e100: Convert e100 to use netdev_alloc_skb().
     e100: reduce time under spinlock
     e100: remove skb->dev assignment
     e100: increment version to 3.5.16-k2
     ixgb: Convert dev_alloc_skb to netdev_alloc_skb.
     ixgb: convert dev->priv to netdev_priv(dev).
     ixgb: recalculate after how many descriptors to wake the queue
     ixgb: remove skb->dev assignment
     ixgb: Increment version to 1.0.112-k2

Linas Vepstas <linas@austin.ibm.com>
     e100: fix error recovery
     ixgb: Add PCI Error recovery callbacks


Cheers,

Auke

---

 drivers/net/e100.c                |   12
 drivers/net/e1000/e1000.h         |    6
 drivers/net/e1000/e1000_ethtool.c |  219 ++++---
 drivers/net/e1000/e1000_hw.c      | 1059 ++++++++++++++++++--------------------
 drivers/net/e1000/e1000_hw.h      |   26
 drivers/net/e1000/e1000_main.c    |  214 +++----
 drivers/net/e1000/e1000_param.c   |  161 +++--
 drivers/net/ixgb/ixgb.h           |    5
 drivers/net/ixgb/ixgb_ethtool.c   |    6
 drivers/net/ixgb/ixgb_main.c      |  140 ++++-
 10 files changed, 981 insertions(+), 867 deletions(-)

---
Auke Kok <auke-jan.h.kok@intel.com>

[PATCH 08/26] e1000: Deprecate mii-tool SIOCMIIREG ioctl

[Kok, Auke]

Deprecate mii-tool SIOCMIIREG ioctl. This ioctl is broken in e1000 and
ethtool has this functionality in working order.

Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
---

 drivers/net/e1000/e1000_main.c |   69 ----------------------------------------
 1 files changed, 0 insertions(+), 69 deletions(-)

diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index c128f62..70d0acd 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -4263,9 +4263,6 @@ e1000_mii_ioctl(struct net_device *netde
 {
 	struct e1000_adapter *adapter = netdev_priv(netdev);
 	struct mii_ioctl_data *data = if_mii(ifr);
-	int retval;
-	uint16_t mii_reg;
-	uint16_t spddplx;
 	unsigned long flags;
 
 	if (adapter->hw.media_type != e1000_media_type_copper)
@@ -4287,72 +4284,6 @@ e1000_mii_ioctl(struct net_device *netde
 		spin_unlock_irqrestore(&adapter->stats_lock, flags);
 		break;
 	case SIOCSMIIREG:
-		if (!capable(CAP_NET_ADMIN))
-			return -EPERM;
-		if (data->reg_num & ~(0x1F))
-			return -EFAULT;
-		mii_reg = data->val_in;
-		spin_lock_irqsave(&adapter->stats_lock, flags);
-		if (e1000_write_phy_reg(&adapter->hw, data->reg_num,
-					mii_reg)) {
-			spin_unlock_irqrestore(&adapter->stats_lock, flags);
-			return -EIO;
-		}
-		if (adapter->hw.media_type == e1000_media_type_copper) {
-			switch (data->reg_num) {
-			case PHY_CTRL:
-				if (mii_reg & MII_CR_POWER_DOWN)
-					break;
-				if (mii_reg & MII_CR_AUTO_NEG_EN) {
-					adapter->hw.autoneg = 1;
-					adapter->hw.autoneg_advertised = 0x2F;
-				} else {
-					if (mii_reg & 0x40)
-						spddplx = SPEED_1000;
-					else if (mii_reg & 0x2000)
-						spddplx = SPEED_100;
-					else
-						spddplx = SPEED_10;
-					spddplx += (mii_reg & 0x100)
-						   ? DUPLEX_FULL :
-						   DUPLEX_HALF;
-					retval = e1000_set_spd_dplx(adapter,
-								    spddplx);
-					if (retval) {
-						spin_unlock_irqrestore(
-							&adapter->stats_lock,
-							flags);
-						return retval;
-					}
-				}
-				if (netif_running(adapter->netdev))
-					e1000_reinit_locked(adapter);
-				else
-					e1000_reset(adapter);
-				break;
-			case M88E1000_PHY_SPEC_CTRL:
-			case M88E1000_EXT_PHY_SPEC_CTRL:
-				if (e1000_phy_reset(&adapter->hw)) {
-					spin_unlock_irqrestore(
-						&adapter->stats_lock, flags);
-					return -EIO;
-				}
-				break;
-			}
-		} else {
-			switch (data->reg_num) {
-			case PHY_CTRL:
-				if (mii_reg & MII_CR_POWER_DOWN)
-					break;
-				if (netif_running(adapter->netdev))
-					e1000_reinit_locked(adapter);
-				else
-					e1000_reset(adapter);
-				break;
-			}
-		}
-		spin_unlock_irqrestore(&adapter->stats_lock, flags);
-		break;
 	default:
 		return -EOPNOTSUPP;
 	}



---
Auke Kok <auke-jan.h.kok@intel.com>

Re: [PATCH 08/26] e1000: Deprecate mii-tool SIOCMIIREG ioctl

[Jeff Garzik]

Kok, Auke wrote:
> Deprecate mii-tool SIOCMIIREG ioctl. This ioctl is broken in e1000 and
> ethtool has this functionality in working order.
> 
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>

This doesn't "deprecated" anything, it changes the ABI on a user's 
machine by _removing_ ioctl handling code that used to be there.

What's broken, and what is needed to fix?  Perhaps e1000 should instead 
be limited to 10/100 networks/speeds?

	Jeff