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

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

[Jeff Garzik]

Linux Kernel Mailing List wrote:
> tree 1771b690cdee80312ace3fe046e29e965a0b30eb
> parent c8d127418d78aaeeb1a417ef7453dc09c9118146
> author Tommy S. Christensen <tommy.christensen@tpack.net> Wed, 07 Sep 2005 05:17:28 -0700
> committer Linus Torvalds <torvalds@g5.osdl.org> Thu, 08 Sep 2005 06:57:30 -0700
> 
> [PATCH] 3c59x: read current link status from phy
> 
> The phy status register must be read twice in order to get the actual link
> state.
> 
> Signed-off-by: Tommy S. Christensen <tommy.christensen@tpack.net>
> Signed-off-by: Andrew Morton <akpm@osdl.org>
> Signed-off-by: Linus Torvalds <torvalds@osdl.org>
> 
>  drivers/net/3c59x.c |    1 +
>  1 files changed, 1 insertion(+)
> 
> diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c
> --- a/drivers/net/3c59x.c
> +++ b/drivers/net/3c59x.c
> @@ -1889,6 +1889,7 @@ vortex_timer(unsigned long data)
>  		{
>  			spin_lock_bh(&vp->lock);
>  			mii_status = mdio_read(dev, vp->phys[0], 1);
> +			mii_status = mdio_read(dev, vp->phys[0], 1);

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

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

[Bogdan Costescu]

On Wed, 7 Sep 2005, Jeff Garzik wrote:

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

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

[Tommy Christensen]

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

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

[Bogdan Costescu]

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

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

[Tommy Christensen]

On Thu, 2005-09-08 at 15:35, Bogdan Costescu wrote:
> 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 !

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

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

[Bogdan Costescu]

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

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

[Andy Fleming]

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

> On Thu, 8 Sep 2005, Tommy Christensen wrote:
>
>> 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.

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

[Bogdan Costescu]

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

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

[Andy Fleming]

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

> On Thu, 8 Sep 2005, Andy Fleming wrote:
>
>
>> 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

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

[Tommy Christensen]

[-- Attachment #1: Type: text/plain, Size: 984 bytes --]

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

[-- Attachment #2: 3c59x-carrier.patch --]
[-- Type: text/plain, Size: 806 bytes --]

[3c59x] Avoid blindly reading link status twice

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

Pointed out by Bogdan Costescu.

Signed-off-by: Tommy S. Christensen <tommy.christensen@tpack.net>


--- linux-2.6.13-git8/drivers/net/3c59x.c-orig	Fri Sep  9 00:05:49 2005
+++ linux-2.6.13-git8/drivers/net/3c59x.c	Fri Sep  9 00:13:55 2005
@@ -1889,7 +1889,9 @@ vortex_timer(unsigned long data)
 		{
 			spin_lock_bh(&vp->lock);
 			mii_status = mdio_read(dev, vp->phys[0], 1);
-			mii_status = mdio_read(dev, vp->phys[0], 1);
+			if (!(mii_status & BMSR_LSTATUS))
+				/* Re-read to get actual link status */
+				mii_status = mdio_read(dev, vp->phys[0], 1);
 			ok = 1;
 			if (vortex_debug > 2)
 				printk(KERN_DEBUG "%s: MII transceiver has status %4.4x.\n",

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

[Andrew Morton]

Tommy Christensen <tommy.christensen@tpack.net> 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?

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

[Tommy Christensen]

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

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

[Jeff Garzik]

Tommy Christensen wrote:
> 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.

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

	Jeff

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

[John W. Linville]

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

> --- linux-2.6.13-git8/drivers/net/3c59x.c-orig	Fri Sep  9 00:05:49 2005
> +++ linux-2.6.13-git8/drivers/net/3c59x.c	Fri Sep  9 00:13:55 2005
> @@ -1889,7 +1889,9 @@ vortex_timer(unsigned long data)
>  		{
>  			spin_lock_bh(&vp->lock);
>  			mii_status = mdio_read(dev, vp->phys[0], 1);
> -			mii_status = mdio_read(dev, vp->phys[0], 1);
> +			if (!(mii_status & BMSR_LSTATUS))
> +				/* Re-read to get actual link status */
> +				mii_status = mdio_read(dev, vp->phys[0], 1);
>  			ok = 1;
>  			if (vortex_debug > 2)
>  				printk(KERN_DEBUG "%s: MII transceiver has status %4.4x.\n",

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

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

[Tommy Christensen]

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

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

[Andrew Morton]

Tommy Christensen <tommy.christensen@tpack.net> wrote:
>
> 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.
> 

Is OK, I'll sort it all out.

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

[Neil Horman]

On Fri, Sep 09, 2005 at 12:44:06AM -0700, Andrew Morton wrote:
> Tommy Christensen <tommy.christensen@tpack.net> wrote:
> >
> > 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.
> > 
> 
> 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
 ***************************************************/

Re: [PATCH] 3c59x: cleanup of mdio_read routines to use MII_* macros in include/linux/mii.h

[Neil Horman]

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