Linux Kernel Mailing List wrote:
> tree 1771b690cdee80312ace3fe046e29e965a0b30eb
> parent c8d127418d78aaeeb1a417ef7453dc09c9118146
> author Tommy S. Christensen <[email protected]> Wed, 07 Sep 2005 05:17:28 -0700
> committer Linus Torvalds <[email protected]> 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 <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> Signed-off-by: Linus Torvalds <[email protected]>
>
> 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
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: [email protected]
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: [email protected]
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 <[email protected]>
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:
>
> > 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: [email protected]
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.
[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 <[email protected]>
--- 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",
Tommy Christensen <[email protected]> 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:
>
>> 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:
> --- 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
[email protected]
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 <[email protected]> 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.
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: [email protected]
On Fri, Sep 09, 2005 at 12:44:06AM -0700, Andrew Morton wrote:
> Tommy Christensen <[email protected]> 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
***************************************************/
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