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

[Chase Douglas]

On Thu, Apr 8, 2010 at 2:44 AM, Bryan Wu <bryan.wu at 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.

Is this performance drop in terms of overall system performance
because of the cpu_relax loop, or network/driver performance? I'm
assuming the former, but I want to make sure.

I was going to ask whether this was safe due to the potential for
msleep being run in an interrupt context. However, I found this
comment which calls the read() function that is reassuring:

http://lxr.linux.no/linux+v2.6.33/drivers/net/phy/mdio_bus.c#L201

Although things look ok to me, I don't have enough insight into this
driver to be able to give an Ack.

-- Chase

> Signed-off-by: Bryan Wu <bryan.wu at 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 53240d3..2280373 100644
> --- a/drivers/net/fec.c
> +++ b/drivers/net/fec.c
> @@ -216,7 +216,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)
> @@ -624,13 +624,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);
>
> @@ -639,15 +655,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));
> @@ -657,9 +665,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);
> @@ -670,15 +675,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