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

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

[Andre Edich]

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,
-- 
2.28.0

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

[Måns Rullgård]

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?

-- 
Måns Rullgård

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

[Andre.Edich]

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?

-- 
Regards,
Andre

> 
> --
> Måns Rullgård

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

[Måns Rullgård]

<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

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

[Måns Rullgård]

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(-)

I can confirm that this fixes the problem I was seeing.

> 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,
> -- 
>
> 2.28.0
>

-- 
Måns Rullgård

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Mon, 29 Mar 2021 11:45:36 +0200 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [net-next] net: phy: lan87xx: fix access to wrong register of LAN87xx
    https://git.kernel.org/netdev/net-next/c/fdb5cc6ab3b6

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html