netdev - RE: [EXT] [PATCH net-next] net: ethernet: fec: Prevent MII event after MII

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: <HE1PR0402MB27459F85A4F7DADB491B5505FFAD0@HE1PR0402MB2745.eurprd04.prod.outlook.com>
Date:   Wed, 29 Apr 2020 01:53:34 +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 <cphealy@...il.com>,
        Leonard Crestez <leonard.crestez@....com>
Subject: RE: [EXT] [PATCH net-next] net: ethernet: fec: Prevent MII event
 after MII_SPEED write

From: Andrew Lunn <andrew@...n.ch> Sent: Wednesday, April 29, 2020 1:59 AM
> The change to polled IO for MDIO completion assumes that MII events are
> only generated for MDIO transactions. However on some SoCs writing to the
> MII_SPEED register can also trigger an MII event. As a result, the next MDIO
> read has a pending MII event, and immediately reads the data registers
> before it contains useful data. When the read does complete, another MII
> event is posted, which results in the next read also going wrong, and the cycle
> continues.
> 
> By writing 0 to the MII_DATA register before writing to the speed register, this
> MII event for the MII_SPEED is suppressed, and polled IO works as expected.
> 
> Fixes: 29ae6bd1b0d8 ("net: ethernet: fec: Replace interrupt driven MDIO with
> polled IO")
> Reported-by: Andy Duan <fugang.duan@....com>
> Suggested-by: Andy Duan <fugang.duan@....com>
> Signed-off-by: Andrew Lunn <andrew@...n.ch>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 1ae075a246a3..aa5e744ec098 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -996,6 +996,9 @@ fec_restart(struct net_device *ndev)
>                 writel(0x0, fep->hwp + FEC_X_CNTRL);
>         }
> 
> +       /* Prevent an MII event being report when changing speed */
> +       writel(0, fep->hwp + FEC_MII_DATA);
> +

Andrew, my suggestion is only add the change in .fec_enet_mii_init().
There have risk and may introduce MII event here when wirte value
into FEC_MII_DATA register.


As I said, if FEC_MII_SPEED register[7:0] is not zero, MII event will be generated
when write FEC_MII_DATA register. 
        * - writing MMFR:
        *      - mscr[7:0]_not_zero

>         /* Set MII speed */
>         writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> 
> @@ -1182,6 +1185,10 @@ fec_stop(struct net_device *ndev)
>                 writel(val, fep->hwp + FEC_ECNTRL);
>                 fec_enet_stop_mode(fep, true);
>         }
> +
> +       /* Prevent an MII event being report when changing speed */
> +       writel(0, fep->hwp + FEC_MII_DATA);
> +

The same here.
We should not add the change here.

I already consider normal case, suspend/resume with power on case,
Suspend/resume with power off (register value lost) case, only add
the change in . fec_enet_mii_init() seems safe.
The patch "net: ethernet: fec: Prevent MII event after MII_SPEED write"
brings some trouble here, we need to consider all cases when writing
FEC_MII_DATA, FEC_MII_SPEED registers.

Again, please note writing the two registers may trigger MII event with
Below logic:
        * 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
        */

If interrupt mode, we don't take care this logic and dependency.

>         writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> 
>         /* We have to keep ENET enabled to have MII interrupt stay
> working */ @@ -2142,6 +2149,16 @@ 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);
> 
>         /* Clear any pending transaction complete indication */
> --
> 2.26.1