Improve support for RealTek 2.5G Ethernet PHYs (RTL822x series).
The PHYs can operate with Clause-22 and Clause-45 MDIO.
When using Clause-45 it is desireable to avoid rate-adapter mode and
rather have the MAC interface mode follow the PHY speed. The PHYs
support 2500Base-X for 2500M, and Cisco SGMII for 1000M/100M/10M.
Also prepare support for proprietary RealTek HiSGMII mode which will
be needed for situations when used with RealTek switch or router SoCs
such as RTL93xx.
Add support for Link Down Power Saving Mode (ALDPS) which is already
supported for older RTL821x series 1GbE PHYs.
Make sure that link-partner advertised modes are only used if the
advertisement can be considered valid. Otherwise we are seeing
false-positives warning about downscaling eventhough higher speeds
are not actually advertised by the link partner.
While at it, improve the driver by using existing macros and inline
functions which are not actually vendor specific.
Alexander Couzens (1):
net: phy: realtek: rtl8221: allow to configure SERDES mode
Chukun Pan (1):
net: phy: realtek: switch interface mode for RTL822x series
Daniel Golle (6):
net: phy: realtek: use genphy_soft_reset for 2.5G PHYs
net: phy: realtek: disable SGMII in-band AN for 2.5G PHYs
net: phy: realtek: use phy_read_paged instead of open coding
net: phy: realtek: use inline functions for 10GbE advertisement
net: phy: realtek: check validity of 10GbE link-partner advertisement
net: phy: realtek: setup ALDPS on RTL822x
drivers/net/phy/realtek.c | 152 ++++++++++++++++++++++++++++++++------
1 file changed, 130 insertions(+), 22 deletions(-)
base-commit: fbc1449d385d65be49a8d164dfd3772f2cb049ae
--
2.40.0
Some vendor bootloaders do weird things with those PHYs which result in
link modes being reported wrongly. Start from a clean sheet by resetting
the PHY.
Reported-by: Yevhen Kolomeiko <[email protected]>
Signed-off-by: Daniel Golle <[email protected]>
---
drivers/net/phy/realtek.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 34fd86b8ecf7d..9b477dd17fa56 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -1038,6 +1038,7 @@ static struct phy_driver realtek_drvs[] = {
.write_page = rtl821x_write_page,
.read_mmd = rtl822x_read_mmd,
.write_mmd = rtl822x_write_mmd,
+ .soft_reset = genphy_soft_reset,
}, {
PHY_ID_MATCH_EXACT(0x001cc840),
.name = "RTL8226B_RTL8221B 2.5Gbps PHY",
@@ -1050,6 +1051,7 @@ static struct phy_driver realtek_drvs[] = {
.write_page = rtl821x_write_page,
.read_mmd = rtl822x_read_mmd,
.write_mmd = rtl822x_write_mmd,
+ .soft_reset = genphy_soft_reset,
}, {
PHY_ID_MATCH_EXACT(0x001cc838),
.name = "RTL8226-CG 2.5Gbps PHY",
@@ -1060,6 +1062,7 @@ static struct phy_driver realtek_drvs[] = {
.resume = rtlgen_resume,
.read_page = rtl821x_read_page,
.write_page = rtl821x_write_page,
+ .soft_reset = genphy_soft_reset,
}, {
PHY_ID_MATCH_EXACT(0x001cc848),
.name = "RTL8226B-CG_RTL8221B-CG 2.5Gbps PHY",
@@ -1070,6 +1073,7 @@ static struct phy_driver realtek_drvs[] = {
.resume = rtlgen_resume,
.read_page = rtl821x_read_page,
.write_page = rtl821x_write_page,
+ .soft_reset = genphy_soft_reset,
}, {
PHY_ID_MATCH_EXACT(0x001cc849),
.name = "RTL8221B-VB-CG 2.5Gbps PHY",
@@ -1081,6 +1085,7 @@ static struct phy_driver realtek_drvs[] = {
.resume = rtlgen_resume,
.read_page = rtl821x_read_page,
.write_page = rtl821x_write_page,
+ .soft_reset = genphy_soft_reset,
}, {
PHY_ID_MATCH_EXACT(0x001cc84a),
.name = "RTL8221B-VM-CG 2.5Gbps PHY",
@@ -1092,6 +1097,7 @@ static struct phy_driver realtek_drvs[] = {
.resume = rtlgen_resume,
.read_page = rtl821x_read_page,
.write_page = rtl821x_write_page,
+ .soft_reset = genphy_soft_reset,
}, {
PHY_ID_MATCH_EXACT(0x001cc961),
.name = "RTL8366RB Gigabit Ethernet",
--
2.40.0
From: Alexander Couzens <[email protected]>
The rtl8221 supports multiple SERDES modes:
- SGMII
- 2500base-x
- HiSGMII
Further it supports rate adaption on SERDES links to allow
slow ethernet speeds (10/100/1000mbit) to work on 2500base-x/HiSGMII
links without reducing the SERDES speed.
When operating without rate adapters the SERDES link will follow the
ethernet speed.
Signed-off-by: Alexander Couzens <[email protected]>
---
drivers/net/phy/realtek.c | 53 +++++++++++++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 3d99fd6664d7a..6389abaab6d5a 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -53,6 +53,15 @@
RTL8201F_ISR_LINK)
#define RTL8201F_IER 0x13
+#define RTL8221B_MMD_SERDES_CTRL MDIO_MMD_VEND1
+#define RTL8221B_MMD_PHY_CTRL MDIO_MMD_VEND2
+#define RTL8221B_SERDES_OPTION 0x697a
+#define RTL8221B_SERDES_OPTION_MODE_MASK GENMASK(5, 0)
+#define RTL8221B_SERDES_OPTION_MODE_2500BASEX_SGMII 0
+#define RTL8221B_SERDES_OPTION_MODE_HISGMII_SGMII 1
+#define RTL8221B_SERDES_OPTION_MODE_2500BASEX 2
+#define RTL8221B_SERDES_OPTION_MODE_HISGMII 3
+
#define RTL8366RB_POWER_SAVE 0x15
#define RTL8366RB_POWER_SAVE_ON BIT(12)
@@ -849,6 +858,48 @@ static irqreturn_t rtl9000a_handle_interrupt(struct phy_device *phydev)
return IRQ_HANDLED;
}
+static int rtl8221b_config_init(struct phy_device *phydev)
+{
+ u16 option_mode;
+
+ switch (phydev->interface) {
+ case PHY_INTERFACE_MODE_2500BASEX:
+ if (!phydev->is_c45) {
+ option_mode = RTL8221B_SERDES_OPTION_MODE_2500BASEX;
+ break;
+ }
+ fallthrough;
+ case PHY_INTERFACE_MODE_SGMII:
+ option_mode = RTL8221B_SERDES_OPTION_MODE_2500BASEX_SGMII;
+ break;
+ default:
+ return 0;
+ }
+
+ phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL,
+ 0x75f3, 0);
+
+ phy_modify_mmd_changed(phydev, RTL8221B_MMD_SERDES_CTRL,
+ RTL8221B_SERDES_OPTION,
+ RTL8221B_SERDES_OPTION_MODE_MASK, option_mode);
+ switch (option_mode) {
+ case RTL8221B_SERDES_OPTION_MODE_2500BASEX_SGMII:
+ case RTL8221B_SERDES_OPTION_MODE_2500BASEX:
+ phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6a04, 0x0503);
+ phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6f10, 0xd455);
+ phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6f11, 0x8020);
+ break;
+ case RTL8221B_SERDES_OPTION_MODE_HISGMII_SGMII:
+ case RTL8221B_SERDES_OPTION_MODE_HISGMII:
+ phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6a04, 0x0503);
+ phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6f10, 0xd433);
+ phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6f11, 0x8020);
+ break;
+ }
+
+ return 0;
+}
+
static struct phy_driver realtek_drvs[] = {
{
PHY_ID_MATCH_EXACT(0x00008201),
@@ -1001,6 +1052,7 @@ static struct phy_driver realtek_drvs[] = {
PHY_ID_MATCH_EXACT(0x001cc849),
.name = "RTL8221B-VB-CG 2.5Gbps PHY",
.get_features = rtl822x_get_features,
+ .config_init = rtl8221b_config_init,
.config_aneg = rtl822x_config_aneg,
.read_status = rtl822x_read_status,
.suspend = genphy_suspend,
@@ -1012,6 +1064,7 @@ static struct phy_driver realtek_drvs[] = {
.name = "RTL8221B-VM-CG 2.5Gbps PHY",
.get_features = rtl822x_get_features,
.config_aneg = rtl822x_config_aneg,
+ .config_init = rtl8221b_config_init,
.read_status = rtl822x_read_status,
.suspend = genphy_suspend,
.resume = rtlgen_resume,
--
2.40.0
MAC drivers don't use SGMII in-band autonegotiation unless told to do so
in device tree using 'managed = "in-band-status"'. When using MDIO to
access a PHY, in-band-status is unneeded as we have link-status via
MDIO. Switch off SGMII in-band autonegotiation using magic values.
Reported-by: Chen Minqiang <[email protected]>
Reported-by: Chukun Pan <[email protected]>
Reported-by: Yevhen Kolomeiko <[email protected]>
Tested-by: Yevhen Kolomeiko <[email protected]>
Signed-off-by: Daniel Golle <[email protected]>
---
drivers/net/phy/realtek.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 9b477dd17fa56..f97b5e49fae58 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -883,6 +883,7 @@ static irqreturn_t rtl9000a_handle_interrupt(struct phy_device *phydev)
static int rtl8221b_config_init(struct phy_device *phydev)
{
u16 option_mode;
+ int val;
switch (phydev->interface) {
case PHY_INTERFACE_MODE_2500BASEX:
@@ -919,6 +920,13 @@ static int rtl8221b_config_init(struct phy_device *phydev)
break;
}
+ /* Disable SGMII AN */
+ phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x7588, 0x2);
+ phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x7589, 0x71d0);
+ phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x7587, 0x3);
+ phy_read_mmd_poll_timeout(phydev, RTL8221B_MMD_SERDES_CTRL, 0x7587,
+ val, !(val & BIT(0)), 500, 100000, false);
+
return 0;
}
--
2.40.0
From: Chukun Pan <[email protected]>
The RTL822x phy can work in Cisco SGMII and 2500BASE-X modes respectively.
Add interface automatic switching MAC-side interface mode for RTL822x
phy to match various wire speeds when using Clause-45 MDIO.
Signed-off-by: Chukun Pan <[email protected]>
Signed-off-by: Daniel Golle <[email protected]>
---
drivers/net/phy/realtek.c | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 6389abaab6d5a..34fd86b8ecf7d 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -684,6 +684,25 @@ static int rtl822x_config_aneg(struct phy_device *phydev)
return __genphy_config_aneg(phydev, ret);
}
+static void rtl822x_update_interface(struct phy_device *phydev)
+{
+ /* Automatically switch SERDES interface between
+ * SGMII and 2500-BaseX according to speed.
+ */
+ switch (phydev->speed) {
+ case SPEED_2500:
+ phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
+ break;
+ case SPEED_1000:
+ case SPEED_100:
+ case SPEED_10:
+ phydev->interface = PHY_INTERFACE_MODE_SGMII;
+ break;
+ default:
+ break;
+ }
+}
+
static int rtl822x_read_status(struct phy_device *phydev)
{
int ret;
@@ -702,11 +721,14 @@ static int rtl822x_read_status(struct phy_device *phydev)
phydev->lp_advertising, lpadv & RTL_LPADV_2500FULL);
}
- ret = genphy_read_status(phydev);
+ ret = rtlgen_read_status(phydev);
if (ret < 0)
return ret;
- return rtlgen_get_speed(phydev);
+ if (phydev->is_c45 && phydev->link)
+ rtl822x_update_interface(phydev);
+
+ return 0;
}
static bool rtlgen_supports_2_5gbps(struct phy_device *phydev)
--
2.40.0
Instead of open coding a paged read, use the phy_read_paged function
in rtlgen_supports_2_5gbps.
Signed-off-by: Daniel Golle <[email protected]>
---
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;
}
--
2.40.0
Only use link-partner advertisement bits for 10GbE modes if they are
actually valid. Check LOCALOK and REMOTEOK bits and clear 10GbE modes
unless both of them are set.
Signed-off-by: Daniel Golle <[email protected]>
---
drivers/net/phy/realtek.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 078f45447ddad..de73049037891 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -706,6 +706,10 @@ static int rtl822x_read_status(struct phy_device *phydev)
if (lpadv < 0)
return lpadv;
+ if (!(lpadv & MDIO_AN_10GBT_STAT_REMOK) ||
+ !(lpadv & MDIO_AN_10GBT_STAT_LOCOK))
+ lpadv = 0;
+
mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising, lpadv);
}
--
2.40.0
Use existing generic inline functions to encode local advertisement
of 10GbE link modes as well as to decode link-partner advertisement.
Signed-off-by: Daniel Golle <[email protected]>
---
drivers/net/phy/realtek.c | 22 +++++-----------------
1 file changed, 5 insertions(+), 17 deletions(-)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 62fb965b6d338..078f45447ddad 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -68,10 +68,6 @@
#define RTL_SUPPORTS_5000FULL BIT(14)
#define RTL_SUPPORTS_2500FULL BIT(13)
#define RTL_SUPPORTS_10000FULL BIT(0)
-#define RTL_ADV_2500FULL BIT(7)
-#define RTL_LPADV_10000FULL BIT(11)
-#define RTL_LPADV_5000FULL BIT(6)
-#define RTL_LPADV_2500FULL BIT(5)
#define RTL9000A_GINMR 0x14
#define RTL9000A_GINMR_LINK_STATUS BIT(4)
@@ -669,14 +665,11 @@ static int rtl822x_config_aneg(struct phy_device *phydev)
int ret = 0;
if (phydev->autoneg == AUTONEG_ENABLE) {
- u16 adv2500 = 0;
-
- if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
- phydev->advertising))
- adv2500 = RTL_ADV_2500FULL;
-
ret = phy_modify_paged_changed(phydev, 0xa5d, 0x12,
- RTL_ADV_2500FULL, adv2500);
+ MDIO_AN_10GBT_CTRL_ADV10G |
+ MDIO_AN_10GBT_CTRL_ADV5G |
+ MDIO_AN_10GBT_CTRL_ADV2_5G,
+ linkmode_adv_to_mii_10gbt_adv_t(phydev->advertising));
if (ret < 0)
return ret;
}
@@ -713,12 +706,7 @@ static int rtl822x_read_status(struct phy_device *phydev)
if (lpadv < 0)
return lpadv;
- linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
- phydev->lp_advertising, lpadv & RTL_LPADV_10000FULL);
- linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
- phydev->lp_advertising, lpadv & RTL_LPADV_5000FULL);
- linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
- phydev->lp_advertising, lpadv & RTL_LPADV_2500FULL);
+ mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising, lpadv);
}
ret = rtlgen_read_status(phydev);
--
2.40.0
Setup Link Down Power Saving Mode according the DTS property
just like for RTL821x 1GE PHYs.
Signed-off-by: Daniel Golle <[email protected]>
---
drivers/net/phy/realtek.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index de73049037891..a2324918c42db 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -62,6 +62,10 @@
#define RTL8221B_SERDES_OPTION_MODE_2500BASEX 2
#define RTL8221B_SERDES_OPTION_MODE_HISGMII 3
+#define RTL8221B_PHYCR1 0xa430
+#define RTL8221B_PHYCR1_ALDPS_EN BIT(2)
+#define RTL8221B_PHYCR1_ALDPS_XTAL_OFF_EN BIT(12)
+
#define RTL8366RB_POWER_SAVE 0x15
#define RTL8366RB_POWER_SAVE_ON BIT(12)
@@ -744,6 +748,25 @@ static int rtl8226_match_phy_device(struct phy_device *phydev)
rtlgen_supports_2_5gbps(phydev);
}
+static int rtl822x_probe(struct phy_device *phydev)
+{
+ struct device *dev = &phydev->mdio.dev;
+ int val;
+
+ val = phy_read_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, RTL8221B_PHYCR1);
+ if (val < 0)
+ return val;
+
+ if (of_property_read_bool(dev->of_node, "realtek,aldps-enable"))
+ val |= RTL8221B_PHYCR1_ALDPS_EN | RTL8221B_PHYCR1_ALDPS_XTAL_OFF_EN;
+ else
+ val &= ~(RTL8221B_PHYCR1_ALDPS_EN | RTL8221B_PHYCR1_ALDPS_XTAL_OFF_EN);
+
+ phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, RTL8221B_PHYCR1, val);
+
+ return 0;
+}
+
static int rtlgen_resume(struct phy_device *phydev)
{
int ret = genphy_resume(phydev);
@@ -1029,6 +1052,7 @@ static struct phy_driver realtek_drvs[] = {
.match_phy_device = rtl8226_match_phy_device,
.get_features = rtl822x_get_features,
.config_aneg = rtl822x_config_aneg,
+ .probe = rtl822x_probe,
.read_status = rtl822x_read_status,
.suspend = genphy_suspend,
.resume = rtlgen_resume,
@@ -1042,6 +1066,7 @@ static struct phy_driver realtek_drvs[] = {
.name = "RTL8226B_RTL8221B 2.5Gbps PHY",
.get_features = rtl822x_get_features,
.config_aneg = rtl822x_config_aneg,
+ .probe = rtl822x_probe,
.read_status = rtl822x_read_status,
.suspend = genphy_suspend,
.resume = rtlgen_resume,
@@ -1055,6 +1080,7 @@ static struct phy_driver realtek_drvs[] = {
.name = "RTL8226-CG 2.5Gbps PHY",
.get_features = rtl822x_get_features,
.config_aneg = rtl822x_config_aneg,
+ .probe = rtl822x_probe,
.read_status = rtl822x_read_status,
.suspend = genphy_suspend,
.resume = rtlgen_resume,
@@ -1066,6 +1092,7 @@ static struct phy_driver realtek_drvs[] = {
.name = "RTL8226B-CG_RTL8221B-CG 2.5Gbps PHY",
.get_features = rtl822x_get_features,
.config_aneg = rtl822x_config_aneg,
+ .probe = rtl822x_probe,
.read_status = rtl822x_read_status,
.suspend = genphy_suspend,
.resume = rtlgen_resume,
@@ -1078,6 +1105,7 @@ static struct phy_driver realtek_drvs[] = {
.get_features = rtl822x_get_features,
.config_init = rtl8221b_config_init,
.config_aneg = rtl822x_config_aneg,
+ .probe = rtl822x_probe,
.read_status = rtl822x_read_status,
.suspend = genphy_suspend,
.resume = rtlgen_resume,
@@ -1090,6 +1118,7 @@ static struct phy_driver realtek_drvs[] = {
.get_features = rtl822x_get_features,
.config_aneg = rtl822x_config_aneg,
.config_init = rtl8221b_config_init,
+ .probe = rtl822x_probe,
.read_status = rtl822x_read_status,
.suspend = genphy_suspend,
.resume = rtlgen_resume,
--
2.40.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 <[email protected]>
> ---
> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
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 <[email protected]>
---
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;
}
--
2.40.0
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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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;
> }