lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <VI1PR0402MB3600B82EE105E43BD20E2190FFDA0@VI1PR0402MB3600.eurprd04.prod.outlook.com>
Date:   Tue, 14 Apr 2020 03:07:09 +0000
From:   Andy Duan <fugang.duan@....com>
To:     Andrew Lunn <andrew@...n.ch>, David Miller <davem@...emloft.net>
CC:     netdev <netdev@...r.kernel.org>,
        Chris Healy <Chris.Healy@....aero>,
        Chris Heally <cphealy@...il.com>
Subject: RE: [EXT] [PATCH] net: ethernet: fec: Replace interrupt driven MDIO
 with polled IO

From: Andrew Lunn <andrew@...n.ch> Sent: Tuesday, April 14, 2020 8:46 AM
> Measurements of the MDIO bus have shown that driving the MDIO bus using
> interrupts is slow. Back to back MDIO transactions take about 90uS, with
> 25uS spent performing the transaction, and the remainder of the time the bus
> is idle.
> 
> Replacing the completion interrupt with polled IO results in back to back
> transactions of 40uS. The polling loop waiting for the hardware to complete
> the transaction takes around 27uS. Which suggests interrupt handling has an
> overhead of 50uS, and polled IO nearly halves this overhead, and doubles the
> MDIO performance.
> 

Although the mdio performance is better, but polling IO by reading register
cause system/bus loading more heavy.

So I don't think it is valuable to change interrupt mode to polling mode.

Thanks,
Andy
> Suggested-by: Chris Heally <cphealy@...il.com>
> Signed-off-by: Andrew Lunn <andrew@...n.ch>
> ---
>  drivers/net/ethernet/freescale/fec.h      |  4 +-
>  drivers/net/ethernet/freescale/fec_main.c | 69 ++++++++++++-----------
>  2 files changed, 37 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h
> b/drivers/net/ethernet/freescale/fec.h
> index bd898f5b4da5..4c8e7f57957a 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -376,8 +376,7 @@ struct bufdesc_ex {
>  #define FEC_ENET_TS_AVAIL       ((uint)0x00010000)
>  #define FEC_ENET_TS_TIMER       ((uint)0x00008000)
> 
> -#define FEC_DEFAULT_IMASK (FEC_ENET_TXF | FEC_ENET_RXF |
> FEC_ENET_MII) -#define FEC_NAPI_IMASK FEC_ENET_MII
> +#define FEC_DEFAULT_IMASK (FEC_ENET_TXF | FEC_ENET_RXF)
>  #define FEC_RX_DISABLED_IMASK (FEC_DEFAULT_IMASK &
> (~FEC_ENET_RXF))
> 
>  /* ENET interrupt coalescing macro define */ @@ -537,7 +536,6 @@ struct
> fec_enet_private {
>         int     link;
>         int     full_duplex;
>         int     speed;
> -       struct  completion mdio_done;
>         int     irq[FEC_IRQ_NUM];
>         bool    bufdesc_ex;
>         int     pause_flag;
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index c1c267b61647..48ac0a3a4eb0 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -938,8 +938,8 @@ fec_restart(struct net_device *ndev)
>         writel((__force u32)cpu_to_be32(temp_mac[1]),
>                fep->hwp + FEC_ADDR_HIGH);
> 
> -       /* Clear any outstanding interrupt. */
> -       writel(0xffffffff, fep->hwp + FEC_IEVENT);
> +       /* Clear any outstanding interrupt, except MDIO. */
> +       writel((0xffffffff & ~FEC_ENET_MII), fep->hwp + FEC_IEVENT);
> 
>         fec_enet_bd_init(ndev);
> 
> @@ -1085,7 +1085,7 @@ fec_restart(struct net_device *ndev)
>         if (fep->link)
>                 writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
>         else
> -               writel(FEC_ENET_MII, fep->hwp + FEC_IMASK);
> +               writel(0, fep->hwp + FEC_IMASK);
> 
>         /* Init the interrupt coalescing */
>         fec_enet_itr_coal_init(ndev);
> @@ -1599,6 +1599,10 @@ fec_enet_interrupt(int irq, void *dev_id)
>         irqreturn_t ret = IRQ_NONE;
> 
>         int_events = readl(fep->hwp + FEC_IEVENT);
> +
> +       /* Don't clear MDIO events, we poll for those */
> +       int_events &= ~FEC_ENET_MII;
> +
>         writel(int_events, fep->hwp + FEC_IEVENT);
>         fec_enet_collect_events(fep, int_events);
> 
> @@ -1606,16 +1610,12 @@ fec_enet_interrupt(int irq, void *dev_id)
>                 ret = IRQ_HANDLED;
> 
>                 if (napi_schedule_prep(&fep->napi)) {
> -                       /* Disable the NAPI interrupts */
> -                       writel(FEC_NAPI_IMASK, fep->hwp +
> FEC_IMASK);
> +                       /* Disable interrupts */
> +                       writel(0, fep->hwp + FEC_IMASK);
>                         __napi_schedule(&fep->napi);
>                 }
>         }
> 
> -       if (int_events & FEC_ENET_MII) {
> -               ret = IRQ_HANDLED;
> -               complete(&fep->mdio_done);
> -       }
>         return ret;
>  }
> 
> @@ -1765,11 +1765,26 @@ static void fec_enet_adjust_link(struct
> net_device *ndev)
>                 phy_print_status(phy_dev);  }
> 
> +static int fec_enet_mdio_wait(struct fec_enet_private *fep) {
> +       int retries = 10000;
> +       uint ievent;
> +
> +       while (retries--) {
> +               ievent = readl(fep->hwp + FEC_IEVENT);
> +               if (ievent & FEC_ENET_MII) {
> +                       writel(FEC_ENET_MII, fep->hwp +
> FEC_IEVENT);
> +                       return 0;
> +               }
> +       }
> +
> +       return -ETIMEDOUT;
> +}
> +
>  static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)  {
>         struct fec_enet_private *fep = bus->priv;
>         struct device *dev = &fep->pdev->dev;
> -       unsigned long time_left;
>         int ret = 0, frame_start, frame_addr, frame_op;
>         bool is_c45 = !!(regnum & MII_ADDR_C45);
> 
> @@ -1777,8 +1792,6 @@ static int fec_enet_mdio_read(struct mii_bus *bus,
> int mii_id, int regnum)
>         if (ret < 0)
>                 return ret;
> 
> -       reinit_completion(&fep->mdio_done);
> -
>         if (is_c45) {
>                 frame_start = FEC_MMFR_ST_C45;
> 
> @@ -1790,11 +1803,9 @@ static int fec_enet_mdio_read(struct mii_bus
> *bus, int mii_id, int regnum)
>                        fep->hwp + FEC_MII_DATA);
> 
>                 /* wait for end of transfer */
> -               time_left =
> wait_for_completion_timeout(&fep->mdio_done,
> -                               usecs_to_jiffies(FEC_MII_TIMEOUT));
> -               if (time_left == 0) {
> +               ret = fec_enet_mdio_wait(fep);
> +               if (ret) {
>                         netdev_err(fep->netdev, "MDIO address write
> timeout\n");
> -                       ret = -ETIMEDOUT;
>                         goto out;
>                 }
> 
> @@ -1813,11 +1824,9 @@ static int fec_enet_mdio_read(struct mii_bus
> *bus, int mii_id, int regnum)
>                 FEC_MMFR_TA, fep->hwp + FEC_MII_DATA);
> 
>         /* wait for end of transfer */
> -       time_left = wait_for_completion_timeout(&fep->mdio_done,
> -                       usecs_to_jiffies(FEC_MII_TIMEOUT));
> -       if (time_left == 0) {
> +       ret = fec_enet_mdio_wait(fep);
> +       if (ret) {
>                 netdev_err(fep->netdev, "MDIO read timeout\n");
> -               ret = -ETIMEDOUT;
>                 goto out;
>         }
> 
> @@ -1835,7 +1844,6 @@ static int fec_enet_mdio_write(struct mii_bus *bus,
> int mii_id, int regnum,  {
>         struct fec_enet_private *fep = bus->priv;
>         struct device *dev = &fep->pdev->dev;
> -       unsigned long time_left;
>         int ret, frame_start, frame_addr;
>         bool is_c45 = !!(regnum & MII_ADDR_C45);
> 
> @@ -1845,8 +1853,6 @@ static int fec_enet_mdio_write(struct mii_bus *bus,
> int mii_id, int regnum,
>         else
>                 ret = 0;
> 
> -       reinit_completion(&fep->mdio_done);
> -
>         if (is_c45) {
>                 frame_start = FEC_MMFR_ST_C45;
> 
> @@ -1858,11 +1864,9 @@ static int fec_enet_mdio_write(struct mii_bus
> *bus, int mii_id, int regnum,
>                        fep->hwp + FEC_MII_DATA);
> 
>                 /* wait for end of transfer */
> -               time_left =
> wait_for_completion_timeout(&fep->mdio_done,
> -                       usecs_to_jiffies(FEC_MII_TIMEOUT));
> -               if (time_left == 0) {
> +               ret = fec_enet_mdio_wait(fep);
> +               if (ret) {
>                         netdev_err(fep->netdev, "MDIO address write
> timeout\n");
> -                       ret = -ETIMEDOUT;
>                         goto out;
>                 }
>         } else {
> @@ -1878,12 +1882,9 @@ static int fec_enet_mdio_write(struct mii_bus
> *bus, int mii_id, int regnum,
>                 fep->hwp + FEC_MII_DATA);
> 
>         /* wait for end of transfer */
> -       time_left = wait_for_completion_timeout(&fep->mdio_done,
> -                       usecs_to_jiffies(FEC_MII_TIMEOUT));
> -       if (time_left == 0) {
> +       ret = fec_enet_mdio_wait(fep);
> +       if (ret)
>                 netdev_err(fep->netdev, "MDIO write timeout\n");
> -               ret  = -ETIMEDOUT;
> -       }
> 
>  out:
>         pm_runtime_mark_last_busy(dev);
> @@ -2079,6 +2080,9 @@ static int fec_enet_mii_init(struct platform_device
> *pdev)
> 
>         writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> 
> +       /* Clear any pending transaction complete indication */
> +       writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> +
>         fep->mii_bus = mdiobus_alloc();
>         if (fep->mii_bus == NULL) {
>                 err = -ENOMEM;
> @@ -3583,7 +3587,6 @@ fec_probe(struct platform_device *pdev)
>                 fep->irq[i] = irq;
>         }
> 
> -       init_completion(&fep->mdio_done);
>         ret = fec_enet_mii_init(pdev);
>         if (ret)
>                 goto failed_mii_init;
> --
> 2.26.0

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ