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

[Bryan Wu]

On 05/08/2010 12:06 AM, Andy Fleming wrote:
> On Thursday, May 6, 2010, Bryan Wu<bryan.wu@xxxxxxxxxxxxx>  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@xxxxxxxxxxxxx>
> > Acked-by: Andy Whitcroft<apw@xxxxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/