[RFC,net-next,5/8] net: phy: realtek: use phy_read_paged instead of open coding

  Instead of open coding a paged read, use the phy_read_paged function
in rtlgen_supports_2_5gbps.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/phy/realtek.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
  

On 22.04.2023 13:48, Daniel Golle wrote:
> Instead of open coding a paged read, use the phy_read_paged function
> in rtlgen_supports_2_5gbps.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  drivers/net/phy/realtek.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index f97b5e49fae58..62fb965b6d338 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -735,9 +735,7 @@ static bool rtlgen_supports_2_5gbps(struct phy_device *phydev)
>  {
>  	int val;
>  
> -	phy_write(phydev, RTL821x_PAGE_SELECT, 0xa61);
> -	val = phy_read(phydev, 0x13);
> -	phy_write(phydev, RTL821x_PAGE_SELECT, 0);
> +	val = phy_read_paged(phydev, 0xa61, 0x13);
>  
>  	return val >= 0 && val & RTL_SUPPORTS_2500FULL;
>  }

I remember I had a reason to open-code it, it took me some minutes
to recall it.
phy_read_paged() calls __phy_read_page() that relies on phydev->drv
being set. phydev->drv is set in phy_probe(). And probing is done
after matching. __phy_read_paged() should have given you a warning.
Did you test this patch? If yes and you didn't get the warning,
then apparently I miss something.
  

On Sat, Apr 22, 2023 at 05:11:57PM +0200, Heiner Kallweit wrote:
> On 22.04.2023 13:48, Daniel Golle wrote:
> > Instead of open coding a paged read, use the phy_read_paged function
> > in rtlgen_supports_2_5gbps.
> > 
> > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > ---
> >  drivers/net/phy/realtek.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > index f97b5e49fae58..62fb965b6d338 100644
> > --- a/drivers/net/phy/realtek.c
> > +++ b/drivers/net/phy/realtek.c
> > @@ -735,9 +735,7 @@ static bool rtlgen_supports_2_5gbps(struct phy_device *phydev)
> >  {
> >  	int val;
> >  
> > -	phy_write(phydev, RTL821x_PAGE_SELECT, 0xa61);
> > -	val = phy_read(phydev, 0x13);
> > -	phy_write(phydev, RTL821x_PAGE_SELECT, 0);
> > +	val = phy_read_paged(phydev, 0xa61, 0x13);
> >  
> >  	return val >= 0 && val & RTL_SUPPORTS_2500FULL;
> >  }
> 
> I remember I had a reason to open-code it, it took me some minutes
> to recall it.
> phy_read_paged() calls __phy_read_page() that relies on phydev->drv
> being set. phydev->drv is set in phy_probe(). And probing is done
> after matching. __phy_read_paged() should have given you a warning.
> Did you test this patch? If yes and you didn't get the warning,
> then apparently I miss something.
>

Yes, you are right, this change was a bit too naive and causes a
NULL pointer dereference e.g. for the r8169 driver which also uses
the RealTek Ethernet PHY driver.
My main concern and original motivation was the lack of mutex protection
for the paged read operation. I suggest to rather make this change
instead:

From 4dd2cc9b91ecb25f278a2c55e07e6455e9000e6b Mon Sep 17 00:00:00 2001
From: Daniel Golle <daniel@makrotopia.org>
Date: Sun, 23 Apr 2023 18:47:45 +0100
Subject: [PATCH] net: phy: realtek: make sure paged read is protected by mutex

As we cannot rely on phy_read_paged function before the PHY is
identified, the paged read in rtlgen_supports_2_5gbps needs to be open
coded as it is being called by the match_phy_device function, ie. before
.read_page and .write_page have been populated.

Make sure it is also protected by the MDIO bus mutex and use
rtl821x_write_page instead of 3 individually locked MDIO bus operations.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/phy/realtek.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index f97b5e49fae5..c27ec4e99fc2 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -735,9 +735,11 @@ static bool rtlgen_supports_2_5gbps(struct phy_device *phydev)
 {
 	int val;
 
-	phy_write(phydev, RTL821x_PAGE_SELECT, 0xa61);
-	val = phy_read(phydev, 0x13);
-	phy_write(phydev, RTL821x_PAGE_SELECT, 0);
+	mutex_lock(&phydev->mdio.bus->mdio_lock);
+	rtl821x_write_page(phydev, 0xa61);
+	val = __phy_read(phydev, 0x13);
+	rtl821x_write_page(phydev, 0);
+	mutex_unlock(&phydev->mdio.bus->mdio_lock);
 
 	return val >= 0 && val & RTL_SUPPORTS_2500FULL;
 }
  

On 23.04.2023 20:01, Daniel Golle wrote:
> On Sat, Apr 22, 2023 at 05:11:57PM +0200, Heiner Kallweit wrote:
>> On 22.04.2023 13:48, Daniel Golle wrote:
>>> Instead of open coding a paged read, use the phy_read_paged function
>>> in rtlgen_supports_2_5gbps.
>>>
>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>>> ---
>>>  drivers/net/phy/realtek.c | 4 +---
>>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
>>> index f97b5e49fae58..62fb965b6d338 100644
>>> --- a/drivers/net/phy/realtek.c
>>> +++ b/drivers/net/phy/realtek.c
>>> @@ -735,9 +735,7 @@ static bool rtlgen_supports_2_5gbps(struct phy_device *phydev)
>>>  {
>>>  	int val;
>>>  
>>> -	phy_write(phydev, RTL821x_PAGE_SELECT, 0xa61);
>>> -	val = phy_read(phydev, 0x13);
>>> -	phy_write(phydev, RTL821x_PAGE_SELECT, 0);
>>> +	val = phy_read_paged(phydev, 0xa61, 0x13);
>>>  
>>>  	return val >= 0 && val & RTL_SUPPORTS_2500FULL;
>>>  }
>>
>> I remember I had a reason to open-code it, it took me some minutes
>> to recall it.
>> phy_read_paged() calls __phy_read_page() that relies on phydev->drv
>> being set. phydev->drv is set in phy_probe(). And probing is done
>> after matching. __phy_read_paged() should have given you a warning.
>> Did you test this patch? If yes and you didn't get the warning,
>> then apparently I miss something.
>>
> 
> Yes, you are right, this change was a bit too naive and causes a
> NULL pointer dereference e.g. for the r8169 driver which also uses
> the RealTek Ethernet PHY driver.
> My main concern and original motivation was the lack of mutex protection
> for the paged read operation. I suggest to rather make this change
> instead:
> 
>>From 4dd2cc9b91ecb25f278a2c55e07e6455e9000e6b Mon Sep 17 00:00:00 2001
> From: Daniel Golle <daniel@makrotopia.org>
> Date: Sun, 23 Apr 2023 18:47:45 +0100
> Subject: [PATCH] net: phy: realtek: make sure paged read is protected by mutex
> 
> As we cannot rely on phy_read_paged function before the PHY is
> identified, the paged read in rtlgen_supports_2_5gbps needs to be open
> coded as it is being called by the match_phy_device function, ie. before
> .read_page and .write_page have been populated.
> 
> Make sure it is also protected by the MDIO bus mutex and use
> rtl821x_write_page instead of 3 individually locked MDIO bus operations.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  drivers/net/phy/realtek.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index f97b5e49fae5..c27ec4e99fc2 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -735,9 +735,11 @@ static bool rtlgen_supports_2_5gbps(struct phy_device *phydev)
>  {
>  	int val;
>  
> -	phy_write(phydev, RTL821x_PAGE_SELECT, 0xa61);
> -	val = phy_read(phydev, 0x13);
> -	phy_write(phydev, RTL821x_PAGE_SELECT, 0);
> +	mutex_lock(&phydev->mdio.bus->mdio_lock);

We have helpers phy_(un)lock_mdio_bus() for this.
Apart from that: LGTM

> +	rtl821x_write_page(phydev, 0xa61);
> +	val = __phy_read(phydev, 0x13);
> +	rtl821x_write_page(phydev, 0);
> +	mutex_unlock(&phydev->mdio.bus->mdio_lock);
>  
>  	return val >= 0 && val & RTL_SUPPORTS_2500FULL;
>  }