On Tue, Oct 20, 2020 at 09:24:04PM +0000, Chris Packham wrote: > > On 20/10/20 11:18 pm, Russell King - ARM Linux admin wrote: > > On Tue, Oct 20, 2020 at 04:45:58PM +1300, Chris Packham wrote: > >> +void mv88e6123_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, > >> void *_p) > >> +{ > >> + u16 *p = _p; > >> + u16 reg; > >> + int i; > >> + > >> + if (mv88e6xxx_serdes_get_lane(chip, port) == 0) > >> + return; > >> + > >> + for (i = 0; i < 26; i++) { > >> + mv88e6xxx_phy_read(chip, port, i, ®); > > Shouldn't this deal with a failed read in some way, rather than just > > assigning the last or possibly uninitialised value to p[i] ? > > mv88e6390_serdes_get_regs() and mv88e6352_serdes_get_regs() also ignore > the error. The generic mv88e6xxx_get_regs() memsets p[] to 0xff so if > the serdes_get_regs functions just left it alone we'd return 0xffff > which is probably better than repeating the last value although it's > still ambiguous because 0xffff is a valid value for plenty of these > registers. > > Since it looks like I need to come up with an alternative to patch #1 > I'll concentrate on that but making the serdes_get_regs() a little more > error tolerant is a cleanup I can easily tack on onto this series.
Yep, it looks like they all suffer the same problem. Interestingly, mv88e6xxx_get_regs() does handle the error by avoiding writing the register entry (so it gets left as 0xffff.) Incidentally, that's also the value you'll get when reading from a PHY that doesn't respond, since the MDIO data line is pulled high when undriven. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!