Re: [PATCH] 3c59x: read current link status from phy

Linux Kernel Mailing List wrote:

It would be nice if somebody would be motivated to check in 
s/1/MII_BMSR/ to utilize the constant in include/linux/mii.h.

	Jeff

On Wed, 7 Sep 2005, Jeff Garzik wrote:

quoted

The phy status register must be read twice in order to get the actual link
state.

Can the original poster give an explanation ? I've enjoyed a rather 
well functioning 3c59x driver for the past ~6 years without such 
double reading. Plus:
- this operation is I/O expensive
- it is performed inside a region protected by a spinlock
- it is performed often, every 60 seconds

Is there some specific hardware that exhibits a problem that is solved 
by this double reading ?

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

On Thu, 2005-09-08 at 13:58, Bogdan Costescu wrote:

Can the original poster give an explanation ? I've enjoyed a rather 
well functioning 3c59x driver for the past ~6 years without such 
double reading. Plus:
- this operation is I/O expensive
- it is performed inside a region protected by a spinlock
- it is performed often, every 60 seconds

Is there some specific hardware that exhibits a problem that is solved 
by this double reading ?

Nothing critical. The idea is to avoid an extra delay of 60 seconds
before detecting link-up.

Please see http://bugzilla.kernel.org/show_bug.cgi?id=5025


-Tommy

On Thu, 8 Sep 2005, Tommy Christensen wrote:

The idea is to avoid an extra delay of 60 seconds before detecting 
link-up.

But you are adding the read to a function that is called repeatedly to 
fix an event that happens only once at start-up !

If this read is really needed (I still doubt it...), can't it be 
performed in vortex_up(), by possibly doubling the existing one there ?
vortex_up() is executed only once at start-up, not every 60 seconds.

Please see http://bugzilla.kernel.org/show_bug.cgi?id=5025

Hah, a Cisco switch.  Look in Documentation/networking/vortex.txt for 
"portfast".

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

On Wed, Sep 07, 2005 at 09:49:13PM -0400, Jeff Garzik wrote:

<snip> 
It would be nice if somebody would be motivated to check in 
s/1/MII_BMSR/ to utilize the constant in include/linux/mii.h.

	Jeff

As requested, a patch to cleanup mdio_read routines in 3c59x.c to use the MII_*
macros defined in include/linux/mii.h

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

 3c59x.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

--- linux-2.6/drivers/net/3c59x.c.orig	2005-09-08 08:24:35.000000000 -0400
+++ linux-2.6/drivers/net/3c59x.c	2005-09-08 08:27:57.000000000 -0400

@@ -1439,7 +1439,7 @@ static int __devinit vortex_probe1(struc
 		if (vp->drv_flags & EXTRA_PREAMBLE)
 			mii_preamble_required++;
 		mdio_sync(ioaddr, 32);
-		mdio_read(dev, 24, 1);
+		mdio_read(dev, 24, MII_BMSR);
 		for (phy = 0; phy < 32 && phy_idx < 1; phy++) {
 			int mii_status, phyx;
 

@@ -1453,7 +1453,7 @@ static int __devinit vortex_probe1(struc
 				phyx = phy - 1;
 			else
 				phyx = phy;
-			mii_status = mdio_read(dev, phyx, 1);
+			mii_status = mdio_read(dev, phyx, MII_BMSR);
 			if (mii_status  &&  mii_status != 0xffff) {
 				vp->phys[phy_idx++] = phyx;
 				if (print_info) {

@@ -1469,7 +1469,7 @@ static int __devinit vortex_probe1(struc
 			printk(KERN_WARNING"  ***WARNING*** No MII transceivers found!\n");
 			vp->phys[0] = 24;
 		} else {
-			vp->advertising = mdio_read(dev, vp->phys[0], 4);
+			vp->advertising = mdio_read(dev, vp->phys[0], MII_ADVERTISE);
 			if (vp->full_duplex) {
 				/* Only advertise the FD media types. */
 				vp->advertising &= ~0x02A0;

@@ -1641,8 +1641,8 @@ vortex_up(struct net_device *dev)
 		int mii_reg1, mii_reg5;
 		EL3WINDOW(4);
 		/* Read BMSR (reg1) only to clear old status. */
-		mii_reg1 = mdio_read(dev, vp->phys[0], 1);
-		mii_reg5 = mdio_read(dev, vp->phys[0], 5);
+		mii_reg1 = mdio_read(dev, vp->phys[0], MII_BMSR);
+		mii_reg5 = mdio_read(dev, vp->phys[0], MII_LPA);
 		if (mii_reg5 == 0xffff  ||  mii_reg5 == 0x0000) {
 			netif_carrier_off(dev); /* No MII device or no link partner report */
 		} else {

@@ -1872,13 +1872,13 @@ vortex_timer(unsigned long data)
 	case XCVR_MII: case XCVR_NWAY:
 		{
 			spin_lock_bh(&vp->lock);
-			mii_status = mdio_read(dev, vp->phys[0], 1);
+			mii_status = mdio_read(dev, vp->phys[0], MII_BMSR);
 			ok = 1;
 			if (vortex_debug > 2)
 				printk(KERN_DEBUG "%s: MII transceiver has status %4.4x.\n",
 					dev->name, mii_status);
 			if (mii_status & BMSR_LSTATUS) {
-				int mii_reg5 = mdio_read(dev, vp->phys[0], 5);
+				int mii_reg5 = mdio_read(dev, vp->phys[0], MII_LPA);
 				if (! vp->force_fd  &&  mii_reg5 != 0xffff) {
 					int duplex;
 

-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
 ***************************************************/

On Thu, 2005-09-08 at 15:35, Bogdan Costescu wrote:

On Thu, 8 Sep 2005, Tommy Christensen wrote:

quoted

The idea is to avoid an extra delay of 60 seconds before detecting 
link-up.

But you are adding the read to a function that is called repeatedly to 
fix an event that happens only once at start-up !

Link state can change anytime, not just at start-up.  That's why
it's being repeatedly monitored ;-)

If this read is really needed (I still doubt it...), can't it be 
performed in vortex_up(), by possibly doubling the existing one there ?
vortex_up() is executed only once at start-up, not every 60 seconds.

That won't solve the reported issue, unfortunately.

Besides, how long would you like to wait for network connectivity
after plugging in the cable?  It is now lowered from [60-120] to
[0-60] seconds.

Personally, I'd prefer the delay to be < 10 seconds.


-Tommy

On Thu, 8 Sep 2005, Tommy Christensen wrote:

Besides, how long would you like to wait for network connectivity 
after plugging in the cable?  It is now lowered from [60-120] to 
[0-60] seconds.

I now understood what the problem was, so I'll put it in words for 
posterity: the Link Status bit of the MII Status register needs to be 
read twice to first clear the error state (link bit=0) after which the 
bit reports the actual value of the link. From the manual:

 	This bit has a latching function. A link failure causes the
 	bit to clear and remain clear until it has been read through
 	the management interface.

I tested this on a Tornado chip and it works as advertised (after link 
is back up, first read gives 0x7829, the second 0x782d).

But I still don't agree with your solution: you are reading the Status 
register twice in all cases, which is wrong. What you want is to read 
it a second time only after the link was marked as down: a simple 
check if bit 2 of the Status register is 0, in which case you issue 
the second read. This still means that there will be 2 reads if the 
link remains down, but at least there is only 1 read for the case 
where the link is up and remains up.

Personally, I'd prefer the delay to be < 10 seconds.

If you sample every 60 seconds ? Teach Shannon how to do it ;-)

If you mean to reduce the sampling period, there is a very good reason 
not to do it: these MDIO operations are expensive - it's a serial 
protocol. vortex_timer() might do 2 (and with the discussed change - 
3) of them - there are better things to do for the CPU than wait for 
these I/O operations. Plus, vortex_timer() also disables the 
interrupt...

The Tornado and at least some Cyclone chips support generating an 
interrupt whenever the link changes, which can be used instead of 
polling for link state. This feature is not used in the 3c59x driver 
and could give you much less than 10 seconds accuracy - but you have 
to code it. ;-)

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

On Sep 8, 2005, at 10:42, Bogdan Costescu wrote:

On Thu, 8 Sep 2005, Tommy Christensen wrote:

quoted

Personally, I'd prefer the delay to be < 10 seconds.

If you sample every 60 seconds ? Teach Shannon how to do it ;-)

If you mean to reduce the sampling period, there is a very good  
reason not to do it: these MDIO operations are expensive - it's a  
serial protocol. vortex_timer() might do 2 (and with the discussed  
change - 3) of them - there are better things to do for the CPU  
than wait for these I/O operations. Plus, vortex_timer() also  
disables the interrupt...

The Tornado and at least some Cyclone chips support generating an  
interrupt whenever the link changes, which can be used instead of  
polling for link state. This feature is not used in the 3c59x  
driver and could give you much less than 10 seconds accuracy - but  
you have to code it. ;-)

The new PHY Layer (drivers/net/phy/*) can provide all these features  
for you without much difficulty, I suspect.  The layer supports  
handling the interrupts for you, or (if it's shared with your  
controller's interrupt) provides simple hooks to make supporting  
interrupts easy.

Is the cost of an extra read every minute really too high?  It's such  
a small fraction of the CPU time, and provides a better user experience.

Bogdan Costescu wrote:

I now understood what the problem was, so I'll put it in words for 
posterity: the Link Status bit of the MII Status register needs to be 
read twice to first clear the error state (link bit=0) after which the 
bit reports the actual value of the link. From the manual:

Yes, this is exactly the point.

But I still don't agree with your solution: you are reading the Status 
register twice in all cases, which is wrong. What you want is to read it 
a second time only after the link was marked as down: a simple check if 
bit 2 of the Status register is 0, in which case you issue the second 
read. This still means that there will be 2 reads if the link remains 
down, but at least there is only 1 read for the case where the link is 
up and remains up.

I don't think this makes much of a difference in the big picture, but
you're certainly right: let's not waste more cycles than we have to.

Can we agree on the patch below?


-Tommy

Tommy Christensen [off-list ref] wrote:

In order to spare some I/O operations, be more intelligent about
 when to read from the PHY.

Seems sane.

Should we also decrease the polling interval?  Perhaps only when the cable
is unplugged?

Andrew Morton wrote:

Should we also decrease the polling interval?  Perhaps only when the cable
is unplugged?

Sounds like a plan. 60 seconds certainly strikes me as being very slow.
OTOH, I'm not aware of the reasoning behind this choice in the first place.
It might make sense for some odd setups.

Since I don't even have any HW to play around with, I think I'll step
down for now.


-Tommy

Tommy Christensen wrote:

Andrew Morton wrote:

quoted

Should we also decrease the polling interval?  Perhaps only when the 
cable
is unplugged?

Sounds like a plan. 60 seconds certainly strikes me as being very slow.
OTOH, I'm not aware of the reasoning behind this choice in the first place.
It might make sense for some odd setups.

Since I don't even have any HW to play around with, I think I'll step
down for now.

The standard for Becker drivers is 5 seconds if link is down, and 60 
seconds if link is up, IIRC.

	Jeff

On Fri, Sep 09, 2005 at 12:39:18AM +0200, Tommy Christensen wrote:

Any chance you could re-diff this to apply on top of the patch posted
earlier today by Neil Horman?

Thanks,

John
-- 
John W. Linville
linville@tuxdriver.com

John W. Linville wrote:

Any chance you could re-diff this to apply on top of the patch posted
earlier today by Neil Horman?

Sure, but his patch didn't apply to -git8.

If Neil would please resend, then I can diff against that.


-Tommy

Tommy Christensen [off-list ref] wrote:

John W. Linville wrote:

quoted

Any chance you could re-diff this to apply on top of the patch posted
earlier today by Neil Horman?

Sure, but his patch didn't apply to -git8.

If Neil would please resend, then I can diff against that.

Is OK, I'll sort it all out.

On Thu, 8 Sep 2005, Andy Fleming wrote:

The new PHY Layer (drivers/net/phy/*) can provide all these features 
for you without much difficulty, I suspect.

As pointed to be Andrew a few days ago, this driver supports a lot of 
chips - for most of them the test hardware would be hard to come by 
and the documentation even more. Unless you'd like to do it based on 
"whoever is interested should cry loud"...

The layer supports handling the interrupts for you, or (if it's 
shared with your controller's interrupt)

Yes, there is only one interrupt that for data transmission (both Tx 
and Rx), statistics, errors and (for those chips that support it) link 
state change.

Is the cost of an extra read every minute really too high?

You probably didn't look at the code. The MII registers are not 
exposed in the PCI space, they need to be accessed through a serial 
protocol, such that each MII register read is in fact about 200 (in 
total) of outw and inw/inl operations.

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

On Fri, Sep 09, 2005 at 12:44:06AM -0700, Andrew Morton wrote:

Tommy Christensen [off-list ref] wrote:

quoted

John W. Linville wrote:

quoted

Any chance you could re-diff this to apply on top of the patch posted
earlier today by Neil Horman?

Sure, but his patch didn't apply to -git8.

If Neil would please resend, then I can diff against that.

Is OK, I'll sort it all out.

Thanks all, I appreciate it.
Neil

-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
 ***************************************************/

On Sep 9, 2005, at 05:10, Bogdan Costescu wrote:

On Thu, 8 Sep 2005, Andy Fleming wrote:

quoted

Is the cost of an extra read every minute really too high?

You probably didn't look at the code. The MII registers are not  
exposed in the PCI space, they need to be accessed through a serial  
protocol, such that each MII register read is in fact about 200 (in  
total) of outw and inw/inl operations.

I certainly looked at the code.  I'm aware that there are probably  
about 150 microseconds of work, tops, to do each read.  Do it outside  
of interrupt time, and separate from the normal thread of the driver  
(like a task struct), and it shouldn't take up that much CPU time.   
And if it's being done every minute, it's really not a big deal, is it?

Anyway, it's not a big deal to me.  I agree that doing only one read,  
if the link is reported as up, is a good idea.  I'll be sure to put  
it in the next rev of the PHY Layer.

I also agree that polling should be done every 5 seconds, at least  
when the link is down.

Andy Fleming
Freescale Open Source Team