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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Wed, Sep 22, 2021 at 11:50:34PM +0000, Vladimir Oltean wrote:
> On Thu, Sep 23, 2021 at 12:03:22AM +0100, Russell King (Oracle) wrote:
> > On Wed, Sep 22, 2021 at 09:48:28PM +0000, Vladimir Oltean wrote:
> > > 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:
> >
> > No it wasn't a trick question. I thought you were calling
> > phylink_fixup_inband_aneg() from phylink_sfp_config(), but I see now
> > that you don't. That's what happens when you try and rush to review.
> 
> How did I "rush to review" exactly? I waited for 24 days since the v2
> for even a single review comment, with even a ping in between, before
> resending the series largely unaltered, just with an extra patch appended.

FFS. Are you intentionally trying to misinterpret everything I say?
Who here is doing a review? You or me?

"That's what happens when you try and rush to review." is a form of
speech - clearly the "you" is not aimed at you Vladimir, but me.
Let's put this a different way.

I am blaming myself for rushing to review this last night.

Is that more clear for you?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!