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

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:   Wed, 22 Sep 2021 23:50:34 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Antoine Tenart <atenart@...nel.org>,
        Michael Walle <michael@...le.cc>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Maxim Kochetkov <fido_max@...ox.ru>,
        Bjarni Jonasson <bjarni.jonasson@...rochip.com>,
        Steen Hegelund <steen.hegelund@...rochip.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        "bcm-kernel-feedback-list@...adcom.com" 
        <bcm-kernel-feedback-list@...adcom.com>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Claudiu Beznea <claudiu.beznea@...rochip.com>
Subject: Re: [RFC PATCH v3 net-next 2/6] net: phylink: introduce a generic
 method for querying PHY in-band autoneg capability

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.

Not complaining that you haven't reviewed this in 24 days, we all have
other things to do, but, "rush"? I have genuinely forgotten the
implementation details of the patches already, and I don't have any medical
issues with the memory that I know of (or I may have forgotten about them too).

So let's say I want to strike a balance between not rushing reviewers
and being able to usefully respond to questions. What is a good waiting time?
It is generally accepted on netdev that if there is no reaction within 3
days, then "out of sight" becomes "out of mind".

Also, "that's what happens" => what? Some confusion, which gets clarified
over two email exchanges in a few tens of minutes? Is that so bad?