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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 12 Jan 2017 22:40:05 +0000
From:   "Kwok, WingMan" <w-kwok2@...com>
To:     Russell King - ARM Linux <linux@...linux.org.uk>
CC:     Andrew Lunn <andrew@...n.ch>,
        "Karicheri, Muralidharan" <m-karicheri2@...com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: Marvell Phy (1510) issue since v4.7 kernel



> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@...linux.org.uk]
> Sent: Thursday, January 12, 2017 4:42 PM
> To: Kwok, WingMan
> Cc: Andrew Lunn; Karicheri, Muralidharan; netdev@...r.kernel.org
> Subject: Re: Marvell Phy (1510) issue since v4.7 kernel
> 
> On Thu, Jan 12, 2017 at 09:31:27PM +0000, Kwok, WingMan wrote:
> >
> > > > I double checked that ours is actually a 88E1514. According
> > > > to Marvell's brief description, it seems that it does support
> > > > fiber.
> > > >
> > > > Reading page 18 reg 30 returns 6.
> > >
> > > O.K, that is consistent. Looking at the Marvell SDK:
> > >
> > > phy/Include/madApiDefs.h:#define    MAD_SUB_88E1510  4   /*
> 88E1510 */
> > > phy/Include/madApiDefs.h:#define    MAD_SUB_88E1518  5   /*
> 88E1518 */
> > > phy/Include/madApiDefs.h:#define    MAD_SUB_88E1512  6   /*
> > > 88E1512/88E1514 */
> > >
> > > I took another look at the patch adding Fibre support. The
> > > suspend/resume functions are wrong.
> > >
> > >         /* Suspend the fiber mode first */
> > >         if (!(phydev->supported & SUPPORTED_FIBRE)) {
> > >
> > > The ! should not be there. Does removing them both fix your
> problem?
> > >
> >
> > Hi Andrew,
> >
> > Thanks for taking the time to look into those patches. Yes we notice
> > the error in the suspend/resume functions also.
> >
> > 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)
> 
> See below.
> 
> > 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.
> 
> So what we need to do is to have a .shutdown function installed - but
> it's not that simple...
> 
> 	.mdiodrv = {
> 		.driver = {
> 			.shutdown = mv88e1512_shutdown,
> 		},
> 	},
> 
> in the appropriate phy_driver array element, and then have:
> 
> static void mv88e1512_shutdown(struct device *dev)
> {
> 	struct phy_device *phydev = to_phy_device(dev);
> 
> 	phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> }
> 
> which will restore the copper page on non-emergency reboots.  For
> emergency reboots, we're out of luck... the real fix would be for
> uboot to ensure that when it detects this PHY, it ensures that the
> right page is selected.
> 
> Now, that all said, there's a bug in this code, which is fixed by
> a patch I recently submitted.  If you have an 88E151x connected via
> SGMII, then the read_status function incorrectly believes it should
> be reading from the fiber page - when in fact the fiber page is
> reporting the status of the host-side link.  So, before trying the
> above modification, please try this patch - it's already been merged
> into the net tree, so should hit -rc soon.
> 
> My guess from what you've said above is that your 88E151x is connected
> via SGMII, so you need the patch below rather than trying to install
> a shutdown function.
> 
> 8<====
> From: Russell King <rmk+kernel@...linux.org.uk>
> Subject: [PATCH] net: phy: marvell: fix Marvell 88E1512 used in SGMII
> mode
> 

Hi Russell,

Thanks for the explanations. Yes, our 88e1514 is connected to the
host via sgmii and your patch does provide a solution to our problem.

Regards,
WingMan