| 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: <AM0PR0402MB36034E23EE0687DFC76C775AFF720@AM0PR0402MB3603.eurprd04.prod.outlook.com>
Date: Mon, 27 Jul 2020 02:21:34 +0000
From: Andy Duan <fugang.duan@....com>
To: Chris Healy <cphealy@...il.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>
CC: "David S. Miller" <davem@...emloft.net>,
netdev <netdev@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>,
Martin Fuzzey <martin.fuzzey@...wbird.group>
Subject: RE: [EXT] Re: [RESENT PATCH net--stat 1/1] net: ethernet: fec: Revert
"net: ethernet: fec: Replace interrupt driven MDIO with polled IO"
From: Chris Healy <cphealy@...il.com> Sent: Monday, July 27, 2020 10:13 AM
> On Sun, Jul 26, 2020 at 7:06 PM Laurent Pinchart
> <laurent.pinchart@...asonboard.com> wrote:
> >
> > On Mon, Jul 27, 2020 at 04:24:02AM +0300, Laurent Pinchart wrote:
> > > On Mon, Apr 27, 2020 at 10:08:04PM +0800, Fugang Duan wrote:
> > > > This reverts commit 29ae6bd1b0d8a57d7c00ab12cbb949fc41986eef.
> > > >
> > > > The commit breaks ethernet function on i.MX6SX, i.MX7D, i.MX8MM,
> > > > i.MX8MQ, and i.MX8QXP platforms. Boot yocto system by NFS mounting
> > > > rootfs will be failed with the commit.
> > >
> > > I'm afraid this commit breaks networking on i.MX7D for me :-( My
> > > board is configured to boot over NFS root with IP autoconfiguration
> > > through DHCP. The DHCP request goes out, the reply it sent back by
> > > the server, but never noticed by the fec driver.
> > >
> > > v5.7 works fine. As 29ae6bd1b0d8a57d7c00ab12cbb949fc41986eef was
> > > merged during the v5.8 merge window, I suspect something else
> > > cropped in between 29ae6bd1b0d8a57d7c00ab12cbb949fc41986eef and
> this
> > > patch that needs to be reverted too. We're close to v5.8 and it
> > > would be annoying to see this regression ending up in the released
> > > kernel. I can test patches, but I'm not familiar enough with the
> > > driver (or the networking
> > > subsystem) to fix the issue myself.
> >
> > If it can be of any help, I've confirmed that, to get the network back
> > to usable state from v5.8-rc6, I have to revert all patches up to this
> > one. This is the top of my branch, on top of v5.8-rc6:
> >
> > 5bbe80c9efea Revert "net: ethernet: fec: Revert "net: ethernet: fec: Replace
> interrupt driven MDIO with polled IO""
> > 5462896a08c1 Revert "net: ethernet: fec: Replace interrupt driven MDIO
> with polled IO"
> > 824a82e2bdfa Revert "net: ethernet: fec: move GPR register offset and bit
> into DT"
> > bfe330591cab Revert "net: fec: disable correct clk in the err path of
> fec_enet_clk_enable"
> > 109958cad578 Revert "net: ethernet: fec: prevent tx starvation under high rx
> load"
>
> I just fired up net-next on my i.MX7d based design (not NFS root though). I can
> bring up the network interface with a gigabit connection but ALL RX traffic is
> failing with CRC errors. Now, my design is using a Micrel KSZ9031 which might
> be part of the problem for me as there were some recent KSZ9031 changes
> made so take what I'm seeing with a grain of salt.
>
> Laurent, couple questions:
>
> 1) Are you able to boot without NFS root and communicate correctly or is this
> issue just when doing an NFS root?
> 2) If you are able to boot up without NFS root, can you check the ethtool
> statistics and see the same RX CRC errors I'm seeing?
>
>
Laurent, Chris,
I am trying imx7d sdb board with BCM54220 siwtch PHY on net-next now,
will update my test result ASAP.
Regards,
Fugang
> >
> > > > Signed-off-by: Fugang Duan <fugang.duan@....com>
> > > >
> > > > diff --git a/drivers/net/ethernet/freescale/fec.h
> > > > b/drivers/net/ethernet/freescale/fec.h
> > > > index a6cdd5b..e74dd1f 100644
> > > > --- a/drivers/net/ethernet/freescale/fec.h
> > > > +++ b/drivers/net/ethernet/freescale/fec.h
> > > > @@ -376,7 +376,8 @@ 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)
> > > > +#define FEC_DEFAULT_IMASK (FEC_ENET_TXF | FEC_ENET_RXF |
> FEC_ENET_MII)
> > > > +#define FEC_NAPI_IMASK FEC_ENET_MII
> > > > #define FEC_RX_DISABLED_IMASK (FEC_DEFAULT_IMASK &
> > > > (~FEC_ENET_RXF))
> > > >
> > > > /* ENET interrupt coalescing macro define */ @@ -542,6 +543,7 @@
> > > > 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 1ae075a..c7b84bb 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, except MDIO. */
> > > > - writel((0xffffffff & ~FEC_ENET_MII), fep->hwp + FEC_IEVENT);
> > > > + /* Clear any outstanding interrupt. */
> > > > + writel(0xffffffff, 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(0, fep->hwp + FEC_IMASK);
> > > > + writel(FEC_ENET_MII, fep->hwp + FEC_IMASK);
> > > >
> > > > /* Init the interrupt coalescing */
> > > > fec_enet_itr_coal_init(ndev);
> > > > @@ -1652,10 +1652,6 @@ 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);
> > > >
> > > > @@ -1663,12 +1659,16 @@ fec_enet_interrupt(int irq, void *dev_id)
> > > > ret = IRQ_HANDLED;
> > > >
> > > > if (napi_schedule_prep(&fep->napi)) {
> > > > - /* Disable interrupts */
> > > > - writel(0, fep->hwp + FEC_IMASK);
> > > > + /* Disable the NAPI interrupts */
> > > > + writel(FEC_NAPI_IMASK, fep->hwp +
> FEC_IMASK);
> > > > __napi_schedule(&fep->napi);
> > > > }
> > > > }
> > > >
> > > > + if (int_events & FEC_ENET_MII) {
> > > > + ret = IRQ_HANDLED;
> > > > + complete(&fep->mdio_done);
> > > > + }
> > > > return ret;
> > > > }
> > > >
> > > > @@ -1818,24 +1818,11 @@ 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_atomic(fep->hwp + FEC_IEVENT, ievent,
> > > > - ievent & FEC_ENET_MII, 2,
> 30000);
> > > > -
> > > > - 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);
> > > >
> > > > @@ -1843,6 +1830,8 @@ 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;
> > > >
> > > > @@ -1854,9 +1843,11 @@ 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 */
> > > > - ret = fec_enet_mdio_wait(fep);
> > > > - if (ret) {
> > > > + time_left =
> wait_for_completion_timeout(&fep->mdio_done,
> > > > + usecs_to_jiffies(FEC_MII_TIMEOUT));
> > > > + if (time_left == 0) {
> > > > netdev_err(fep->netdev, "MDIO address write
> > > > timeout\n");
> > > > + ret = -ETIMEDOUT;
> > > > goto out;
> > > > }
> > > >
> > > > @@ -1875,9 +1866,11 @@ 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 */
> > > > - ret = fec_enet_mdio_wait(fep);
> > > > - if (ret) {
> > > > + time_left = wait_for_completion_timeout(&fep->mdio_done,
> > > > + usecs_to_jiffies(FEC_MII_TIMEOUT));
> > > > + if (time_left == 0) {
> > > > netdev_err(fep->netdev, "MDIO read timeout\n");
> > > > + ret = -ETIMEDOUT;
> > > > goto out;
> > > > }
> > > >
> > > > @@ -1895,6 +1888,7 @@ 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);
> > > >
> > > > @@ -1904,6 +1898,8 @@ 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;
> > > >
> > > > @@ -1915,9 +1911,11 @@ 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 */
> > > > - ret = fec_enet_mdio_wait(fep);
> > > > - if (ret) {
> > > > + time_left =
> wait_for_completion_timeout(&fep->mdio_done,
> > > > + usecs_to_jiffies(FEC_MII_TIMEOUT));
> > > > + if (time_left == 0) {
> > > > netdev_err(fep->netdev, "MDIO address write
> > > > timeout\n");
> > > > + ret = -ETIMEDOUT;
> > > > goto out;
> > > > }
> > > > } else {
> > > > @@ -1933,9 +1931,12 @@ 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 */
> > > > - ret = fec_enet_mdio_wait(fep);
> > > > - if (ret)
> > > > + time_left = wait_for_completion_timeout(&fep->mdio_done,
> > > > + usecs_to_jiffies(FEC_MII_TIMEOUT));
> > > > + if (time_left == 0) {
> > > > netdev_err(fep->netdev, "MDIO write timeout\n");
> > > > + ret = -ETIMEDOUT;
> > > > + }
> > > >
> > > > out:
> > > > pm_runtime_mark_last_busy(dev); @@ -2144,9 +2145,6 @@ 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;
> > > > @@ -3688,6 +3686,7 @@ 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;
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
Powered by blists - more mailing lists