netdev - Re: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

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: <20200428133445.GA21352@lunn.ch>
Date:   Tue, 28 Apr 2020 15:34:45 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Andy Duan <fugang.duan@....com>
Cc:     Leonard Crestez <leonard.crestez@....com>,
        David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Chris Healy <Chris.Healy@....aero>,
        dl-linux-imx <linux-imx@....com>, Chris Healy <cphealy@...il.com>
Subject: Re: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven
 MDIO with polled IO

> Andrew, after investigate the issue, there have one MII event coming later then
> clearing MII pending event when writing MSCR register (MII_SPEED).
> 
> Check the rtl design by co-working with our IC designer, the MII event generation
> condition:
> - writing MSCR:
> 	- mmfr[31:0]_not_zero & mscr[7:0]_is_zero & mscr_reg_data_in[7:0] != 0
> - writing MMFR:
> 	- mscr[7:0]_not_zero
> 	
> mmfr[31:0]: current MMFR register value
> mscr[7:0]: current MSCR register value
> mscr_reg_data_in[7:0]: the value wrote to MSCR
> 
> 
> Below patch can fix the block issue:
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -2142,6 +2142,15 @@ static int fec_enet_mii_init(struct platform_device *pdev)
>         if (suppress_preamble)
>                 fep->phy_speed |= BIT(7);
> 
> +       /*
> +        * Clear MMFR to avoid to generate MII event by writing MSCR.
> +        * MII event generation condition:
> +        * - writing MSCR:
> +        *      - mmfr[31:0]_not_zero & mscr[7:0]_is_zero & mscr_reg_data_in[7:0] != 0
> +        * - writing MMFR:
> +        *      - mscr[7:0]_not_zero
> +        */
> +       writel(0, fep->hwp + FEC_MII_DATA);
>         writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);

Hi Andy

Thanks for digging into the internal of the FEC. Just to make sure i
understand this correctly:

In fec_enet_mii_init() we have:

        holdtime = DIV_ROUND_UP(clk_get_rate(fep->clk_ipg), 100000000) - 1;

        fep->phy_speed = mii_speed << 1 | holdtime << 8;

        writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);

        /* Clear any pending transaction complete indication */
        writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);

You are saying this write to the FEC_MII_SPEED register can on some
SoCs trigger an FEC_ENET_MII event. And because it does not happen
immediately, it happens after the clear which is performed here?
Sometime later we then go into fec_enet_mdio_wait(), the event is
still pending, so we read the FEC_MII_DATA register too early?

But this does not fully explain the problem. This should only affect
the first MDIO transaction, because as we exit fec_enet_mdio_wait()
the event is cleared. But Leonard reported that all reads return 0,
not just the first.

    Andrew