[PATCH] pcnet32 cable status check

[PATCH] pcnet32 cable status check

[Kent Yoder]

  Hi,

Name: PCnet32 cable status patch
Author: Kent Yoder
Status: Tested on 2.4.20-pre8

D: A patch for the pcnet32 driver to check the status of the 
D: cable via a watchdog timer as the e100/e1000 drivers do.

Comments and criticism are welcome, as I've gotten no response from the 
maintainer. 

Thanks,
Kent


--- linux-2.4.19.vanilla/drivers/net/pcnet32.c	Fri Aug  2 19:39:44 2002
+++ linux-2.4.19/drivers/net/pcnet32.c	Mon Sep 30 10:59:27 2002
@@ -22,8 +22,8 @@
  *************************************************************************/
 
 #define DRV_NAME	"pcnet32"
-#define DRV_VERSION	"1.27a"
-#define DRV_RELDATE	"10.02.2002"
+#define DRV_VERSION	"1.27b"
+#define DRV_RELDATE	"30.09.2002"
 #define PFX		DRV_NAME ": "
 
 static const char *version =
@@ -212,6 +212,8 @@
  *	   fix pci probe not increment cards_found
  *	   FD auto negotiate error workaround for xSeries250
  *	   clean up and using new mii module
+ * v1.27b  Sep 30 2002 Kent Yoder <yoder1@us.ibm.com>
+ * 	   Added timer for cable connection state changes.
  */
 
 
@@ -316,9 +318,11 @@
     int	shared_irq:1,			/* shared irq possible */
 	ltint:1,			/* enable TxDone-intr inhibitor */
 	dxsuflo:1,			/* disable transmit stop on uflo */
-	mii:1;				/* mii port available */
+	mii:1,				/* mii port available */
+	link:1;				/* cable link up/down */
     struct net_device	*next;
     struct mii_if_info mii_if;
+    struct timer_list	watchdog_timer;
 };
 
 static void pcnet32_probe_vlbus(void);
@@ -334,6 +338,7 @@
 static struct net_device_stats *pcnet32_get_stats(struct net_device *);
 static void pcnet32_set_multicast_list(struct net_device *);
 static int  pcnet32_ioctl(struct net_device *, struct ifreq *, int);
+static void pcnet32_watchdog(struct net_device *);
 static int mdio_read(struct net_device *dev, int phy_id, int reg_num);
 static void mdio_write(struct net_device *dev, int phy_id, int reg_num, int val);
 
@@ -778,6 +783,13 @@
 	}
     }
 
+    /* Set the mii phy_id so that we can query the link state */
+    if (lp->mii)
+	lp->mii_if.phy_id = ((lp->a.read_bcr (ioaddr, 33)) >> 5) & 0x1f;
+
+    init_timer(&lp->watchdog_timer);
+    lp->watchdog_timer.data = (unsigned long) dev;
+    lp->watchdog_timer.function = (void *) &pcnet32_watchdog;
     
     /* The PCNET32-specific entries in the device structure. */
     dev->open = &pcnet32_open;
@@ -902,6 +914,19 @@
 
     netif_start_queue(dev);
 
+    /* If we have mii, print the link status and start the watchdog */
+    if (lp->mii) {
+	if(mii_link_ok (&lp->mii_if)) {
+	    printk (KERN_INFO PFX "%s: Cable link is up.\n", dev->name);
+	    lp->link = 1;
+	} else {
+	    printk (KERN_INFO PFX "%s: Cable not connected.\n", dev->name);
+	    lp->link = 0;
+	}
+
+        mod_timer(&(lp->watchdog_timer), jiffies + (2 * HZ));
+    }
+    
     i = 0;
     while (i++ < 100)
 	if (lp->a.read_csr (ioaddr, 0) & 0x0100)
@@ -1372,6 +1397,8 @@
     struct pcnet32_private *lp = dev->priv;
     int i;
 
+    del_timer_sync(&lp->watchdog_timer);
+
     netif_stop_queue(dev);
 
     lp->stats.rx_missed_errors = lp->a.read_csr (ioaddr, 112);
@@ -1650,6 +1677,22 @@
 	}
     }
     return -EOPNOTSUPP;
+}
+
+static void pcnet32_watchdog(struct net_device *dev)
+{
+    struct pcnet32_private *lp = dev->priv;
+
+    if(!netif_running(dev))
+	return;
+
+    if(mii_link_ok(&lp->mii_if) != (lp->link & 1)) {
+	printk(KERN_INFO PFX "%s: Cable link is %s.\n", 
+		dev->name, (lp->link ? "down" : "up"));
+	lp->link ^= 1;
+    }
+    
+    mod_timer(&(lp->watchdog_timer), jiffies + (2 * HZ));
 }
 
 static struct pci_driver pcnet32_driver = {

Re: [PATCH] pcnet32 cable status check

[Jeff Garzik]

Comments:

It looks good as a starting point :)

I just added mii_check_media() to drivers/net/mii.c.  It's in the latest 
2.5.x snapshot, 
ftp://ftp.kernel.org/pub/linux/kernel/v2.5/snapshots/patch-2.5.39-bk2.bz2

and is in Marcelo's inbox as well.  For simple implementations (and I 
think pcnet32 qualifies), the timer should not need to do anything 
beyond calling mii_check_media().  One important feature of this is use 
of the standard netif_carrier_{off,on} to indicate link to the system. 
netif_carrier_xxx also means you don't need lp->link...

Oops on 2.5.39 was Re: [PATCH] pcnet32 cable status check

[Kent Yoder]

>I just added mii_check_media() to drivers/net/mii.c.  It's in the latest 
>2.5.x snapshot, 
>ftp://ftp.kernel.org/pub/linux/kernel/v2.5/snapshots/patch-2.5.39-bk2.bz2

  That looks like just what the doctor ordered, but unfortunately it looks 
like 2.5.39-bk2 is oopsing on boot on this machine.  The boot freezes 
and at the top of the screen I can see what looks like the tail end of an 
oops (this is hand typed):

--------
wait_for_completion+0x12a/0x1e0
default_wake_function+0x0/0x40
try_to_wake_up+0x332/0x340
default_wake_function+0x0/0x40
set_cpus_allowed+0x22f/0x250
ksoftirqd+0x5b/0x110
ksoftirqd+0x0/0x110
kernel_thread_helper+0x5/0xc

[.. Linux NET starting.. other ACPI info ..]

ACPI Namespace successfully loaded at root c053f4bc

<freeze>

Kent

Re: Oops on 2.5.39 was Re: [PATCH] pcnet32 cable status check

[Jeff Garzik]

Kent Yoder wrote:
>>I just added mii_check_media() to drivers/net/mii.c.  It's in the latest 
>>2.5.x snapshot, 
>>ftp://ftp.kernel.org/pub/linux/kernel/v2.5/snapshots/patch-2.5.39-bk2.bz2
> 
> 
>   That looks like just what the doctor ordered, but unfortunately it looks 
> like 2.5.39-bk2 is oopsing on boot on this machine.

for the purposes of testing, you can copy drivers/net/mii.c and 
include/linux/mii.h directly into 2.4.x at those same locations.

Re: [PATCH] pcnet32 cable status check

[Kent Yoder]

  Hi,

  Here's the updated version, now dependent on Jeff's new mii code.  This is 
a bit more modular as well and new functionality can be added inside the 
watchdog function without anything depending on mii.

Thanks,
Kent

--- linux-2.5.39.vanilla/drivers/net/pcnet32.c	Sat Sep 21 23:25:05 2002
+++ linux-2.5.39/drivers/net/pcnet32.c	Tue Oct  1 11:21:13 2002
@@ -22,8 +22,8 @@
  *************************************************************************/
 
 #define DRV_NAME	"pcnet32"
-#define DRV_VERSION	"1.27a"
-#define DRV_RELDATE	"10.02.2002"
+#define DRV_VERSION	"1.27b"
+#define DRV_RELDATE	"01.10.2002"
 #define PFX		DRV_NAME ": "
 
 static const char *version =
@@ -96,6 +96,8 @@
 
 #define PCNET32_DMA_MASK 0xffffffff
 
+#define PCNET32_WATCHDOG_TIMEOUT (jiffies + (2 * HZ))
+
 /*
  * table to translate option values from tulip
  * to internal options
@@ -211,6 +213,8 @@
  *	   fix pci probe not increment cards_found
  *	   FD auto negotiate error workaround for xSeries250
  *	   clean up and using new mii module
+ * v1.27b  Sep 30 2002 Kent Yoder <yoder1@us.ibm.com>
+ * 	   Added timer for cable connection state changes.
  */
 
 
@@ -318,6 +322,7 @@
 	mii:1;				/* mii port available */
     struct net_device	*next;
     struct mii_if_info mii_if;
+    struct timer_list	watchdog_timer;
 };
 
 static void pcnet32_probe_vlbus(void);
@@ -333,6 +338,7 @@
 static struct net_device_stats *pcnet32_get_stats(struct net_device *);
 static void pcnet32_set_multicast_list(struct net_device *);
 static int  pcnet32_ioctl(struct net_device *, struct ifreq *, int);
+static void pcnet32_watchdog(struct net_device *);
 static int mdio_read(struct net_device *dev, int phy_id, int reg_num);
 static void mdio_write(struct net_device *dev, int phy_id, int reg_num, int val);
 
@@ -777,6 +783,13 @@
 	}
     }
 
+    /* Set the mii phy_id so that we can query the link state */
+    if (lp->mii)
+	lp->mii_if.phy_id = ((lp->a.read_bcr (ioaddr, 33)) >> 5) & 0x1f;
+
+    init_timer (&lp->watchdog_timer);
+    lp->watchdog_timer.data = (unsigned long) dev;
+    lp->watchdog_timer.function = (void *) &pcnet32_watchdog;
     
     /* The PCNET32-specific entries in the device structure. */
     dev->open = &pcnet32_open;
@@ -901,6 +914,12 @@
 
     netif_start_queue(dev);
 
+    /* If we have mii, print the link status and start the watchdog */
+    if (lp->mii) {
+	mii_check_media (&lp->mii_if, 1, 1);
+	mod_timer (&(lp->watchdog_timer), PCNET32_WATCHDOG_TIMEOUT);
+    }
+    
     i = 0;
     while (i++ < 100)
 	if (lp->a.read_csr (ioaddr, 0) & 0x0100)
@@ -1371,6 +1390,8 @@
     struct pcnet32_private *lp = dev->priv;
     int i;
 
+    del_timer_sync(&lp->watchdog_timer);
+
     netif_stop_queue(dev);
 
     lp->stats.rx_missed_errors = lp->a.read_csr (ioaddr, 112);
@@ -1651,6 +1672,17 @@
     return -EOPNOTSUPP;
 }
 
+static void pcnet32_watchdog(struct net_device *dev)
+{
+    struct pcnet32_private *lp = dev->priv;
+
+    /* Print the link status if it has changed */
+    if (lp->mii)
+	mii_check_media (&lp->mii_if, 1, 0);
+
+    mod_timer (&(lp->watchdog_timer), PCNET32_WATCHDOG_TIMEOUT);
+}
+
 static struct pci_driver pcnet32_driver = {
     name:	DRV_NAME,
     probe:	pcnet32_probe_pci,

Re: [PATCH] pcnet32 cable status check

[Felipe W Damasio]

----- Original Message -----
From: "Kent Yoder" <key@austin.ibm.com>
To: "Jeff Garzik" <jgarzik@pobox.com>
Cc: <linux-kernel@vger.kernel.org>; <tsbogend@alpha.franken.de>
Sent: Tuesday, October 01, 2002 1:34 PM
Subject: Re: [PATCH] pcnet32 cable status check


>
>   Hi,
>
>   Here's the updated version, now dependent on Jeff's new mii code.  This
is
> a bit more modular as well and new functionality can be added inside the
> watchdog function without anything depending on mii.

    You should use netif_carrier_{on|off} to notify the upper layer of a
link change/loss (until the otherwise is true). Check the 8139cp driver.

    Also, you shouldn't need the timer stuff to keep track of link change.
Just the mii_check_media and netif_carrier_{on|off} and you should be fine.

Felipe

Re: [PATCH] pcnet32 cable status check

[Jeff Garzik]

Felipe W Damasio wrote:
>     Also, you shouldn't need the timer stuff to keep track of link change.
> Just the mii_check_media and netif_carrier_{on|off} and you should be fine.

That depends on the hardware, specifically the presence of a reliable 
link interrupt....

Re: [PATCH] pcnet32 cable status check

[Kent Yoder]

>Felipe W Damasio wrote:
>>     Also, you shouldn't need the timer stuff to keep track of link change.
>> Just the mii_check_media and netif_carrier_{on|off} and you should be fine.
>
>That depends on the hardware, specifically the presence of a reliable 
>link interrupt....

  Yeah, there's no interrupt generated by the cable being taken out on the 
card I am testing with, a PCnet/FAST III 79C975. I am looking around for 
another in the series though..

Kent

Re: [PATCH] pcnet32 cable status check

[Jeff Garzik]

Kent Yoder wrote:
> +static void pcnet32_watchdog(struct net_device *dev)
> +{
> +    struct pcnet32_private *lp = dev->priv;
> +
> +    /* Print the link status if it has changed */
> +    if (lp->mii)
> +	mii_check_media (&lp->mii_if, 1, 0);
> +
> +    mod_timer (&(lp->watchdog_timer), PCNET32_WATCHDOG_TIMEOUT);
> +}


Looks good ;-)

One small thing -- since you appear to test all cases for (lp->mii) 
before calling mod_timer, I don't think you need to test lp->mii inside 
the timer...

As Felipe mentioned, using the link interrupt instead of a timer is 
preferred -- but my own preference would be to apply your patch with the 
small remove-lp->mii-check fixup, and then investigate the support of 
link interrupts.  The reasoning is that, pcnet32 covers a ton of chips, 
and not all may support a link interrupt.

	Jeff

Re: [PATCH] pcnet32 cable status check

[Kent Yoder]

Thus Spake Jeff Garzik:
>
>Looks good ;-)

 Thanks,

>
>One small thing -- since you appear to test all cases for (lp->mii) 
>before calling mod_timer, I don't think you need to test lp->mii inside 
>the timer...

  Well, the reason I left that in there was so that another person could add 
functionality to the watchdog if they wanted on a non-mii enabled card 
without having to know that the check would need to be added. If that's not 
that big a deal, I can remove it...

>As Felipe mentioned, using the link interrupt instead of a timer is 
>preferred -- but my own preference would be to apply your patch with the 
>small remove-lp->mii-check fixup, and then investigate the support of 
>link interrupts.  The reasoning is that, pcnet32 covers a ton of chips, 
>and not all may support a link interrupt.
>
>	Jeff
>
>
>

Re: [PATCH] pcnet32 cable status check

[Jeff Garzik]

Kent Yoder wrote:
> Thus Spake Jeff Garzik:
> 
>>Looks good ;-)
> 
> 
>  Thanks,
> 
> 
>>One small thing -- since you appear to test all cases for (lp->mii) 
>>before calling mod_timer, I don't think you need to test lp->mii inside 
>>the timer...
> 
> 
>   Well, the reason I left that in there was so that another person could add 
> functionality to the watchdog if they wanted on a non-mii enabled card 
> without having to know that the check would need to be added. If that's not 
> that big a deal, I can remove it...


That's a fair enough argument, patch applied.

Re: [PATCH] pcnet32 cable status check

[Felipe W Damasio]

----- Original Message -----
From: "Jeff Garzik" <jgarzik@pobox.com>
To: "Kent Yoder" <key@austin.ibm.com>
Cc: <linux-kernel@vger.kernel.org>; <tsbogend@alpha.franken.de>
Sent: Tuesday, October 01, 2002 2:19 PM
Subject: Re: [PATCH] pcnet32 cable status check


> Kent Yoder wrote:
> > +static void pcnet32_watchdog(struct net_device *dev)
> > +{
> > +    struct pcnet32_private *lp = dev->priv;
> > +
> > +    /* Print the link status if it has changed */
> > +    if (lp->mii)
> > + mii_check_media (&lp->mii_if, 1, 0);
> > +
> > +    mod_timer (&(lp->watchdog_timer), PCNET32_WATCHDOG_TIMEOUT);
> > +}
>
>
> Looks good ;-)
>
> One small thing -- since you appear to test all cases for (lp->mii)
> before calling mod_timer, I don't think you need to test lp->mii inside
> the timer...
>
> As Felipe mentioned, using the link interrupt instead of a timer is
> preferred -- but my own preference would be to apply your patch with the
> small remove-lp->mii-check fixup, and then investigate the support of
> link interrupts.  The reasoning is that, pcnet32 covers a ton of chips,
> and not all may support a link interrupt.

    Sounds nice.

    When identified which chips support Link Change, a bit indicating so
could be add to the capabilities of the NIC so we could choose link
interrupt instead of timer when appropriate.

    I'll try and investigate some chips soon and let you know.

Felipe

Re: [PATCH] pcnet32 cable status check

[Kent Yoder]

>
>That's a fair enough argument, patch applied.
>

  Excellent, thanks.  Are the mii changes planned to be included in a 2.4 
tree at some time?

Kent

Re: [PATCH] pcnet32 cable status check

[Kent Yoder]

Thus Spake Jeff Garzik:
>As Felipe mentioned, using the link interrupt instead of a timer is 
>preferred -- but my own preference would be to apply your patch with the 
>small remove-lp->mii-check fixup, and then investigate the support of 
>link interrupts.  The reasoning is that, pcnet32 covers a ton of chips, 
>and not all may support a link interrupt.

  Jeff, I came across a PCNET FAST/79C971, which apparently also does not
give an interrupt when the cable is pulled.  (My original tested card was a
PCnet/FAST III 79C975).

Kent

Re: [PATCH] pcnet32 cable status check

[Jeff Garzik]

thanks, applied to 2.4 and 2.5