| Message ID | 85eb0791bd614ccfdeccdc6fe39be55e602c521c.1682163424.git.daniel@makrotopia.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Improvements for RealTek 2.5G Ethernet PHYs | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 10 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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; > }
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; }
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(-)