netdev - RE: Marvell Phy (1510) issue since v4.7 kernel

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <230CBA6E4B6B6B418E8730AC28E6FC7E15354F22@DFLE11.ent.ti.com>
Date:   Thu, 12 Jan 2017 22:49:56 +0000
From:   "Kwok, WingMan" <w-kwok2@...com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "rmk+kernel@....linux.org.uk" <rmk+kernel@....linux.org.uk>,
        "Karicheri, Muralidharan" <m-karicheri2@...com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: Marvell Phy (1510) issue since v4.7 kernel

Hi Andrew,

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@...n.ch]
> Sent: Thursday, January 12, 2017 4:50 PM
> To: Kwok, WingMan
> Cc: rmk+kernel@....linux.org.uk; Karicheri, Muralidharan;
> netdev@...r.kernel.org
> Subject: Re: Marvell Phy (1510) issue since v4.7 kernel
> 
> > But our problem is caused by the read_status function:
> >
> > 	if ((phydev->supported & SUPPORTED_FIBRE)) {
> > 		err = phy_write(phydev, MII_MARVELL_PHY_PAGE,
> MII_M1111_FIBER);
> > 		if (err < 0)
> > 			goto error;
> >
> > 		err = marvell_read_status_page(phydev, MII_M1111_FIBER);
> > 		if (err < 0)
> > 			goto error;
> >
> > 		/* If the fiber link is up, it is the selected and used
> link.
> > 		* In this case, we need to stay in the fiber page.
> > 		* Please to be careful about that, avoid to restore Copper
> page
> > 		* in other functions which could break the behaviour
> > 		* for some fiber phy like 88E1512.
> > 		* */
> > 		if (phydev->link)
> > 			return 0;
> >
> > which keeps the fiber page if phydev->link is true (for some
> > reason this is the case even though we are not using fiber)
> 
> How are you using the PHY. What phy-mode do you have set?  Do you
> happen to be using it as an RGMII to SERDES/SGMII bridge? This is what
> Russell King is doing, i think.
> 

our 88e1514 is connected to the host via sgmii.

> Have you tried the patch Russell submitted recently.
> 
> Author: Russell King <rmk+kernel@...linux.org.uk>
> Date:   Tue Jan 10 23:13:45 2017 +0000
> 
>     net: phy: marvell: fix Marvell 88E1512 used in SGMII mode
> 
>     When an Marvell 88E1512 PHY is connected to a nic in SGMII mode,
> the
>     fiber page is used for the SGMII host-side connection.  The PHY
> driver
>     notices that SUPPORTED_FIBRE is set, so it tries reading the fiber
> page
>     for the link status, and ends up reading the MAC-side status
> instead of
>     the outgoing (copper) link.  This leads to incorrect results
> reported
>     via ethtool.
> 
>     If the PHY is connected via SGMII to the host, ignore the fiber
> page.
>     However, continue to allow the existing power management code to
>     suspend and resume the fiber page.
> 

Thanks for pointer. It does fix the problem.

> > However, this causes a problem in kernel reboot because neither
> > the suspend/resume is called to restore the copper page and
> > u-boot marvell phy driver does not support 1510 fiber, which
> > will then result in writing to the wrong phy regs and causes
> > a sgmii auto-nego time out.
> 
> This is still a u-boot bug. It should not assume the PHY is in a sane
> state. It should reset it and configure it as needed. So far, i don't
> think you have reported any issues with Linux usage of the PHY. There
> clearly are bugs, but your real problem is u-boot.
> 

Yes. Agree.

> > In addition to fixing the ! in suspend/resume, my suggestion
> > would be to change also the read_status function to
> > always restore the copper page after doing the fiber stuffs:
> 
> Nope. This is done deliberately, as the comment suggests:
> 
> > 		/* If the fiber link is up, it is the selected and used
> link.
> > 		* In this case, we need to stay in the fiber page.
> > 		* Please to be careful about that, avoid to restore Copper
> page
> > 		* in other functions which could break the behaviour
> > 		* for some fiber phy like 88E1512.
> > 		* */
> > 		if (phydev->link)
> > 			return 0;
> 
> The point is, the phylib will continue polling the PHY registers,
> reading them. If the FIBRE is up, we want to read the FIBRE values,
> not the copper.
> 

Thanks for the explanations.

> > Another issue is that, as of now, FIBER is enabled regardless
> > of the specific 88e151x. But I believe there is 88e151x chip(s)
> > that does not support fiber. Should fiber be enabled only for
> > those that do support fiber?
> 
> Yes, we should look at register 30, page 18 any set SUPPORTED_FIBRE
> based on that.
> 

Thanks for the suggestions.

>       Andrew

Just want to know if there is already a patch in the net tree
fixing the incorrect ! in the suspend/resume functions also?

WingMan