Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 23 May 2020 21:51:31 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     Russell King - ARM Linux admin <linux@armlinux.org.uk>
Cc:     Jeremy Linton <jeremy.linton@arm.com>, netdev@vger.kernel.org,
        davem@davemloft.net, f.fainelli@gmail.com, hkallweit1@gmail.com,
        madalin.bucur@oss.nxp.com, calvin.johnson@oss.nxp.com,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC 03/11] net: phy: refactor c45 phy identification sequence
Message-ID: <20200523195131.GN610998@lunn.ch>
References: <20200522213059.1535892-1-jeremy.linton@arm.com>
 <20200522213059.1535892-4-jeremy.linton@arm.com>
 <20200523183058.GX1551@shell.armlinux.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200523183058.GX1551@shell.armlinux.org.uk>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

> >  static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
> >  			   struct phy_c45_device_ids *c45_ids) {
> > -	int phy_reg;
> > -	int i, reg_addr;
> > +	int ret;
> > +	int i;
> >  	const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
> >  	u32 *devs = &c45_ids->devices_in_package;
> 
> I feel a "reverse christmas tree" complaint brewing... yes, the original
> code didn't follow it.  Maybe a tidy up while touching this code?

At minimum, a patch should not make it worse. ret and i should clearly
be after devs.

> >  static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id,
> >  		      bool is_c45, struct phy_c45_device_ids *c45_ids)
> >  {
> > -	int phy_reg;
> > +	int ret;
> >  
> >  	if (is_c45)
> >  		return get_phy_c45_ids(bus, addr, phy_id, c45_ids);
> >  
> > -	/* Grab the bits from PHYIR1, and put them in the upper half */
> > -	phy_reg = mdiobus_read(bus, addr, MII_PHYSID1);
> > -	if (phy_reg < 0) {
> > +	ret = _get_phy_id(bus, addr, 0, phy_id, false);
> > +	if (ret < 0) {
> >  		/* returning -ENODEV doesn't stop bus scanning */
> > -		return (phy_reg == -EIO || phy_reg == -ENODEV) ? -ENODEV : -EIO;
> > +		return (ret == -EIO || ret == -ENODEV) ? -ENODEV : -EIO;
> 
> Since ret will only ever be -EIO here, this can only ever return
> -ENODEV, which is a functional change in the code (probably unintended.)
> Nevertheless, it's likely introducing a bug if the intention is for
> some other return from mdiobus_read() to be handled differently.
> 
> >  	}
> >  
> > -	*phy_id = phy_reg << 16;
> > -
> > -	/* Grab the bits from PHYIR2, and put them in the lower half */
> > -	phy_reg = mdiobus_read(bus, addr, MII_PHYSID2);
> > -	if (phy_reg < 0)
> > -		return -EIO;
> 
> ... whereas this one always returns -EIO on any error.
> 
> So, I think you have the potential in this patch to introduce a subtle
> change of behaviour, which may lead to problems - have you closely
> analysed why the code was the way it was, and whether your change of
> behaviour is actually valid?

I could be remembering this wrongly, but i think this is to do with
orion_mdio_xsmi_read() returning -ENODEV, not 0xffffffffff, if there
is no device on the bus at the given address. -EIO is fatal to the
scan, everything stops with the assumption the bus is broken. -ENODEV
should not be fatal to the scan.

   Andrew