lkml.org 
[lkml]   [2020]   [May]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC 03/11] net: phy: refactor c45 phy identification sequence
On Sat, May 23, 2020 at 09:51:31PM +0200, Andrew Lunn wrote:
> > > 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.

Maybe orion_mdio_xsmi_read() should be fixed then? Also, maybe
adding return code documentation for mdiobus_read() / mdiobus_write()
would help MDIO driver authors have some consistency in what
errors they are expected to return (does anyone know for certain?)

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

\
 
 \ /
  Last update: 2020-05-23 22:03    [W:0.129 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site