| 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
| ||
|
Message-ID: <HE1PR0402MB2745E92A12EC41A973B7F6C9FFD70@HE1PR0402MB2745.eurprd04.prod.outlook.com>
Date: Sun, 19 Apr 2020 06:22:54 +0000
From: Andy Duan <fugang.duan@....com>
To: Chris Healy <cphealy@...il.com>
CC: Andrew Lunn <andrew@...n.ch>, David Miller <davem@...emloft.net>,
netdev <netdev@...r.kernel.org>,
Florian Fainelli <f.fainelli@...il.com>,
Heiner Kallweit <hkallweit1@...il.com>
Subject: RE: [EXT] [PATCH net-next v2 1/3] net: ethernet: fec: Replace
interrupt driven MDIO with polled IO
From: Chris Healy <cphealy@...il.com> Sent: Sunday, April 19, 2020 6:39 AM
> I did some profiling using an oscilloscope with my NXP Vybrid based platform
Vybrid platform is low-end platform, can you test the value on i.MX6/MX8 ?
I think i.MX6/MX8 add some sleep_us has no MDIO transaction performance drop.
> to see what different "sleep_us" values resulted in for start of MDIO to start
> of MDIO transaction times. Here's what I found:
>
> 0 - ~38us to ~40us
> 1 - ~48us to ~64us
> 2 - ~48us to ~64us
> 3 - ~48us to ~64us
> 4 - ~48us to ~64us
> 4 - ~48us to ~64us
> 5 - ~48us to ~64us
> 6 - ~48us to ~64us
> 7 - ~48us to ~64us
> 8 - ~48us to ~64us
> 9 - ~56us to ~88us
> 10 - ~56us to ~112us
>
> Basically, with the "sleep_us" value set to 0, I would get the shortest inter
> transaction times with a very low variance. Once I went to a non-zero value,
> the inter transaction time went up, as well as the variance, which I suppose
> makes sense....
>
>
> On Sat, Apr 18, 2020 at 6:55 AM Andy Duan <fugang.duan@....com> wrote:
> >
> > From: Andrew Lunn <andrew@...n.ch> Sent: Saturday, April 18, 2020 8:04
> > 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.
> > >
> > > 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 | 67
> > > ++++++++++++-----------
> > > 2 files changed, 35 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/freescale/fec.h
> > > b/drivers/net/ethernet/freescale/fec.h
> > > index e74dd1f86bba..a6cdd5b61921 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 */ @@ -543,7 +542,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 dc6f8763a5d4..6530829632b1 100644
> > > --- a/drivers/net/ethernet/freescale/fec_main.c
> > > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > > @@ -976,8 +976,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);
> > >
> > > @@ -1123,7 +1123,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); @@ -1652,6 +1652,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);
> > >
> > > @@ -1659,16 +1663,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;
> > > }
> > >
> > > @@ -1818,11 +1818,24 @@ 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) {
> > > + uint ievent;
> > > + int ret;
> > > +
> > > + ret = readl_poll_timeout(fep->hwp + FEC_IEVENT, ievent,
> > > + ievent & FEC_ENET_MII, 0,
> 30000);
> >
> > sleep X us between reads ?
> >
> > > +
> > > + if (!ret)
> > > + writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > 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);
> > >
> > > @@ -1830,8 +1843,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;
> > >
> > > @@ -1843,11 +1854,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;
> > > }
> > >
> > > @@ -1866,11 +1875,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;
> > > }
> > >
> > > @@ -1888,7 +1895,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);
> > >
> > > @@ -1898,8 +1904,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;
> > >
> > > @@ -1911,11 +1915,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 {
> > > @@ -1931,12 +1933,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); @@ -2132,6 +2131,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;
> > > @@ -3674,7 +3676,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.1
> >
Powered by blists - more mailing lists