Re: [PATCH net-next] net: phy: lan87xx: fix access to wrong register of LAN87xx

["Måns Rullgård" <mans@mansr.com>]

<Andre.Edich@microchip.com> writes:

> On Mon, 2021-03-29 at 12:19 +0100, Måns Rullgård wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>> the content is safe
>> 
>> Andre Edich <andre.edich@microchip.com> writes:
>> 
>> > The function lan87xx_config_aneg_ext was introduced to configure
>> > LAN95xxA but as well writes to undocumented register of LAN87xx.
>> > This fix prevents that access.
>> > 
>> > The function lan87xx_config_aneg_ext gets more suitable for the new
>> > behavior name.
>> > 
>> > Reported-by: Måns Rullgård <mans@mansr.com>
>> > Fixes: 05b35e7eb9a1 ("smsc95xx: add phylib support")
>> > Signed-off-by: Andre Edich <andre.edich@microchip.com>
>> > ---
>> >  drivers/net/phy/smsc.c | 7 +++++--
>> >  1 file changed, 5 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
>> > index ddb78fb4d6dc..d8cac02a79b9 100644
>> > --- a/drivers/net/phy/smsc.c
>> > +++ b/drivers/net/phy/smsc.c
>> > @@ -185,10 +185,13 @@ static int lan87xx_config_aneg(struct
>> > phy_device *phydev)
>> >       return genphy_config_aneg(phydev);
>> >  }
>> > 
>> > -static int lan87xx_config_aneg_ext(struct phy_device *phydev)
>> > +static int lan95xx_config_aneg_ext(struct phy_device *phydev)
>> >  {
>> >       int rc;
>> > 
>> > +     if (phydev->phy_id != 0x0007c0f0) /* not (LAN9500A or LAN9505A)
>> > */
>> > +             return lan87xx_config_aneg(phydev);
>> > +
>> >       /* Extend Manual AutoMDIX timer */
>> >       rc = phy_read(phydev, PHY_EDPD_CONFIG);
>> >       if (rc < 0)
>> > @@ -441,7 +444,7 @@ static struct phy_driver smsc_phy_driver[] = {
>> >       .read_status    = lan87xx_read_status,
>> >       .config_init    = smsc_phy_config_init,
>> >       .soft_reset     = smsc_phy_reset,
>> > -     .config_aneg    = lan87xx_config_aneg_ext,
>> > +     .config_aneg    = lan95xx_config_aneg_ext,
>> > 
>> >       /* IRQ related */
>> >       .config_intr    = smsc_phy_config_intr,
>> > --
>> 
>> This seems to differentiate based on the "revision" field of the ID
>> register.  Can we be certain that a future update of chip won't break
>> this assumption?
>
> The way to fail would be to "fix" and release any of LAN95xxA in the
> way that the register map will is changed or feature is disabled but
> the Phy ID  remains the same.  This is very unlikely and obviously
> wrong.  I don't believe this may happen.
>
> If a new chip with the different Phy ID but the same feature will be
> released, then it must be explicitly added into the table.
>
> Is there a third case I don't see?

I was thinking that an updated LAN9500A might get the revision field set
to 1, making it indistinguishable from a LAN8710A.  That wouldn't break
anything as such, but that bit wouldn't get set either.  It seems
unlikely that this will ever happen, though, and it if it does, it can
be dealt with then.

Some rummaging in my boxes of old boards turned up both A and B
revisions of the LAN8720A, and they both have the same ID register value
(0xc0f1).  I had wondered if the A version might have had the revision
field set to 0, but apparently not.

-- 
Måns Rullgård