[PATCH net-next] net: phy: realtek: make sure paged read is protected by mutex

[PATCH net-next] net: phy: realtek: make sure paged read is protected by mutex

[Daniel Golle]

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 3941cfa1c5f2..c4d0d93523ad 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -1030,9 +1030,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 & MDIO_PMA_SPEED_2_5G;
 }
-- 
2.46.2

Re: [PATCH net-next] net: phy: realtek: make sure paged read is protected by mutex

[Andrew Lunn]

On Fri, Oct 04, 2024 at 04:52:04PM +0100, Daniel Golle wrote:
> 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.

match_phy_device() as far as i know, is only used during bus probe,
when trying to match a driver to a device. What are you trying to lock
against during probe?

	Andrew

Re: [PATCH net-next] net: phy: realtek: make sure paged read is protected by mutex

[Daniel Golle]

On Fri, Oct 04, 2024 at 11:25:29PM +0200, Andrew Lunn wrote:
> On Fri, Oct 04, 2024 at 04:52:04PM +0100, Daniel Golle wrote:
> > 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.
> 
> match_phy_device() as far as i know, is only used during bus probe,
> when trying to match a driver to a device. What are you trying to lock
> against during probe?

The idea is to reduce the amount of unnecessary lock/unlock cycles (1 vs
3). Of course, we could just omit locking entirely here, but that seemed
a bit wild to me, and even if it would work in that specific case, it
just serve as a bad example.

Re: [PATCH net-next] net: phy: realtek: make sure paged read is protected by mutex

[Andrew Lunn]

On Fri, Oct 04, 2024 at 11:02:13PM +0100, Daniel Golle wrote:
> On Fri, Oct 04, 2024 at 11:25:29PM +0200, Andrew Lunn wrote:
> > On Fri, Oct 04, 2024 at 04:52:04PM +0100, Daniel Golle wrote:
> > > 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.
> > 
> > match_phy_device() as far as i know, is only used during bus probe,
> > when trying to match a driver to a device. What are you trying to lock
> > against during probe?
> 
> The idea is to reduce the amount of unnecessary lock/unlock cycles (1 vs
> 3). Of course, we could just omit locking entirely here, but that seemed
> a bit wild to me, and even if it would work in that specific case, it
> just serve as a bad example.

I would just comment the requirement that it can only be used during
probe, and remove all the locks.

	Andrew