Re: [RFC PATCH v3 net-next 2/6] net: phylink: introduce a generic method for querying PHY in-band autoneg capability

From: Vladimir Oltean <vladimir.oltean@nxp.com>
To: "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	Antoine Tenart <atenart@kernel.org>,
	Michael Walle <michael@walle.cc>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Ioana Ciornei <ioana.ciornei@nxp.com>,
	Maxim Kochetkov <fido_max@inbox.ru>,
	Bjarni Jonasson <bjarni.jonasson@microchip.com>,
	Steen Hegelund <steen.hegelund@microchip.com>,
	"UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>,
	"bcm-kernel-feedback-list@broadcom.com" 
	<bcm-kernel-feedback-list@broadcom.com>,
	Nicolas Ferre <nicolas.ferre@microchip.com>,
	Claudiu Beznea <claudiu.beznea@microchip.com>
Subject: Re: [RFC PATCH v3 net-next 2/6] net: phylink: introduce a generic method for querying PHY in-band autoneg capability
Date: Wed, 22 Sep 2021 21:48:28 +0000	[thread overview]
Message-ID: <20210922214827.wczsgk3yw3vjsv5w@skbuf> (raw)
In-Reply-To: <20210922213116.7wlvnjfeqjltiecs@skbuf>

On Thu, Sep 23, 2021 at 12:31:16AM +0300, Vladimir Oltean wrote:
> On Wed, Sep 22, 2021 at 10:22:19PM +0100, Russell King (Oracle) wrote:
> > On Wed, Sep 22, 2021 at 09:14:42PM +0300, Vladimir Oltean wrote:
> > > +static unsigned int phylink_fixup_inband_aneg(struct phylink *pl,
> > > +					      struct phy_device *phy,
> > > +					      unsigned int mode)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = phy_validate_inband_aneg(phy, pl->link_interface);
> > > +	if (ret == PHY_INBAND_ANEG_UNKNOWN) {
> > > +		phylink_dbg(pl,
> > > +			    "PHY driver does not report in-band autoneg capability, assuming %s\n",
> > > +			    phylink_autoneg_inband(mode) ? "true" : "false");
> > > +
> > > +		return mode;
> > > +	}
> > > +
> > > +	if (phylink_autoneg_inband(mode) && !(ret & PHY_INBAND_ANEG_ON)) {
> > > +		phylink_err(pl,
> > > +			    "Requested in-band autoneg but driver does not support this, disabling it.\n");
> > 
> > If we add support to the BCM84881 driver to work with
> > phy_validate_inband_aneg(), then this will always return
> > PHY_INBAND_ANEG_OFF and never PHY_INBAND_ANEG_ON. Consequently,
> > this will always produce this "error". It is not an error in the
> > SFP case, but it is if firmware is misconfigured.
> > 
> > So, this needs better handling - we should not be issuing an error-
> > level kernel message for something that is "normal".
> 
> Is this better?
> 
> 		phylink_printk(phy_on_sfp(phy) ? KERN_DEBUG : KERN_ERR, pl,
> 			       "Requested in-band autoneg but driver does not support this, disabling it.\n");

Ah, not sure whether that was a trick question or not, but
phylink_fixup_inband_aneg function does not get called for the SFP code
path, I even noted this in the commit message but forgot:

|   So if the 3 code paths:
|   - phylink_sfp_config
|   - phylink_connect_phy
|   - phylink_fwnode_phy_connect
|
|   do more or less the same thing (adapt pl->cur_link_an_mode based on the
|   capability reported by the PHY), the intention is different. With SFP
|   modules this behavior is absolutely to be expected, and pl->cfg_link_an_mode
|   only denotes the initial operating mode. On the other hand, when the PHY
|   is on-board, the initial link AN mode should ideally also be the final
|   one. So the implementations for the three are different.

That's why phy_validate_inband_aneg is called twice, once in
phylink_sfp_config and once for the on-board case.