RE: [PATCH] rcu: small rcu_dereference doc update
> -Original Message- > From: Paul E. McKenney [mailto:paul...@linux.vnet.ibm.com] > Sent: Friday, April 17, 2015 11:41 AM > To: Jeff Haran > Cc: Milos Vyletel; Josh Triplett; Steven Rostedt; Mathieu Desnoyers; Lai > Jiangshan; Jonathan Corbet; open list:READ-COPY UPDATE...; open > list:DOCUMENTATION > Subject: Re: [PATCH] rcu: small rcu_dereference doc update > > On Fri, Apr 17, 2015 at 04:53:15PM +, Jeff Haran wrote: > > > -Original Message- > > > From: Paul E. McKenney [mailto:paul...@linux.vnet.ibm.com] > > > Sent: Friday, April 17, 2015 7:07 AM > > > To: Milos Vyletel > > > Cc: Josh Triplett; Steven Rostedt; Mathieu Desnoyers; Lai Jiangshan; > > > Jonathan Corbet; open list:READ-COPY UPDATE...; open > > > list:DOCUMENTATION; Jeff Haran > > > Subject: Re: [PATCH] rcu: small rcu_dereference doc update > > > > > > On Fri, Apr 17, 2015 at 12:33:36PM +0200, Milos Vyletel wrote: > > > > Make a note stating that repeated calls of rcu_dereference() may > > > > not return the same pointer if update happens while in critical section. > > > > > > > > Reported-by: Jeff Haran > > > > Signed-off-by: Milos Vyletel > > > > > > Hmmm... Seems like that should be obvious, but on the other hand, I > > > have been using RCU for more than twenty years, so my obviousness > > > sensors might need recalibration. > > > > > > Queued for 4.2. > > > > > > Thanx, Paul > > > > It's just that the original text suggests repeated rcu_dereference() calls > > are > discouraged because they are ugly and not efficient on some architectures. > When I read that I concluded that those were the only reasons not to do it, > that despite the possible inefficiency it would always return the same > pointer. Depending on how one's code is structured, being able to do this > could be advantageous. Then I started looking at the code that implements it > and I couldn't see how it could possibly be the case. I even wrote a little > kernel module to prove to myself that doing this could return different > pointer values. If I misinterpreted the original text I figured others might > also. > Milos even found some code in the kernel where it's author had done this, > so it might be a widely held misunderstanding. It's easy for people who have > worked with rwlock_ts to think an RCU read lock works the same. > > Fair point, and thank you the rationale! Are there any other parts of the RCU > documentation that are similarly blind to your initial point of view? If so, > it > would be good for them to be fixed. > > Thanx, Paul I can't think of much off the top of my head, but I'm hoping I might get some time to review it again and perhaps provide some more concrete suggestions. One thing that does come to mind is the article you wrote in LWN, http://lwn.net/Articles/263130/, where you discussed RCU as a reader-write lock replacement. whatisRCU.txt seems to incorporated some of that. Something along the lines of the original section in the LWN article where there was some discussion of the differences between a rwlock_t read lock critical section and a RCU read lock critical section might be beneficial, a key thing being that with RCU there really is no locking, the value of the pointer can change in a RCU critical section because writers aren't blocked from updating it. Another thing might be some discussion that the cases where you'd call read_lock_bh() are way different than when you'd call rcu_read_lock_bh() and as a corollary why there is no rcu_read_lock_irq(). To me it seems that the names of some of these functions are perhaps misleading. rcu_read_lock() sort of implies there is some locking going on when there isn't. It might have been easier to understand if rcu_read_lock() was called rcu_get() and rcu_read_unlock() was called rcu_put() to reflect that they are really as much about memory management as synchronization. Too late the change any of that obviously. Thanks, Jeff Haran -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] rcu: small rcu_dereference doc update
> -Original Message- > From: Paul E. McKenney [mailto:paul...@linux.vnet.ibm.com] > Sent: Friday, April 17, 2015 7:07 AM > To: Milos Vyletel > Cc: Josh Triplett; Steven Rostedt; Mathieu Desnoyers; Lai Jiangshan; > Jonathan Corbet; open list:READ-COPY UPDATE...; open > list:DOCUMENTATION; Jeff Haran > Subject: Re: [PATCH] rcu: small rcu_dereference doc update > > On Fri, Apr 17, 2015 at 12:33:36PM +0200, Milos Vyletel wrote: > > Make a note stating that repeated calls of rcu_dereference() may not > > return the same pointer if update happens while in critical section. > > > > Reported-by: Jeff Haran > > Signed-off-by: Milos Vyletel > > Hmmm... Seems like that should be obvious, but on the other hand, I have > been using RCU for more than twenty years, so my obviousness sensors > might need recalibration. > > Queued for 4.2. > > Thanx, Paul It's just that the original text suggests repeated rcu_dereference() calls are discouraged because they are ugly and not efficient on some architectures. When I read that I concluded that those were the only reasons not to do it, that despite the possible inefficiency it would always return the same pointer. Depending on how one's code is structured, being able to do this could be advantageous. Then I started looking at the code that implements it and I couldn't see how it could possibly be the case. I even wrote a little kernel module to prove to myself that doing this could return different pointer values. If I misinterpreted the original text I figured others might also. Milos even found some code in the kernel where it's author had done this, so it might be a widely held misunderstanding. It's easy for people who have worked with rwlock_ts to think an RCU read lock works the same. Thanks, Jeff Haran > > --- > > Documentation/RCU/whatisRCU.txt | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/RCU/whatisRCU.txt > > b/Documentation/RCU/whatisRCU.txt index 88dfce1..82b1b2c 100644 > > --- a/Documentation/RCU/whatisRCU.txt > > +++ b/Documentation/RCU/whatisRCU.txt > > @@ -256,7 +256,9 @@ rcu_dereference() > > If you are going to be fetching multiple fields from the > > RCU-protected structure, using the local variable is of > > course preferred. Repeated rcu_dereference() calls look > > - ugly and incur unnecessary overhead on Alpha CPUs. > > + ugly, do not guarantee that same pointer will be returned > > + if update happened while in critical section and incur > > + unnecessary overhead on Alpha CPUs. > > > > Note that the value returned by rcu_dereference() is valid > > only within the enclosing RCU read-side critical section. > > -- > > 2.1.0 > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] rcu: small rcu_dereference doc update
-Original Message- From: Paul E. McKenney [mailto:paul...@linux.vnet.ibm.com] Sent: Friday, April 17, 2015 7:07 AM To: Milos Vyletel Cc: Josh Triplett; Steven Rostedt; Mathieu Desnoyers; Lai Jiangshan; Jonathan Corbet; open list:READ-COPY UPDATE...; open list:DOCUMENTATION; Jeff Haran Subject: Re: [PATCH] rcu: small rcu_dereference doc update On Fri, Apr 17, 2015 at 12:33:36PM +0200, Milos Vyletel wrote: Make a note stating that repeated calls of rcu_dereference() may not return the same pointer if update happens while in critical section. Reported-by: Jeff Haran jeff.ha...@citrix.com Signed-off-by: Milos Vyletel mi...@redhat.com Hmmm... Seems like that should be obvious, but on the other hand, I have been using RCU for more than twenty years, so my obviousness sensors might need recalibration. Queued for 4.2. Thanx, Paul It's just that the original text suggests repeated rcu_dereference() calls are discouraged because they are ugly and not efficient on some architectures. When I read that I concluded that those were the only reasons not to do it, that despite the possible inefficiency it would always return the same pointer. Depending on how one's code is structured, being able to do this could be advantageous. Then I started looking at the code that implements it and I couldn't see how it could possibly be the case. I even wrote a little kernel module to prove to myself that doing this could return different pointer values. If I misinterpreted the original text I figured others might also. Milos even found some code in the kernel where it's author had done this, so it might be a widely held misunderstanding. It's easy for people who have worked with rwlock_ts to think an RCU read lock works the same. Thanks, Jeff Haran --- Documentation/RCU/whatisRCU.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Documentation/RCU/whatisRCU.txt b/Documentation/RCU/whatisRCU.txt index 88dfce1..82b1b2c 100644 --- a/Documentation/RCU/whatisRCU.txt +++ b/Documentation/RCU/whatisRCU.txt @@ -256,7 +256,9 @@ rcu_dereference() If you are going to be fetching multiple fields from the RCU-protected structure, using the local variable is of course preferred. Repeated rcu_dereference() calls look - ugly and incur unnecessary overhead on Alpha CPUs. + ugly, do not guarantee that same pointer will be returned + if update happened while in critical section and incur + unnecessary overhead on Alpha CPUs. Note that the value returned by rcu_dereference() is valid only within the enclosing RCU read-side critical section. -- 2.1.0 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] rcu: small rcu_dereference doc update
-Original Message- From: Paul E. McKenney [mailto:paul...@linux.vnet.ibm.com] Sent: Friday, April 17, 2015 11:41 AM To: Jeff Haran Cc: Milos Vyletel; Josh Triplett; Steven Rostedt; Mathieu Desnoyers; Lai Jiangshan; Jonathan Corbet; open list:READ-COPY UPDATE...; open list:DOCUMENTATION Subject: Re: [PATCH] rcu: small rcu_dereference doc update On Fri, Apr 17, 2015 at 04:53:15PM +, Jeff Haran wrote: -Original Message- From: Paul E. McKenney [mailto:paul...@linux.vnet.ibm.com] Sent: Friday, April 17, 2015 7:07 AM To: Milos Vyletel Cc: Josh Triplett; Steven Rostedt; Mathieu Desnoyers; Lai Jiangshan; Jonathan Corbet; open list:READ-COPY UPDATE...; open list:DOCUMENTATION; Jeff Haran Subject: Re: [PATCH] rcu: small rcu_dereference doc update On Fri, Apr 17, 2015 at 12:33:36PM +0200, Milos Vyletel wrote: Make a note stating that repeated calls of rcu_dereference() may not return the same pointer if update happens while in critical section. Reported-by: Jeff Haran jeff.ha...@citrix.com Signed-off-by: Milos Vyletel mi...@redhat.com Hmmm... Seems like that should be obvious, but on the other hand, I have been using RCU for more than twenty years, so my obviousness sensors might need recalibration. Queued for 4.2. Thanx, Paul It's just that the original text suggests repeated rcu_dereference() calls are discouraged because they are ugly and not efficient on some architectures. When I read that I concluded that those were the only reasons not to do it, that despite the possible inefficiency it would always return the same pointer. Depending on how one's code is structured, being able to do this could be advantageous. Then I started looking at the code that implements it and I couldn't see how it could possibly be the case. I even wrote a little kernel module to prove to myself that doing this could return different pointer values. If I misinterpreted the original text I figured others might also. Milos even found some code in the kernel where it's author had done this, so it might be a widely held misunderstanding. It's easy for people who have worked with rwlock_ts to think an RCU read lock works the same. Fair point, and thank you the rationale! Are there any other parts of the RCU documentation that are similarly blind to your initial point of view? If so, it would be good for them to be fixed. Thanx, Paul I can't think of much off the top of my head, but I'm hoping I might get some time to review it again and perhaps provide some more concrete suggestions. One thing that does come to mind is the article you wrote in LWN, http://lwn.net/Articles/263130/, where you discussed RCU as a reader-write lock replacement. whatisRCU.txt seems to incorporated some of that. Something along the lines of the original section in the LWN article where there was some discussion of the differences between a rwlock_t read lock critical section and a RCU read lock critical section might be beneficial, a key thing being that with RCU there really is no locking, the value of the pointer can change in a RCU critical section because writers aren't blocked from updating it. Another thing might be some discussion that the cases where you'd call read_lock_bh() are way different than when you'd call rcu_read_lock_bh() and as a corollary why there is no rcu_read_lock_irq(). To me it seems that the names of some of these functions are perhaps misleading. rcu_read_lock() sort of implies there is some locking going on when there isn't. It might have been easier to understand if rcu_read_lock() was called rcu_get() and rcu_read_unlock() was called rcu_put() to reflect that they are really as much about memory management as synchronization. Too late the change any of that obviously. Thanks, Jeff Haran -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/1] IBM PPC EMAC driver:improved support for PHY,resending
> -Original Message- > From: Benjamin Herrenschmidt [mailto:[EMAIL PROTECTED] > Sent: Thursday, April 26, 2007 8:48 PM > To: Jeff Haran > Cc: [EMAIL PROTECTED]; linux-kernel@vger.kernel.org > Subject: Re: [PATCH 1/1] IBM PPC EMAC driver:improved support > for PHY,resending > > On Thu, 2007-04-26 at 18:18 -0700, Jeff Haran wrote: > > From: Jeff Haran <[EMAIL PROTECTED]> > > > > Resending with Outlook patch mangling hopefully corrected (Maybe I > > should write a HOWTO, this was harder than fixing the driver). > > Note, sorry about that, still mangled :-( > > Just send it as an attachment ... and get yourself a linux desktop :-) > > Cheers, > Ben. > Ben, OK. Attached is a gziped tar file named patch.tar.gz containing the following: patch - The patch file, hopefully ungarbled. ibm_emac_phy.c - The modified source file, in case the patch file is still wrong. log.txt - A log of what I did to create the patch, in case I bungled that too. Note I created the patch using the instructions in Documentation/SubmittingPatches. Let me know if this gets thru ok. If not, I can try Morse Code. Jeff Haran patch.tar.gz Description: patch.tar.gz
RE: [PATCH 1/1] IBM PPC EMAC driver:improved support for PHY,resending
-Original Message- From: Benjamin Herrenschmidt [mailto:[EMAIL PROTECTED] Sent: Thursday, April 26, 2007 8:48 PM To: Jeff Haran Cc: [EMAIL PROTECTED]; linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/1] IBM PPC EMAC driver:improved support for PHY,resending On Thu, 2007-04-26 at 18:18 -0700, Jeff Haran wrote: From: Jeff Haran [EMAIL PROTECTED] Resending with Outlook patch mangling hopefully corrected (Maybe I should write a HOWTO, this was harder than fixing the driver). Note, sorry about that, still mangled :-( Just send it as an attachment ... and get yourself a linux desktop :-) Cheers, Ben. Ben, OK. Attached is a gziped tar file named patch.tar.gz containing the following: patch - The patch file, hopefully ungarbled. ibm_emac_phy.c - The modified source file, in case the patch file is still wrong. log.txt - A log of what I did to create the patch, in case I bungled that too. Note I created the patch using the instructions in Documentation/SubmittingPatches. Let me know if this gets thru ok. If not, I can try Morse Code. Jeff Haran patch.tar.gz Description: patch.tar.gz
[PATCH 1/1] IBM PPC EMAC driver:improved support for PHY, resending
From: Jeff Haran <[EMAIL PROTECTED]>
Resending with Outlook patch mangling hopefully corrected (Maybe I
should write a HOWTO, this was harder than fixing the driver).
This patch fixes some problems I found while debugging the IBM EMAC
driver for PPC32 systems.
The first problem was in the function that configures the PHY for
autonegotiation, genmii_setup_aneg(). The original code does a
read/modify/write of the autonegotiation advertizement register (reg 4),
followed by a read/modify/write of the control register (reg 0). While
the original code follows the proper procedure as per reading the IEEE
specs, what I found is that on at least one PHY model (National DP83843)
the read of the control register comes back with the soft reset bit set
(bit 15). Because of the read/modify/write operation, this causes the
write to write a 1 back to the reset bit, which initiates a software
reset of the PHY. This software reset causes the PHY to return to its
power up state which advertizes all modes of operation, thus negating
the write to the autoneg advertizement register. The modification is to
spin reading the control register until the soft reset bit is clear
before doing the modify/write. I guess this bit is in reality more of
the "device busy" bit on at least some PHYs.
The second problem was in the function that configures the PHY for
forced operation, genmii_setup_forced(). The original code initiates a
software reset operation via a write of a 1 to bit 15 of the control
register (reg 0), but then proceeds to do a second write to that same
register without waiting until that reset bit is cleared by the PHY
itself (which according to the IEEE specs indicates that the PHY reset
is complete). This is a violation of how one is supposed to use this
software reset feature of these PHYs and I believe was the cause of
mysterious, difficult to reproduce link failures that we've observed on
some of our systems that use this driver. The fix is to modify the
function so that it spins waiting for the reset bit to clear after doing
the soft reset and before doing the subsequent write. Since this
modification, we haven't seen the mysterious link failures, though they
were so rare its difficult to say at this point whether this was the
cause.
I also added some error handling and reporting for the abnormal case
where the reset bit never clears from the soft reset operation.
Applied to kernel version 2.6.21.
Signed-off-by: Jeff Haran <[EMAIL PROTECTED]>
---
--- linux-2.6.21/drivers/net/ibm_emac/ibm_emac_phy.c.orig
2007-04-25 20:08:32.0 -0700
+++ linux-2.6.21/drivers/net/ibm_emac/ibm_emac_phy.c2007-04-26
14:42:09.562996000 -0700
@@ -22,8 +22,12 @@
#include
+#include "ibm_emac_core.h"
#include "ibm_emac_phy.h"
+#define NL "\n"
+#define PHY_DBG(f,x...) printk("emac" f, ##x)
+
static inline int phy_read(struct mii_phy *phy, int reg)
{
return phy->mdio_read(phy->dev, phy->address, reg);
@@ -34,11 +38,34 @@ static inline void phy_write(struct mii_
phy->mdio_write(phy->dev, phy->address, reg, val);
}
-int mii_reset_phy(struct mii_phy *phy)
+/*
+ * polls MII_BMCR until BMCR_RESET bit clears or operation times out.
+ *
+ * returns:
+ * >= 0 => success, value in BMCR returned to caller
+ * -EBUSY => failure, RESET bit never cleared
+ * otherwise => failure, lower level PHY read failed
+ */
+
+static int mii_spin_reset_complete(struct mii_phy *phy)
{
int val;
int limit = 1;
+ while (limit--) {
+ val = phy_read(phy, MII_BMCR);
+ if ((val >= 0) && ((val & BMCR_RESET) == 0))
+ return val; /* success */
+ udelay(10);
+ }
+
+ return (val < 0) ? val : -EBUSY;
+}
+
+int mii_reset_phy(struct mii_phy *phy)
+{
+ int val;
+
val = phy_read(phy, MII_BMCR);
val &= ~BMCR_ISOLATE;
val |= BMCR_RESET;
@@ -46,16 +73,17 @@ int mii_reset_phy(struct mii_phy *phy)
udelay(300);
- while (limit--) {
- val = phy_read(phy, MII_BMCR);
- if (val >= 0 && (val & BMCR_RESET) == 0)
- break;
- udelay(10);
+ val = mii_spin_reset_complete(phy);
+
+ if (val < 0) {
+ PHY_DBG("%d: reset_complete failed in reset %d" NL,
+ ((struct ocp_enet_private *)
(phy->dev->priv))->def->index, val);
+ } else {
+ if (val & BMCR_ISOLATE)
+ phy_write(phy, MII_BMCR, val & ~BMCR_ISOLATE);
}
- if ((val & BMCR_ISOLATE) && limit > 0)
- phy_write(phy, MII_BMCR, val & ~BMCR_ISOLATE);
- return limit <= 0;
+ return val < 0;
}
static int genmii_setup_aneg(struct mii_phy *phy, u32 advertise)
@@ -102,9 +130,18 @@ stat
RE: [PATCH 1/1] IBM PPC EMAC driver:improved support for PHYconfiguration
> -Original Message- > From: Benjamin Herrenschmidt [mailto:[EMAIL PROTECTED] > Sent: Thursday, April 26, 2007 5:19 PM > To: Jeff Haran > Cc: [EMAIL PROTECTED]; linux-kernel@vger.kernel.org > Subject: Re: [PATCH 1/1] IBM PPC EMAC driver:improved support > for PHYconfiguration > > On Thu, 2007-04-26 at 16:18 -0700, Jeff Haran wrote: > > From: Jeff Haran <[EMAIL PROTECTED]> > > > > This patch fixes some problems I found while debugging the IBM EMAC ... > Your patch appears to have been line wrapped by your mailer though... Benjamin, Friggin Outlook and our M$ centered corporate IT environment. Even when you tell it to send plain text it modifies it. This was my first attempt at patch submission, so I don't know all the hoops yet. I can try sending the patch again if I can figure out how to disable the line wrap. Or if you have some FTP server I can put to I can send you the patch file directly there. Please let me know which you'd prefer. Thanks, Jeff > > Cheers, > Ben. > > > - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] IBM PPC EMAC driver:improved support for PHY configuration
From: Jeff Haran <[EMAIL PROTECTED]>
This patch fixes some problems I found while debugging the IBM EMAC
driver for PPC32 systems.
The first problem was in the function that configures the PHY for
autonegotiation, genmii_setup_aneg(). The original code does a
read/modify/write of the autonegotiation advertizement register (reg 4),
followed by a read/modify/write of the control register (reg 0). While
the original code follows the proper procedure as per reading the IEEE
specs, what I found is that on at least one PHY model (National DP83843)
the read of the control register comes back with the soft reset bit set
(bit 15). Because of the read/modify/write operation, this causes the
write to write a 1 back to the reset bit, which initiates a software
reset of the PHY. This software reset causes the PHY to return to its
power up state which advertizes all modes of operation, thus negating
the write to the autoneg advertizement register. The modification is to
spin reading the control register until the soft reset bit is clear
before doing the modify/write. I guess this bit is in reality more of
the "device busy" bit on at least some PHYs.
The second problem was in the function that configures the PHY for
forced operation, genmii_setup_forced(). The original code initiates a
software reset operation via a write of a 1 to bit 15 of the control
register (reg 0), but then proceeds to do a second write to that same
register without waiting until that reset bit is cleared by the PHY
itself (which according to the IEEE specs indicates that the PHY reset
is complete). This is a violation of how one is supposed to use this
software reset feature of these PHYs and I believe was the cause of
mysterious, difficult to reproduce link failures that we've observed on
some of our systems that use this driver. The fix is to modify the
function so that it spins waiting for the reset bit to clear after doing
the soft reset and before doing the subsequent write. Since this
modification, we haven't seen the mysterious link failures, though they
were so rare its difficult to say at this point whether this was the
cause.
I also added some error handling and reporting for the abnormal case
where the reset bit never clears from the soft reset operation.
Applied to kernel version 2.6.21.
Signed-off-by: Jeff Haran <[EMAIL PROTECTED]>
---
--- linux-2.6.21/drivers/net/ibm_emac/ibm_emac_phy.c.orig
2007-04-25 20:08:32.0 -0700
+++ linux-2.6.21/drivers/net/ibm_emac/ibm_emac_phy.c2007-04-26
14:42:09.562996000 -0700
@@ -22,8 +22,12 @@
#include
+#include "ibm_emac_core.h"
#include "ibm_emac_phy.h"
+#define NL "\n"
+#define PHY_DBG(f,x...) printk("emac" f, ##x)
+
static inline int phy_read(struct mii_phy *phy, int reg)
{
return phy->mdio_read(phy->dev, phy->address, reg);
@@ -34,11 +38,34 @@ static inline void phy_write(struct mii_
phy->mdio_write(phy->dev, phy->address, reg, val);
}
-int mii_reset_phy(struct mii_phy *phy)
+/*
+ * polls MII_BMCR until BMCR_RESET bit clears or operation times out.
+ *
+ * returns:
+ * >= 0 => success, value in BMCR returned to caller
+ * -EBUSY => failure, RESET bit never cleared
+ * otherwise => failure, lower level PHY read failed
+ */
+
+static int mii_spin_reset_complete(struct mii_phy *phy)
{
int val;
int limit = 1;
+ while (limit--) {
+ val = phy_read(phy, MII_BMCR);
+ if ((val >= 0) && ((val & BMCR_RESET) == 0))
+ return val; /* success */
+ udelay(10);
+ }
+
+ return (val < 0) ? val : -EBUSY;
+}
+
+int mii_reset_phy(struct mii_phy *phy)
+{
+ int val;
+
val = phy_read(phy, MII_BMCR);
val &= ~BMCR_ISOLATE;
val |= BMCR_RESET;
@@ -46,16 +73,17 @@ int mii_reset_phy(struct mii_phy *phy)
udelay(300);
- while (limit--) {
- val = phy_read(phy, MII_BMCR);
- if (val >= 0 && (val & BMCR_RESET) == 0)
- break;
- udelay(10);
+ val = mii_spin_reset_complete(phy);
+
+ if (val < 0) {
+ PHY_DBG("%d: reset_complete failed in reset %d" NL,
+ ((struct ocp_enet_private *)
(phy->dev->priv))->def->index, val);
+ } else {
+ if (val & BMCR_ISOLATE)
+ phy_write(phy, MII_BMCR, val & ~BMCR_ISOLATE);
}
- if ((val & BMCR_ISOLATE) && limit > 0)
- phy_write(phy, MII_BMCR, val & ~BMCR_ISOLATE);
- return limit <= 0;
+ return val < 0;
}
static int genmii_setup_aneg(struct mii_phy *phy, u32 advertise)
@@ -102,9 +130,18 @@ static int genmii_setup_aneg(struct mii_
}
/* Start/Restart aneg */
- ctl = phy_read(phy, MII_BMCR);
-
[PATCH 1/1] IBM PPC EMAC driver:improved support for PHY configuration
From: Jeff Haran [EMAIL PROTECTED]
This patch fixes some problems I found while debugging the IBM EMAC
driver for PPC32 systems.
The first problem was in the function that configures the PHY for
autonegotiation, genmii_setup_aneg(). The original code does a
read/modify/write of the autonegotiation advertizement register (reg 4),
followed by a read/modify/write of the control register (reg 0). While
the original code follows the proper procedure as per reading the IEEE
specs, what I found is that on at least one PHY model (National DP83843)
the read of the control register comes back with the soft reset bit set
(bit 15). Because of the read/modify/write operation, this causes the
write to write a 1 back to the reset bit, which initiates a software
reset of the PHY. This software reset causes the PHY to return to its
power up state which advertizes all modes of operation, thus negating
the write to the autoneg advertizement register. The modification is to
spin reading the control register until the soft reset bit is clear
before doing the modify/write. I guess this bit is in reality more of
the device busy bit on at least some PHYs.
The second problem was in the function that configures the PHY for
forced operation, genmii_setup_forced(). The original code initiates a
software reset operation via a write of a 1 to bit 15 of the control
register (reg 0), but then proceeds to do a second write to that same
register without waiting until that reset bit is cleared by the PHY
itself (which according to the IEEE specs indicates that the PHY reset
is complete). This is a violation of how one is supposed to use this
software reset feature of these PHYs and I believe was the cause of
mysterious, difficult to reproduce link failures that we've observed on
some of our systems that use this driver. The fix is to modify the
function so that it spins waiting for the reset bit to clear after doing
the soft reset and before doing the subsequent write. Since this
modification, we haven't seen the mysterious link failures, though they
were so rare its difficult to say at this point whether this was the
cause.
I also added some error handling and reporting for the abnormal case
where the reset bit never clears from the soft reset operation.
Applied to kernel version 2.6.21.
Signed-off-by: Jeff Haran [EMAIL PROTECTED]
---
--- linux-2.6.21/drivers/net/ibm_emac/ibm_emac_phy.c.orig
2007-04-25 20:08:32.0 -0700
+++ linux-2.6.21/drivers/net/ibm_emac/ibm_emac_phy.c2007-04-26
14:42:09.562996000 -0700
@@ -22,8 +22,12 @@
#include asm/ocp.h
+#include ibm_emac_core.h
#include ibm_emac_phy.h
+#define NL \n
+#define PHY_DBG(f,x...) printk(emac f, ##x)
+
static inline int phy_read(struct mii_phy *phy, int reg)
{
return phy-mdio_read(phy-dev, phy-address, reg);
@@ -34,11 +38,34 @@ static inline void phy_write(struct mii_
phy-mdio_write(phy-dev, phy-address, reg, val);
}
-int mii_reset_phy(struct mii_phy *phy)
+/*
+ * polls MII_BMCR until BMCR_RESET bit clears or operation times out.
+ *
+ * returns:
+ * = 0 = success, value in BMCR returned to caller
+ * -EBUSY = failure, RESET bit never cleared
+ * otherwise = failure, lower level PHY read failed
+ */
+
+static int mii_spin_reset_complete(struct mii_phy *phy)
{
int val;
int limit = 1;
+ while (limit--) {
+ val = phy_read(phy, MII_BMCR);
+ if ((val = 0) ((val BMCR_RESET) == 0))
+ return val; /* success */
+ udelay(10);
+ }
+
+ return (val 0) ? val : -EBUSY;
+}
+
+int mii_reset_phy(struct mii_phy *phy)
+{
+ int val;
+
val = phy_read(phy, MII_BMCR);
val = ~BMCR_ISOLATE;
val |= BMCR_RESET;
@@ -46,16 +73,17 @@ int mii_reset_phy(struct mii_phy *phy)
udelay(300);
- while (limit--) {
- val = phy_read(phy, MII_BMCR);
- if (val = 0 (val BMCR_RESET) == 0)
- break;
- udelay(10);
+ val = mii_spin_reset_complete(phy);
+
+ if (val 0) {
+ PHY_DBG(%d: reset_complete failed in reset %d NL,
+ ((struct ocp_enet_private *)
(phy-dev-priv))-def-index, val);
+ } else {
+ if (val BMCR_ISOLATE)
+ phy_write(phy, MII_BMCR, val ~BMCR_ISOLATE);
}
- if ((val BMCR_ISOLATE) limit 0)
- phy_write(phy, MII_BMCR, val ~BMCR_ISOLATE);
- return limit = 0;
+ return val 0;
}
static int genmii_setup_aneg(struct mii_phy *phy, u32 advertise)
@@ -102,9 +130,18 @@ static int genmii_setup_aneg(struct mii_
}
/* Start/Restart aneg */
- ctl = phy_read(phy, MII_BMCR);
- ctl |= (BMCR_ANENABLE | BMCR_ANRESTART);
- phy_write(phy, MII_BMCR, ctl);
+ /* on some PHYs (e.g. National DP83843) a write to MII_ADVERTISE
+* causes BMCR_RESET to be set on the next read of MII_BMCR,
which
RE: [PATCH 1/1] IBM PPC EMAC driver:improved support for PHYconfiguration
-Original Message- From: Benjamin Herrenschmidt [mailto:[EMAIL PROTECTED] Sent: Thursday, April 26, 2007 5:19 PM To: Jeff Haran Cc: [EMAIL PROTECTED]; linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/1] IBM PPC EMAC driver:improved support for PHYconfiguration On Thu, 2007-04-26 at 16:18 -0700, Jeff Haran wrote: From: Jeff Haran [EMAIL PROTECTED] This patch fixes some problems I found while debugging the IBM EMAC ... Your patch appears to have been line wrapped by your mailer though... Benjamin, Friggin Outlook and our M$ centered corporate IT environment. Even when you tell it to send plain text it modifies it. This was my first attempt at patch submission, so I don't know all the hoops yet. I can try sending the patch again if I can figure out how to disable the line wrap. Or if you have some FTP server I can put to I can send you the patch file directly there. Please let me know which you'd prefer. Thanks, Jeff Cheers, Ben. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] IBM PPC EMAC driver:improved support for PHY, resending
From: Jeff Haran [EMAIL PROTECTED]
Resending with Outlook patch mangling hopefully corrected (Maybe I
should write a HOWTO, this was harder than fixing the driver).
This patch fixes some problems I found while debugging the IBM EMAC
driver for PPC32 systems.
The first problem was in the function that configures the PHY for
autonegotiation, genmii_setup_aneg(). The original code does a
read/modify/write of the autonegotiation advertizement register (reg 4),
followed by a read/modify/write of the control register (reg 0). While
the original code follows the proper procedure as per reading the IEEE
specs, what I found is that on at least one PHY model (National DP83843)
the read of the control register comes back with the soft reset bit set
(bit 15). Because of the read/modify/write operation, this causes the
write to write a 1 back to the reset bit, which initiates a software
reset of the PHY. This software reset causes the PHY to return to its
power up state which advertizes all modes of operation, thus negating
the write to the autoneg advertizement register. The modification is to
spin reading the control register until the soft reset bit is clear
before doing the modify/write. I guess this bit is in reality more of
the device busy bit on at least some PHYs.
The second problem was in the function that configures the PHY for
forced operation, genmii_setup_forced(). The original code initiates a
software reset operation via a write of a 1 to bit 15 of the control
register (reg 0), but then proceeds to do a second write to that same
register without waiting until that reset bit is cleared by the PHY
itself (which according to the IEEE specs indicates that the PHY reset
is complete). This is a violation of how one is supposed to use this
software reset feature of these PHYs and I believe was the cause of
mysterious, difficult to reproduce link failures that we've observed on
some of our systems that use this driver. The fix is to modify the
function so that it spins waiting for the reset bit to clear after doing
the soft reset and before doing the subsequent write. Since this
modification, we haven't seen the mysterious link failures, though they
were so rare its difficult to say at this point whether this was the
cause.
I also added some error handling and reporting for the abnormal case
where the reset bit never clears from the soft reset operation.
Applied to kernel version 2.6.21.
Signed-off-by: Jeff Haran [EMAIL PROTECTED]
---
--- linux-2.6.21/drivers/net/ibm_emac/ibm_emac_phy.c.orig
2007-04-25 20:08:32.0 -0700
+++ linux-2.6.21/drivers/net/ibm_emac/ibm_emac_phy.c2007-04-26
14:42:09.562996000 -0700
@@ -22,8 +22,12 @@
#include asm/ocp.h
+#include ibm_emac_core.h
#include ibm_emac_phy.h
+#define NL \n
+#define PHY_DBG(f,x...) printk(emac f, ##x)
+
static inline int phy_read(struct mii_phy *phy, int reg)
{
return phy-mdio_read(phy-dev, phy-address, reg);
@@ -34,11 +38,34 @@ static inline void phy_write(struct mii_
phy-mdio_write(phy-dev, phy-address, reg, val);
}
-int mii_reset_phy(struct mii_phy *phy)
+/*
+ * polls MII_BMCR until BMCR_RESET bit clears or operation times out.
+ *
+ * returns:
+ * = 0 = success, value in BMCR returned to caller
+ * -EBUSY = failure, RESET bit never cleared
+ * otherwise = failure, lower level PHY read failed
+ */
+
+static int mii_spin_reset_complete(struct mii_phy *phy)
{
int val;
int limit = 1;
+ while (limit--) {
+ val = phy_read(phy, MII_BMCR);
+ if ((val = 0) ((val BMCR_RESET) == 0))
+ return val; /* success */
+ udelay(10);
+ }
+
+ return (val 0) ? val : -EBUSY;
+}
+
+int mii_reset_phy(struct mii_phy *phy)
+{
+ int val;
+
val = phy_read(phy, MII_BMCR);
val = ~BMCR_ISOLATE;
val |= BMCR_RESET;
@@ -46,16 +73,17 @@ int mii_reset_phy(struct mii_phy *phy)
udelay(300);
- while (limit--) {
- val = phy_read(phy, MII_BMCR);
- if (val = 0 (val BMCR_RESET) == 0)
- break;
- udelay(10);
+ val = mii_spin_reset_complete(phy);
+
+ if (val 0) {
+ PHY_DBG(%d: reset_complete failed in reset %d NL,
+ ((struct ocp_enet_private *)
(phy-dev-priv))-def-index, val);
+ } else {
+ if (val BMCR_ISOLATE)
+ phy_write(phy, MII_BMCR, val ~BMCR_ISOLATE);
}
- if ((val BMCR_ISOLATE) limit 0)
- phy_write(phy, MII_BMCR, val ~BMCR_ISOLATE);
- return limit = 0;
+ return val 0;
}
static int genmii_setup_aneg(struct mii_phy *phy, u32 advertise)
@@ -102,9 +130,18 @@ static int genmii_setup_aneg(struct mii_
}
/* Start/Restart aneg */
- ctl = phy_read(phy, MII_BMCR);
- ctl |= (BMCR_ANENABLE | BMCR_ANRESTART);
- phy_write(phy, MII_BMCR, ctl);
+ /* on some
