[PATCH 0/2] net-next/fec: bug fixing after introduced phylib supporting

[PATCH 0/2] net-next/fec: bug fixing after introduced phylib supporting

[Bryan Wu]

After introduced phylib supporting, we found some critical issues in Ubuntu on
Freescale iMX51. Following 2 patches fix those bugs which was recorded in our
Launchpad bug tracker.

Bryan Wu (2):
  netdev/fec: fix performance impact from mdio poll operation
  netdev/fec: fix ifconfig eth0 down hang issue

 drivers/net/fec.c |   73 +++++++++++++++++++++++++++--------------------------
 1 files changed, 37 insertions(+), 36 deletions(-)

[PATCH 1/2] netdev/fec: fix performance impact from mdio poll operation

[Bryan Wu]

BugLink: http://bugs.launchpad.net/bugs/546649
BugLink: http://bugs.launchpad.net/bugs/457878

After introducing phylib supporting, users experienced performace drop. That is
because of the mdio polling operation of phylib. Use msleep to replace the busy
waiting cpu_relax() and remove the warning message.

Signed-off-by: Bryan Wu <bryan.wu@canonical.com>
Acked-by: Andy Whitcroft <apw@canonical.com>
---
 drivers/net/fec.c |   45 +++++++++++++++++++++------------------------
 1 files changed, 21 insertions(+), 24 deletions(-)

diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index 2b1651a..9c58f6b 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -203,7 +203,7 @@ static void fec_stop(struct net_device *dev);
 #define FEC_MMFR_TA		(2 << 16)
 #define FEC_MMFR_DATA(v)	(v & 0xffff)
 
-#define FEC_MII_TIMEOUT		10000
+#define FEC_MII_TIMEOUT		10
 
 /* Transmitter timeout */
 #define TX_TIMEOUT (2 * HZ)
@@ -611,13 +611,29 @@ spin_unlock:
 /*
  * NOTE: a MII transaction is during around 25 us, so polling it...
  */
-static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
+static int fec_enet_mdio_poll(struct fec_enet_private *fep)
 {
-	struct fec_enet_private *fep = bus->priv;
 	int timeout = FEC_MII_TIMEOUT;
 
 	fep->mii_timeout = 0;
 
+	/* wait for end of transfer */
+	while (!(readl(fep->hwp + FEC_IEVENT) & FEC_ENET_MII)) {
+		msleep(1);
+		if (timeout-- < 0) {
+			fep->mii_timeout = 1;
+			break;
+		}
+	}
+
+	return 0;
+}
+
+static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
+{
+	struct fec_enet_private *fep = bus->priv;
+
+
 	/* clear MII end of transfer bit*/
 	writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
 
@@ -626,15 +642,7 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 		FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(regnum) |
 		FEC_MMFR_TA, fep->hwp + FEC_MII_DATA);
 
-	/* wait for end of transfer */
-	while (!(readl(fep->hwp + FEC_IEVENT) & FEC_ENET_MII)) {
-		cpu_relax();
-		if (timeout-- < 0) {
-			fep->mii_timeout = 1;
-			printk(KERN_ERR "FEC: MDIO read timeout\n");
-			return -ETIMEDOUT;
-		}
-	}
+	fec_enet_mdio_poll(fep);
 
 	/* return value */
 	return FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
@@ -644,9 +652,6 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 			   u16 value)
 {
 	struct fec_enet_private *fep = bus->priv;
-	int timeout = FEC_MII_TIMEOUT;
-
-	fep->mii_timeout = 0;
 
 	/* clear MII end of transfer bit*/
 	writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
@@ -657,15 +662,7 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 		FEC_MMFR_TA | FEC_MMFR_DATA(value),
 		fep->hwp + FEC_MII_DATA);
 
-	/* wait for end of transfer */
-	while (!(readl(fep->hwp + FEC_IEVENT) & FEC_ENET_MII)) {
-		cpu_relax();
-		if (timeout-- < 0) {
-			fep->mii_timeout = 1;
-			printk(KERN_ERR "FEC: MDIO write timeout\n");
-			return -ETIMEDOUT;
-		}
-	}
+	fec_enet_mdio_poll(fep);
 
 	return 0;
 }
-- 
1.7.0.1

Re: [PATCH 1/2] netdev/fec: fix performance impact from mdio poll operation

[Andy Fleming]

On Thursday, May 6, 2010, Bryan Wu <bryan.wu@canonical.com> wrote:
> BugLink: http://bugs.launchpad.net/bugs/546649
> BugLink: http://bugs.launchpad.net/bugs/457878
>
> After introducing phylib supporting, users experienced performace drop. That is
> because of the mdio polling operation of phylib. Use msleep to replace the busy
> waiting cpu_relax() and remove the warning message.

I'm a little confused by this patch.  In order to improve performance,
you made the mdio functions fail silently?  Why are you getting
timeouts at all?  The Phy lib is not going to respond well if an mdio
write times out without returning an error.  And returning an error
means the whole state machine has to reset, as a failed write is not
something we currently handle gracefully.

Is it possible to use an interrupt to get the phy state change?  This
would allow the phy lib to stop its incessant polling.  It doesn't
solve the question of why it's timing out, though.

>
> Signed-off-by: Bryan Wu <bryan.wu@canonical.com>
> Acked-by: Andy Whitcroft <apw@canonical.com>
> ---
>  drivers/net/fec.c |   45 +++++++++++++++++++++------------------------
>  1 files changed, 21 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/net/fec.c b/drivers/net/fec.c
> index 2b1651a..9c58f6b 100644
> --- a/drivers/net/fec.c
> +++ b/drivers/net/fec.c
> @@ -203,7 +203,7 @@ static void fec_stop(struct net_device *dev);
>  #define FEC_MMFR_TA            (2 << 16)
>  #define FEC_MMFR_DATA(v)       (v & 0xffff)
>
> -#define FEC_MII_TIMEOUT                10000
> +#define FEC_MII_TIMEOUT                10
>
>  /* Transmitter timeout */
>  #define TX_TIMEOUT (2 * HZ)
> @@ -611,13 +611,29 @@ spin_unlock:
>  /*
>   * NOTE: a MII transaction is during around 25 us, so polling it...
>   */
> -static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
> +static int fec_enet_mdio_poll(struct fec_enet_private *fep)
>  {
> -       struct fec_enet_private *fep = bus->priv;
>         int timeout = FEC_MII_TIMEOUT;
>
>         fep->mii_timeout = 0;
>
> +       /* wait for end of transfer */
> +       while (!(readl(fep->hwp + FEC_IEVENT) & FEC_ENET_MII)) {
> +               msleep(1);
> +               if (timeout-- < 0) {
> +                       fep->mii_timeout = 1;
> +                       break;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
> +{
> +       struct fec_enet_private *fep = bus->priv;
> +
> +
>         /* clear MII end of transfer bit*/
>         writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
>
> @@ -626,15 +642,7 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>                 FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(regnum) |
>                 FEC_MMFR_TA, fep->hwp + FEC_MII_DATA);
>
> -       /* wait for end of transfer */
> -       while (!(readl(fep->hwp + FEC_IEVENT) & FEC_ENET_MII)) {
> -               cpu_relax();
> -               if (timeout-- < 0) {
> -                       fep->mii_timeout = 1;
> -                       printk(KERN_ERR "FEC: MDIO read timeout\n");
> -                       return -ETIMEDOUT;
> -               }
> -       }
> +       fec_enet_mdio_poll(fep);
>
>         /* return value */
>         return FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
> @@ -644,9 +652,6 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
>                            u16 value)
>  {
>         struct fec_enet_private *fep = bus->priv;
> -       int timeout = FEC_MII_TIMEOUT;
> -
> -       fep->mii_timeout = 0;
>
>         /* clear MII end of transfer bit*/
>         writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> @@ -657,15 +662,7 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
>                 FEC_MMFR_TA | FEC_MMFR_DATA(value),
>                 fep->hwp + FEC_MII_DATA);
>
> -       /* wait for end of transfer */
> -       while (!(readl(fep->hwp + FEC_IEVENT) & FEC_ENET_MII)) {
> -               cpu_relax();
> -               if (timeout-- < 0) {
> -                       fep->mii_timeout = 1;
> -                       printk(KERN_ERR "FEC: MDIO write timeout\n");
> -                       return -ETIMEDOUT;
> -               }
> -       }
> +       fec_enet_mdio_poll(fep);
>
>         return 0;
>  }
> --
> 1.7.0.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 1/2] netdev/fec: fix performance impact from mdio poll operation

[Andy Fleming]

On Thursday, May 6, 2010, Bryan Wu <bryan.wu@canonical.com> wrote:
> BugLink: http://bugs.launchpad.net/bugs/546649
> BugLink: http://bugs.launchpad.net/bugs/457878
>
> After introducing phylib supporting, users experienced performace drop. That is
> because of the mdio polling operation of phylib. Use msleep to replace the busy
> waiting cpu_relax() and remove the warning message.

I'm a little confused by this patch.  In order to improve performance,
you made the mdio functions fail silently?  Why are you getting
timeouts at all?  The Phy lib is not going to respond well if an mdio
write times out without returning an error.  And returning an error
means the whole state machine has to reset, as a failed write is not
something we currently handle gracefully.

Is it possible to use an interrupt to get the phy state change?  This
would allow the phy lib to stop its incessant polling.  It doesn't
solve the question of why it's timing out, though.

>
> Signed-off-by: Bryan Wu <bryan.wu@canonical.com>
> Acked-by: Andy Whitcroft <apw@canonical.com>
> ---
>  drivers/net/fec.c |   45 +++++++++++++++++++++------------------------
>  1 files changed, 21 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/net/fec.c b/drivers/net/fec.c
> index 2b1651a..9c58f6b 100644
> --- a/drivers/net/fec.c
> +++ b/drivers/net/fec.c
> @@ -203,7 +203,7 @@ static void fec_stop(struct net_device *dev);
>  #define FEC_MMFR_TA            (2 << 16)
>  #define FEC_MMFR_DATA(v)       (v & 0xffff)
>
> -#define FEC_MII_TIMEOUT                10000
> +#define FEC_MII_TIMEOUT                10
>
>  /* Transmitter timeout */
>  #define TX_TIMEOUT (2 * HZ)
> @@ -611,13 +611,29 @@ spin_unlock:
>  /*
>   * NOTE: a MII transaction is during around 25 us, so polling it...
>   */
> -static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
> +static int fec_enet_mdio_poll(struct fec_enet_private *fep)
>  {
> -       struct fec_enet_private *fep = bus->priv;
>         int timeout = FEC_MII_TIMEOUT;
>
>         fep->mii_timeout = 0;
>
> +       /* wait for end of transfer */
> +       while (!(readl(fep->hwp + FEC_IEVENT) & FEC_ENET_MII)) {
> +               msleep(1);
> +               if (timeout-- < 0) {
> +                       fep->mii_timeout = 1;
> +                       break;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
> +{
> +       struct fec_enet_private *fep = bus->priv;
> +
> +
>         /* clear MII end of transfer bit*/
>         writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
>
> @@ -626,15 +642,7 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>                 FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(regnum) |
>                 FEC_MMFR_TA, fep->hwp + FEC_MII_DATA);
>
> -       /* wait for end of transfer */
> -       while (!(readl(fep->hwp + FEC_IEVENT) & FEC_ENET_MII)) {
> -               cpu_relax();
> -               if (timeout-- < 0) {
> -                       fep->mii_timeout = 1;
> -                       printk(KERN_ERR "FEC: MDIO read timeout\n");
> -                       return -ETIMEDOUT;
> -               }
> -       }
> +       fec_enet_mdio_poll(fep);
>
>         /* return value */
>         return FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
> @@ -644,9 +652,6 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
>                            u16 value)
>  {
>         struct fec_enet_private *fep = bus->priv;
> -       int timeout = FEC_MII_TIMEOUT;
> -
> -       fep->mii_timeout = 0;
>
>         /* clear MII end of transfer bit*/
>         writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> @@ -657,15 +662,7 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
>                 FEC_MMFR_TA | FEC_MMFR_DATA(value),
>                 fep->hwp + FEC_MII_DATA);
>
> -       /* wait for end of transfer */
> -       while (!(readl(fep->hwp + FEC_IEVENT) & FEC_ENET_MII)) {
> -               cpu_relax();
> -               if (timeout-- < 0) {
> -                       fep->mii_timeout = 1;
> -                       printk(KERN_ERR "FEC: MDIO write timeout\n");
> -                       return -ETIMEDOUT;
> -               }
> -       }
> +       fec_enet_mdio_poll(fep);
>
>         return 0;
>  }
> --
> 1.7.0.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 1/2] netdev/fec: fix performance impact from mdio poll operation

[Bryan Wu]

On 05/08/2010 12:06 AM, Andy Fleming wrote:
> On Thursday, May 6, 2010, Bryan Wu<bryan.wu@canonical.com>  wrote:
>> BugLink: http://bugs.launchpad.net/bugs/546649
>> BugLink: http://bugs.launchpad.net/bugs/457878
>>
>> After introducing phylib supporting, users experienced performace drop. That is
>> because of the mdio polling operation of phylib. Use msleep to replace the busy
>> waiting cpu_relax() and remove the warning message.
>
> I'm a little confused by this patch.  In order to improve performance,
> you made the mdio functions fail silently?  Why are you getting
> timeouts at all?

 From the BugLink, we experienced a lot of error message about mdio_read 
timeout. Actually, the logical in this function is quite simple and I still have 
no idea about why mdio_read timeout. That might be hardware defect.

> The Phy lib is not going to respond well if an mdio
> write times out without returning an error.  And returning an error
> means the whole state machine has to reset, as a failed write is not
> something we currently handle gracefully.
>

Right, if we return error to phylib due to mdio read times out, the performance 
will drop a lot.

> Is it possible to use an interrupt to get the phy state change?  This
> would allow the phy lib to stop its incessant polling.  It doesn't
> solve the question of why it's timing out, though.
>

I'm going to try the interrupt, but fec driver is shared by several SoCs. It 
might be a little complicated and still cannot fix the timing out issue. Is 
there any example driver to use an interrupt to get the phy state change?

Thanks,
-Bryan

>>
>> Signed-off-by: Bryan Wu<bryan.wu@canonical.com>
>> Acked-by: Andy Whitcroft<apw@canonical.com>
>> ---
>>   drivers/net/fec.c |   45 +++++++++++++++++++++------------------------
>>   1 files changed, 21 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/net/fec.c b/drivers/net/fec.c
>> index 2b1651a..9c58f6b 100644
>> --- a/drivers/net/fec.c
>> +++ b/drivers/net/fec.c
>> @@ -203,7 +203,7 @@ static void fec_stop(struct net_device *dev);
>>   #define FEC_MMFR_TA            (2<<  16)
>>   #define FEC_MMFR_DATA(v)       (v&  0xffff)
>>
>> -#define FEC_MII_TIMEOUT                10000
>> +#define FEC_MII_TIMEOUT                10
>>
>>   /* Transmitter timeout */
>>   #define TX_TIMEOUT (2 * HZ)
>> @@ -611,13 +611,29 @@ spin_unlock:
>>   /*
>>    * NOTE: a MII transaction is during around 25 us, so polling it...
>>    */
>> -static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>> +static int fec_enet_mdio_poll(struct fec_enet_private *fep)
>>   {
>> -       struct fec_enet_private *fep = bus->priv;
>>          int timeout = FEC_MII_TIMEOUT;
>>
>>          fep->mii_timeout = 0;
>>
>> +       /* wait for end of transfer */
>> +       while (!(readl(fep->hwp + FEC_IEVENT)&  FEC_ENET_MII)) {
>> +               msleep(1);
>> +               if (timeout--<  0) {
>> +                       fep->mii_timeout = 1;
>> +                       break;
>> +               }
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>> +{
>> +       struct fec_enet_private *fep = bus->priv;
>> +
>> +
>>          /* clear MII end of transfer bit*/
>>          writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
>>
>> @@ -626,15 +642,7 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>>                  FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(regnum) |
>>                  FEC_MMFR_TA, fep->hwp + FEC_MII_DATA);
>>
>> -       /* wait for end of transfer */
>> -       while (!(readl(fep->hwp + FEC_IEVENT)&  FEC_ENET_MII)) {
>> -               cpu_relax();
>> -               if (timeout--<  0) {
>> -                       fep->mii_timeout = 1;
>> -                       printk(KERN_ERR "FEC: MDIO read timeout\n");
>> -                       return -ETIMEDOUT;
>> -               }
>> -       }
>> +       fec_enet_mdio_poll(fep);
>>
>>          /* return value */
>>          return FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
>> @@ -644,9 +652,6 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
>>                             u16 value)
>>   {
>>          struct fec_enet_private *fep = bus->priv;
>> -       int timeout = FEC_MII_TIMEOUT;
>> -
>> -       fep->mii_timeout = 0;
>>
>>          /* clear MII end of transfer bit*/
>>          writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
>> @@ -657,15 +662,7 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
>>                  FEC_MMFR_TA | FEC_MMFR_DATA(value),
>>                  fep->hwp + FEC_MII_DATA);
>>
>> -       /* wait for end of transfer */
>> -       while (!(readl(fep->hwp + FEC_IEVENT)&  FEC_ENET_MII)) {
>> -               cpu_relax();
>> -               if (timeout--<  0) {
>> -                       fep->mii_timeout = 1;
>> -                       printk(KERN_ERR "FEC: MDIO write timeout\n");
>> -                       return -ETIMEDOUT;
>> -               }
>> -       }
>> +       fec_enet_mdio_poll(fep);
>>
>>          return 0;
>>   }
>> --
>> 1.7.0.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

Re: [PATCH 1/2] netdev/fec: fix performance impact from mdio poll operation

[Andy Fleming]

On Saturday, May 8, 2010, Bryan Wu <bryan.wu@canonical.com> wrote:
> On 05/08/2010 12:06 AM, Andy Fleming wrote:

> From the BugLink, we experienced a lot of error
>
> The Phy lib is not going to respond well if an mdio
> write times out without returning an error.  And returning an error
> means the whole state machine has to reset, as a failed write is not
> something we currently handle gracefully.
>
>
>
> Right, if we return error to phylib due to mdio read times out, the performance will drop a lot.


Yes, and if you *don't* return an error, you risk incorrect behavior.
This patch changes the driver to return the read result, after
detecting that the read result is invalid.  I don't know what the
value in that register will be, but it can't be correct.  If the
hardware is broken, then we need to find a workaround that doesn't
break things for chips which have working mdio.  It's also possible we
just need a longer timeout.  Does the mdio bus ever return correct
values?




>
>
> Is it possible to use an interrupt to get the phy state change?  This
> would allow the phy lib to stop its incessant polling.  It doesn't
> solve the question of why it's timing out, though.
>
>
>
> I'm going to try the interrupt, but fec driver is shared by several SoCs. It might be a little complicated and still cannot fix the timing out issue. Is there any example driver to use an interrupt to get the phy state change?

Well, there are examples, but they may be only somewhat helpful.  From
the perspective of the PHY Lib, all you have to do is put a correct
value into the irqs array in the mdio bus struct for your bus.  The
part where you will probably have to solve a problem is in figuring
out how to convey that information to the bus driver.  On Power CPUs
and socs, we use a device tree, which has an interrupt associated with
each PHY.  Look at wherever your mdio bus *device* is registered.
That's probably where you'll have that information.

Feel free to look at drivers/net/fsl_pq_mdio.c

Another possibility is to use the fixed link phy driver, which will
never attempt to use the mdio bus, and won't poll.

Re: [PATCH 1/2] netdev/fec: fix performance impact from mdio poll operation

[Andy Fleming]

On Saturday, May 8, 2010, Bryan Wu <bryan.wu@canonical.com> wrote:
> On 05/08/2010 12:06 AM, Andy Fleming wrote:

> From the BugLink, we experienced a lot of error
>
> The Phy lib is not going to respond well if an mdio
> write times out without returning an error.  And returning an error
> means the whole state machine has to reset, as a failed write is not
> something we currently handle gracefully.
>
>
>
> Right, if we return error to phylib due to mdio read times out, the performance will drop a lot.


Yes, and if you *don't* return an error, you risk incorrect behavior.
This patch changes the driver to return the read result, after
detecting that the read result is invalid.  I don't know what the
value in that register will be, but it can't be correct.  If the
hardware is broken, then we need to find a workaround that doesn't
break things for chips which have working mdio.  It's also possible we
just need a longer timeout.  Does the mdio bus ever return correct
values?




>
>
> Is it possible to use an interrupt to get the phy state change?  This
> would allow the phy lib to stop its incessant polling.  It doesn't
> solve the question of why it's timing out, though.
>
>
>
> I'm going to try the interrupt, but fec driver is shared by several SoCs. It might be a little complicated and still cannot fix the timing out issue. Is there any example driver to use an interrupt to get the phy state change?

Well, there are examples, but they may be only somewhat helpful.  From
the perspective of the PHY Lib, all you have to do is put a correct
value into the irqs array in the mdio bus struct for your bus.  The
part where you will probably have to solve a problem is in figuring
out how to convey that information to the bus driver.  On Power CPUs
and socs, we use a device tree, which has an interrupt associated with
each PHY.  Look at wherever your mdio bus *device* is registered.
That's probably where you'll have that information.

Feel free to look at drivers/net/fsl_pq_mdio.c

Another possibility is to use the fixed link phy driver, which will
never attempt to use the mdio bus, and won't poll.

Re: [PATCH 1/2] netdev/fec: fix performance impact from mdio poll operation

[David Miller]

From: Bryan Wu <bryan.wu@canonical.com>
Date: Fri,  7 May 2010 10:27:18 +0800

> BugLink: http://bugs.launchpad.net/bugs/546649
> BugLink: http://bugs.launchpad.net/bugs/457878
> 
> After introducing phylib supporting, users experienced performace drop. That is
> because of the mdio polling operation of phylib. Use msleep to replace the busy
> waiting cpu_relax() and remove the warning message.
> 
> Signed-off-by: Bryan Wu <bryan.wu@canonical.com>
> Acked-by: Andy Whitcroft <apw@canonical.com>

As you've already been told, making these MDIO interfaces fail silently
is not acceptable.

Please fix this bug properly and resubmit this patch series.

Thanks.

Re: [PATCH 1/2] netdev/fec: fix performance impact from mdio poll operation

[Bryan Wu]

On 05/16/2010 03:28 PM, David Miller wrote:
> From: Bryan Wu <bryan.wu@canonical.com>
> Date: Fri,  7 May 2010 10:27:18 +0800
> 
>> BugLink: http://bugs.launchpad.net/bugs/546649
>> BugLink: http://bugs.launchpad.net/bugs/457878
>>
>> After introducing phylib supporting, users experienced performace drop. That is
>> because of the mdio polling operation of phylib. Use msleep to replace the busy
>> waiting cpu_relax() and remove the warning message.
>>
>> Signed-off-by: Bryan Wu <bryan.wu@canonical.com>
>> Acked-by: Andy Whitcroft <apw@canonical.com>
> 
> As you've already been told, making these MDIO interfaces fail silently
> is not acceptable.
> 
> Please fix this bug properly and resubmit this patch series.
> 

So sorry for the delay. My board's broken for several weeks. After I got a new,
I will try to fix this and resubmit this patch.

Thanks
-- 
Bryan Wu <bryan.wu@canonical.com>
Kernel Developer    +86.138-1617-6545 Mobile
Ubuntu Kernel Team | Hardware Enablement Team
Canonical Ltd.      www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com

[PATCH 2/2] netdev/fec: fix ifconfig eth0 down hang issue

[Bryan Wu]

BugLink: http://bugs.launchpad.net/bugs/559065

In fec open/close function, we need to use phy_connect and phy_disconnect
operation before we start/stop phy. Otherwise it will cause system hang.

Only call fec_enet_mii_probe() in open function, because the first open
action will cause NULL pointer error.

Signed-off-by: Bryan Wu <bryan.wu@canonical.com>
---
 drivers/net/fec.c |   28 ++++++++++++++++------------
 1 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index 9c58f6b..af4243f 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -678,6 +678,8 @@ static int fec_enet_mii_probe(struct net_device *dev)
 	struct phy_device *phy_dev = NULL;
 	int phy_addr;
 
+	fep->phy_dev = NULL;
+
 	/* find the first phy */
 	for (phy_addr = 0; phy_addr < PHY_MAX_ADDR; phy_addr++) {
 		if (fep->mii_bus->phy_map[phy_addr]) {
@@ -708,6 +710,11 @@ static int fec_enet_mii_probe(struct net_device *dev)
 	fep->link = 0;
 	fep->full_duplex = 0;
 
+	printk(KERN_INFO "%s: Freescale FEC PHY driver [%s] "
+		"(mii_bus:phy_addr=%s, irq=%d)\n", dev->name,
+		fep->phy_dev->drv->name, dev_name(&fep->phy_dev->dev),
+		fep->phy_dev->irq);
+
 	return 0;
 }
 
@@ -753,13 +760,8 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 	if (mdiobus_register(fep->mii_bus))
 		goto err_out_free_mdio_irq;
 
-	if (fec_enet_mii_probe(dev) != 0)
-		goto err_out_unregister_bus;
-
 	return 0;
 
-err_out_unregister_bus:
-	mdiobus_unregister(fep->mii_bus);
 err_out_free_mdio_irq:
 	kfree(fep->mii_bus->irq);
 err_out_free_mdiobus:
@@ -912,7 +914,12 @@ fec_enet_open(struct net_device *dev)
 	if (ret)
 		return ret;
 
-	/* schedule a link state check */
+	/* Probe and connect to PHY when open the interface */
+	ret = fec_enet_mii_probe(dev);
+	if (ret) {
+		fec_enet_free_buffers(dev);
+		return ret;
+	}
 	phy_start(fep->phy_dev);
 	netif_start_queue(dev);
 	fep->opened = 1;
@@ -926,10 +933,12 @@ fec_enet_close(struct net_device *dev)
 
 	/* Don't know what to do yet. */
 	fep->opened = 0;
-	phy_stop(fep->phy_dev);
 	netif_stop_queue(dev);
 	fec_stop(dev);
 
+	if (fep->phy_dev)
+		phy_disconnect(fep->phy_dev);
+
         fec_enet_free_buffers(dev);
 
 	return 0;
@@ -1293,11 +1302,6 @@ fec_probe(struct platform_device *pdev)
 	if (ret)
 		goto failed_register;
 
-	printk(KERN_INFO "%s: Freescale FEC PHY driver [%s] "
-		"(mii_bus:phy_addr=%s, irq=%d)\n", ndev->name,
-		fep->phy_dev->drv->name, dev_name(&fep->phy_dev->dev),
-		fep->phy_dev->irq);
-
 	return 0;
 
 failed_register:
-- 
1.7.0.1

Re: [PATCH 2/2] netdev/fec: fix ifconfig eth0 down hang issue

[Bryan Wu]

Since this patch is for another bug and doesn't depend on my first one patch.
Could you please review this?

Thanks,
-Bryan

On 05/07/2010 10:27 AM, Bryan Wu wrote:
> BugLink: http://bugs.launchpad.net/bugs/559065
> 
> In fec open/close function, we need to use phy_connect and phy_disconnect
> operation before we start/stop phy. Otherwise it will cause system hang.
> 
> Only call fec_enet_mii_probe() in open function, because the first open
> action will cause NULL pointer error.
> 
> Signed-off-by: Bryan Wu <bryan.wu@canonical.com>
> ---
>  drivers/net/fec.c |   28 ++++++++++++++++------------
>  1 files changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/fec.c b/drivers/net/fec.c
> index 9c58f6b..af4243f 100644
> --- a/drivers/net/fec.c
> +++ b/drivers/net/fec.c
> @@ -678,6 +678,8 @@ static int fec_enet_mii_probe(struct net_device *dev)
>  	struct phy_device *phy_dev = NULL;
>  	int phy_addr;
>  
> +	fep->phy_dev = NULL;
> +
>  	/* find the first phy */
>  	for (phy_addr = 0; phy_addr < PHY_MAX_ADDR; phy_addr++) {
>  		if (fep->mii_bus->phy_map[phy_addr]) {
> @@ -708,6 +710,11 @@ static int fec_enet_mii_probe(struct net_device *dev)
>  	fep->link = 0;
>  	fep->full_duplex = 0;
>  
> +	printk(KERN_INFO "%s: Freescale FEC PHY driver [%s] "
> +		"(mii_bus:phy_addr=%s, irq=%d)\n", dev->name,
> +		fep->phy_dev->drv->name, dev_name(&fep->phy_dev->dev),
> +		fep->phy_dev->irq);
> +
>  	return 0;
>  }
>  
> @@ -753,13 +760,8 @@ static int fec_enet_mii_init(struct platform_device *pdev)
>  	if (mdiobus_register(fep->mii_bus))
>  		goto err_out_free_mdio_irq;
>  
> -	if (fec_enet_mii_probe(dev) != 0)
> -		goto err_out_unregister_bus;
> -
>  	return 0;
>  
> -err_out_unregister_bus:
> -	mdiobus_unregister(fep->mii_bus);
>  err_out_free_mdio_irq:
>  	kfree(fep->mii_bus->irq);
>  err_out_free_mdiobus:
> @@ -912,7 +914,12 @@ fec_enet_open(struct net_device *dev)
>  	if (ret)
>  		return ret;
>  
> -	/* schedule a link state check */
> +	/* Probe and connect to PHY when open the interface */
> +	ret = fec_enet_mii_probe(dev);
> +	if (ret) {
> +		fec_enet_free_buffers(dev);
> +		return ret;
> +	}
>  	phy_start(fep->phy_dev);
>  	netif_start_queue(dev);
>  	fep->opened = 1;
> @@ -926,10 +933,12 @@ fec_enet_close(struct net_device *dev)
>  
>  	/* Don't know what to do yet. */
>  	fep->opened = 0;
> -	phy_stop(fep->phy_dev);
>  	netif_stop_queue(dev);
>  	fec_stop(dev);
>  
> +	if (fep->phy_dev)
> +		phy_disconnect(fep->phy_dev);
> +
>          fec_enet_free_buffers(dev);
>  
>  	return 0;
> @@ -1293,11 +1302,6 @@ fec_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto failed_register;
>  
> -	printk(KERN_INFO "%s: Freescale FEC PHY driver [%s] "
> -		"(mii_bus:phy_addr=%s, irq=%d)\n", ndev->name,
> -		fep->phy_dev->drv->name, dev_name(&fep->phy_dev->dev),
> -		fep->phy_dev->irq);
> -
>  	return 0;
>  
>  failed_register:

Re: [PATCH 2/2] netdev/fec: fix ifconfig eth0 down hang issue

[David Miller]

From: Bryan Wu <bryan.wu@canonical.com>
Date: Fri, 28 May 2010 13:26:42 +0800

> Since this patch is for another bug and doesn't depend on my first one patch.
> Could you please review this?

Resubmit it as a formal, new, seperate patch submission and it will
get looked at.

Re: [PATCH 2/2] netdev/fec: fix ifconfig eth0 down hang issue

[Bryan Wu]

On 05/28/2010 03:48 PM, David Miller wrote:
> From: Bryan Wu <bryan.wu@canonical.com>
> Date: Fri, 28 May 2010 13:26:42 +0800
> 
>> Since this patch is for another bug and doesn't depend on my first one patch.
>> Could you please review this?
> 
> Resubmit it as a formal, new, seperate patch submission and it will
> get looked at.

Okay, will do soon.

Thanks
-Bryan