From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jose Abreu Subject: Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks Date: Wed, 24 Oct 2018 08:50:03 +0100 Message-ID: <5e65254c-04dc-6dba-eb1c-10bafadf95c5@synopsys.com> References: <20181022122839.GB24112@lunn.ch> <8114f345-0efb-9a64-b867-2cfe2fba09ab@synopsys.com> <20181022154851.GA1492@n2100.armlinux.org.uk> <943a6194-001a-d6a0-84ba-b93b728ce64b@synopsys.com> <20181023102023.GM30658@n2100.armlinux.org.uk> <20181023105828.GN30658@n2100.armlinux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Cc: Florian Fainelli , Andrew Lunn , , "David S. Miller" , Joao Pinto To: Russell King - ARM Linux , Jose Abreu Return-path: Received: from us01smtprelay-2.synopsys.com ([198.182.47.9]:38298 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726832AbeJXQRI (ORCPT ); Wed, 24 Oct 2018 12:17:08 -0400 In-Reply-To: <20181023105828.GN30658@n2100.armlinux.org.uk> Sender: netdev-owner@vger.kernel.org List-ID: On 23-10-2018 11:58, Russell King - ARM Linux wrote: > On Tue, Oct 23, 2018 at 11:28:09AM +0100, Jose Abreu wrote: >> On 23-10-2018 11:20, Russell King - ARM Linux wrote: >>> I have no idea what you're proposing there - your patches weren't copied >>> to me. >> They just set / unset MDIO_CTRL1_LPOWER bit in PCS. I find that >> without this remote end doesn't detect link is down ... >> >> If it's okay for Generic 10G driver I can submit only this and >> manually reset PHY in stmmac driver so that I don't need to >> implement custom PHY driver ... > > >> BTW, I just found out currently Generic 10G Driver is broken >> without patch 4/4 of this series [1] >> >> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_987570_&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=zYEDSMZPTCHiPc_3B8buyu0kXnlIEYawnHWAxPrsoSU&s=MlF6I2cBSYkGxgEwNV-hXpXJIvXv_gRYXP-CazjkUSw&e= > How is it broken - what are the symptoms? > > The generic 10G driver is bound not via the normal bus matching and > phy_bus_match(), but via a manual bind in phy_attach_direct(). This > calls the probe function, which is phy_probe(), which initialises > the supported/advertising to the driver's features (which as you note > are zero.) Since 719655a14971 ("net: phy: Replace phy driver features u32 with link_mode bitmap"), phy_probe() calls ethtool_convert_link_mode_to_legacy_u32() with phydrv->features as argument. Since features are NULL, we will get NULL pointer dereference. I guess Generic 10G driver was forgotten in the conversion. Thanks and Best Regards, Jose Miguel Abreu > > However, phy_attach_direct() goes on to call phy_init_hw(), which > calls the config_init() method. The config_init() method initialises > the supported/advertising masks to 10GbaseT. This is (partly) what > I refer to when I say that the generic 10G support is crippled - it > only supports this single speed and media. > > So the supported/advertising masks should be forced to only 10GbaseT > at the completion of phy_attach_direct(). > > The "generic 10G" support doesn't do autonegotiation, configuration > or link mode forcing. It only assumes 10GbaseT is supported, and > only checks for the "link up" bits. > > It isn't like the non-10G generic PHY support due to history - it > was added in 2014 by Andy Fleming (see 124059fd53af). > > BTW, your patch 1 is wrong as well (introducing phy_update_link()). > You don't take account that a 10G phy may have alternative ways of > reading the link (like 88x3310 does, because it has multiple > instances of AN/PCS/PHYXS at 1k offsets.) All the gen10g_* > functions are legacy functions for the crippled "generic" 10G > support. >