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

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 [off-list ref] writes:

quoted

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 <redacted>
Fixes: 05b35e7eb9a1 ("smsc95xx: add phylib support")
Signed-off-by: Andre Edich <redacted>
---
 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?

-- 
Regards,
Andre

--
Måns Rullgård