netdev - Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks

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: <20181022154851.GA1492@n2100.armlinux.org.uk>
Date:   Mon, 22 Oct 2018 16:48:51 +0100
From:   Russell King - ARM Linux <linux@...linux.org.uk>
To:     Jose Abreu <jose.abreu@...opsys.com>
Cc:     Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Joao Pinto <joao.pinto@...opsys.com>
Subject: Re: [PATCH net-next 3/4] net: phy-c45: Implement
 reset/suspend/resume callbacks

On Mon, Oct 22, 2018 at 01:47:48PM +0100, Jose Abreu wrote:
> Hello,
> 
> On 22-10-2018 13:28, Andrew Lunn wrote:
> >>  EXPORT_SYMBOL_GPL(gen10g_resume);
> >> @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = {
> >>  	.phy_id         = 0xffffffff,
> >>  	.phy_id_mask    = 0xffffffff,
> >>  	.name           = "Generic 10G PHY",
> >> -	.soft_reset	= gen10g_no_soft_reset,
> >> +	.soft_reset	= gen10g_soft_reset,
> >>  	.config_init    = gen10g_config_init,
> >>  	.features       = 0,
> >>  	.aneg_done	= genphy_c45_aneg_done,
> > Hi Jose
> >
> > You need to be careful here. There is a reason this is called
> > gen10g_no_soft_reset, rather than having an empty
> > gen10g_soft_reset. Some PHYs break when you do a reset.  So adding a
> > gen10g_soft_reset is fine, but don't change this here, without first
> > understanding the history, and talking to Russell King.
> 
> Hmm, the reset function only interacts with standard PCS
> registers, which should always be available ...
> 
> >From my tests I need to do at least 1 reset during power-up so in
> ultimate case I can add a feature quirk or similar.
> 
> Russell, can you please comment ?

Setting the reset bit on 88x3310 causes the entire device to become
completely inaccessible until hardware reset.  Therefore, this bit
must _never_ be set for these devices.  That said, we have a separate
driver for these PHYs, but that will only be used for them if it's
present in the kernel.  If we accidentally fall back to the generic
driver, then we'll screw the 88x3310 until a full hardware reset.

We also have a bunch of net devices that make use of this crippled
"generic" 10G support - we don't know whether resetting the PHY
for those systems will cause a regression - maybe board firmware
already configured the PHY?  I can't say either way on that, except
that we've had crippled 10G support in PHYLIB for a number of years
now _with_ users, and adding reset support drastically changes the
subsystem's behaviour for these users.

I would recommend not touching the generic 10G driver, but instead
implement your own driver for your PHY to avoid causing regressions.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up