| Message ID | 792b8c0d1fc194e2b53cb09d45a234bc668e34c6.1728057091.git.daniel@makrotopia.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: realtek: make sure paged read is protected by mutex | expand |
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
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.
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
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; }
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(-)