bitrot in drivers/net/au1000_eth.c

bitrot in drivers/net/au1000_eth.c

[Ulrich Eckhardt]

Hi!

I've been debugging a problem in above mentioned file and found several cases 
of redundant, unused or even buggy code in the handling of the MII there. 
Also, there is a comment that suggests that I'm not the only one: 
 * FIXME
 * All of the PHY code really should be detached from the MAC
 * code.

An important point there is that much of the code is in fact not even specific 
to the au1x00 ethernet adapters. I found driver code for the MII I wanted to 
drive in sis900.c, and it looked almost similar to the code in au1x00.c. 
Simply adding the device/vendor IDs to a map and choosing the first of the 
drivers there got my ethernet running.

Now, question is how to proceed. There are basically three ways I would go:
1. Leave it like it is, because someone else is working on it. I'd just post a 
mini-patch that binds my device to an existing driver.
2. Remove the dead/unused parts from au1x00.c, try to restructure and document 
the code so it is easier to maintain in the future.
3. Split off the MII handling code or, better, reuse the facility already 
provided by drivers/net/mii.c. This would mean a significant rewrite of 
au1x00.c, including probably breaking things on the way.

Uli

Re: bitrot in drivers/net/au1000_eth.c

[Pete Popov]

Ulrich Eckhardt wrote:
> Hi!
> 
> I've been debugging a problem in above mentioned file and found several cases 
> of redundant, unused or even buggy code in the handling of the MII there. 
> Also, there is a comment that suggests that I'm not the only one: 
>  * FIXME
>  * All of the PHY code really should be detached from the MAC
>  * code.

Yes, I put that in over four years ago. There was no mii code at 
all, that I could find, at that time. I wanted to separate the mii 
code out of the mac driver but it never happened.

> An important point there is that much of the code is in fact not even specific 
> to the au1x00 ethernet adapters. I found driver code for the MII I wanted to 
> drive in sis900.c, and it looked almost similar to the code in au1x00.c. 
> Simply adding the device/vendor IDs to a map and choosing the first of the 
> drivers there got my ethernet running.
> 
> Now, question is how to proceed. There are basically three ways I would go:
> 1. Leave it like it is, because someone else is working on it. I'd just post a 
> mini-patch that binds my device to an existing driver.

For 2.6, I was told someone is working on something ...

> 2. Remove the dead/unused parts from au1x00.c, try to restructure and document 
> the code so it is easier to maintain in the future.
> 3. Split off the MII handling code or, better, reuse the facility already 
> provided by drivers/net/mii.c. This would mean a significant rewrite of 
> au1x00.c, including probably breaking things on the way.

That's a possibility too but more code needs to be added to mii.c. I 
actually revisited the code yesterday and was trying to figure out 
how to clean it up. But someone told me that there is 2.6 work in 
progress to do this so I decided to just wait. Maybe someone knows 
more about it.

Pete

Re: bitrot in drivers/net/au1000_eth.c

[Matt Porter]

On Fri, Jan 28, 2005 at 09:01:36AM -0800, Pete Popov wrote:
> Ulrich Eckhardt wrote:

<snip>

> > 3. Split off the MII handling code or, better, reuse the facility already 
> > provided by drivers/net/mii.c. This would mean a significant rewrite of 
> > au1x00.c, including probably breaking things on the way.
> 
> That's a possibility too but more code needs to be added to mii.c. I 
> actually revisited the code yesterday and was trying to figure out 
> how to clean it up. But someone told me that there is 2.6 work in 
> progress to do this so I decided to just wait. Maybe someone knows 
> more about it.

I suggest everyone take a look at the effort posted to netdev:

http://oss.sgi.com/archives/netdev/2004-12/msg00643.html

It's an attempt at a phy abstraction layer that goes the next
logical step after the minimal support provided in mii.h.

It's evolved out of the in-driver abstraction that is currently
used in the sungem, ibm_emac, and gianfar drivers in 2.6. It
was just a matter of time before somebody got tired of copying
the same PHY mgmt bits into every driver. :)

-Matt

Re: bitrot in drivers/net/au1000_eth.c

[Pete Popov]

> I suggest everyone take a look at the effort posted to netdev:
> 
> http://oss.sgi.com/archives/netdev/2004-12/msg00643.html

That's the work Dan told me about. Now we just have to update the 
au1x driver :)

Pete


> It's an attempt at a phy abstraction layer that goes the next
> logical step after the minimal support provided in mii.h.
> 
> It's evolved out of the in-driver abstraction that is currently
> used in the sungem, ibm_emac, and gianfar drivers in 2.6. It
> was just a matter of time before somebody got tired of copying
> the same PHY mgmt bits into every driver. :)
> 
> -Matt
> 
> 

Re: bitrot in drivers/net/au1000_eth.c

[Ulrich Eckhardt]

On Friday 28 January 2005 18:20, Matt Porter wrote:
> I suggest everyone take a look at the effort posted to netdev:
>
> http://oss.sgi.com/archives/netdev/2004-12/msg00643.html
>
> It's an attempt at a phy abstraction layer that goes the next
> logical step after the minimal support provided in mii.h.

Ok, this is a major enhancement to the current ad-hoc MII handling and 
probably the way to go in the future. My main concern is if/when this patch 
will be applied to the kernel, but until then I'll probably stick with the 
current code, keeping it alive as good as possible.

Is anyone aware of the acceptance/state of that patch or have further info?

Uli

Re: bitrot in drivers/net/au1000_eth.c

[Ulrich Eckhardt]

I received a private reply from Herbert Valerio Riedel with a patch that falls 
into category

> 3. Split off the MII handling code or, better, reuse the facility already
> provided by drivers/net/mii.c. 

with the permission to forward the patch here. Here comes the code and some 
comments by him...

>>>>>>>>>>>>>>>>>>>>>>>>>>>>>code>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

there's a nasty bug in the code, which triggers a kernel freeze when
performing MII ioctl's w/ mii-tool on a device which is down;

the following code fixes that, and uses the generic mii ioctl helper in
order to use proven code...

hope it's welcomed or useful :-)


Index: drivers/net/au1000_eth.c
===================================================================
--- drivers/net/au1000_eth.c (revision 25)
+++ drivers/net/au1000_eth.c (working copy)
@@ -91,7 +91,7 @@
 static void au1000_timer(unsigned long);
 static int au1000_ioctl(struct net_device *, struct ifreq *, int);
 static int mdio_read(struct net_device *, int, int);
-static void mdio_write(struct net_device *, int, int, u16);
+static void mdio_write(struct net_device *, int, int, int);
 static void dump_mii(struct net_device *dev, int phy_id);
 
 // externs
@@ -846,7 +846,7 @@
  return (int)*mii_data_reg;
 }
 
-static void mdio_write(struct net_device *dev, int phy_id, int reg, u16 
value)
+static void mdio_write(struct net_device *dev, int phy_id, int reg, int 
value)
 {
  struct au1000_private *aup = (struct au1000_private *) dev->priv;
  volatile u32 *mii_control_reg;
@@ -951,6 +951,10 @@
     aup->phy_ops = mii_chip_table[i].phy_ops;
     aup->phy_ops->phy_init(dev,phy_addr);
 
+    aup->mii_if.phy_id = phy_addr;
+    aup->mii_if.phy_id_mask = 0x1f;    
+    aup->mii_if.reg_num_mask = 0x1f;
+
     // Check for dual-phy and then store required 
     // values and set indicators. We need to do 
     // this now since mdio_{read,write} need the 
@@ -1544,6 +1548,10 @@
  aup->mii->mii_control_reg = 0;
  aup->mii->mii_data_reg = 0;
 
+        aup->mii_if.dev = dev;
+        aup->mii_if.mdio_read = mdio_read;
+        aup->mii_if.mdio_write = mdio_write;
+
  if (mii_probe(dev) != 0) {
   goto err_out;
  }
@@ -1592,6 +1600,7 @@
  dev->tx_timeout = au1000_tx_timeout;
  dev->watchdog_timeo = ETH_TX_TIMEOUT;
 
+
  /* 
   * The boot code uses the ethernet controller, so reset it to start 
   * fresh.  au1000_init() expects that the device is in reset state.
@@ -2127,24 +2136,17 @@
 
 static int au1000_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 {
- struct au1000_private *aup = (struct au1000_private *)dev->priv;
- u16 *data = (u16 *)&rq->ifr_ifru;
+ struct au1000_private *const aup = netdev_priv(dev);
+ int rc;
+ unsigned long flags;
 
- switch(cmd) { 
- case SIOCGMIIPHY:
-  data[0] = aup->phy_addr;
-  /* Fall through */
- case SIOCGMIIREG:
-  data[3] = mdio_read(dev, aup->phy_addr, data[1]);
-  return 0;
- case SIOCSMIIREG:
-  if (!capable(CAP_NET_ADMIN))
-   return -EPERM;
-  mdio_write(dev, aup->phy_addr, data[1], data[2]);
-  return 0;
- default:
-  return -EOPNOTSUPP;
- }
+ if (!netif_running(dev))
+  return -EINVAL;
+
+ spin_lock_irqsave(&aup->lock, flags);
+        rc = generic_mii_ioctl(&aup->mii_if, if_mii(rq), cmd, NULL);
+        spin_unlock_irqrestore(&aup->lock, flags);
+        return rc;
 }
 
 
Index: drivers/net/au1000_eth.h
===================================================================
--- drivers/net/au1000_eth.h (revision 25)
+++ drivers/net/au1000_eth.h (working copy)
@@ -214,6 +214,7 @@
  int mac_id;
  mii_phy_t *mii;
  struct phy_ops *phy_ops;
+ struct mii_if_info mii_if;
  
  /* These variables are just for quick access to certain regs addresses. */
  volatile mac_reg_t *mac;  /* mac registers                      */   
Index: drivers/net/Kconfig
===================================================================
--- drivers/net/Kconfig (revision 25)
+++ drivers/net/Kconfig (working copy)
@@ -467,6 +467,7 @@
  bool "MIPS AU1000 Ethernet support"
  depends on NET_ETHERNET && SOC_AU1X00
  select CRC32
+ select MII
  help
    If you have an Alchemy Semi AU1X00 based system
    say Y.  Otherwise, say N.