[PATCH net-next 0/3] net: phy: realtek: add hwmon support

[PATCH net-next 0/3] net: phy: realtek: add hwmon support

[Heiner Kallweit]

This adds hwmon support for the temperature sensor on RTL822x.
It's available on the standalone versions of the PHY's, and on the
internal PHY's of RTL8125B(P)/RTL8125D/RTL8126.

Heiner Kallweit (3):
  net: phy: realtek: add support for reading MDIO_MMD_VEND2 regs on
    RTL8125/RTL8126
  net: phy: realtek: rename realtek.c to realtek_main.c
  net: phy: realtek: add hwmon support for temp sensor on RTL822x

 drivers/net/phy/Kconfig                       |  6 ++
 drivers/net/phy/Makefile                      |  2 +
 drivers/net/phy/realtek.h                     | 10 +++
 drivers/net/phy/realtek_hwmon.c               | 83 +++++++++++++++++++
 drivers/net/phy/{realtek.c => realtek_main.c} | 24 +++++-
 5 files changed, 123 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/phy/realtek.h
 create mode 100644 drivers/net/phy/realtek_hwmon.c
 rename drivers/net/phy/{realtek.c => realtek_main.c} (98%)

-- 
2.47.1

[PATCH net-next 1/3] net: phy: realtek: add support for reading MDIO_MMD_VEND2 regs on RTL8125/RTL8126

[Heiner Kallweit]

RTL8125/RTL8126 don't support MMD access to the internal PHY, but
provide a mechanism to access at least all MDIO_MMD_VEND2 registers.
By exposing this mechanism standard MMD access functions can be used
to access the MDIO_MMD_VEND2 registers.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/realtek.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index f65d7f1f3..af9874143 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -736,7 +736,11 @@ static int rtlgen_read_mmd(struct phy_device *phydev, int devnum, u16 regnum)
 {
 	int ret;
 
-	if (devnum == MDIO_MMD_PCS && regnum == MDIO_PCS_EEE_ABLE) {
+	if (devnum == MDIO_MMD_VEND2) {
+		rtl821x_write_page(phydev, regnum >> 4);
+		ret = __phy_read(phydev, 0x10 + ((regnum & 0xf) >> 1));
+		rtl821x_write_page(phydev, 0);
+	} else if (devnum == MDIO_MMD_PCS && regnum == MDIO_PCS_EEE_ABLE) {
 		rtl821x_write_page(phydev, 0xa5c);
 		ret = __phy_read(phydev, 0x12);
 		rtl821x_write_page(phydev, 0);
@@ -760,7 +764,11 @@ static int rtlgen_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,
 {
 	int ret;
 
-	if (devnum == MDIO_MMD_AN && regnum == MDIO_AN_EEE_ADV) {
+	if (devnum == MDIO_MMD_VEND2) {
+		rtl821x_write_page(phydev, regnum >> 4);
+		ret = __phy_write(phydev, 0x10 + ((regnum & 0xf) >> 1), val);
+		rtl821x_write_page(phydev, 0);
+	} else if (devnum == MDIO_MMD_AN && regnum == MDIO_AN_EEE_ADV) {
 		rtl821x_write_page(phydev, 0xa5d);
 		ret = __phy_write(phydev, 0x10, val);
 		rtl821x_write_page(phydev, 0);
-- 
2.47.1

[PATCH net-next 2/3] net: phy: realtek: rename realtek.c to realtek_main.c

[Heiner Kallweit]

In preparation of adding a source file with hwmon support, rename
realtek.c to realtek_main.c.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/Makefile                      | 1 +
 drivers/net/phy/{realtek.c => realtek_main.c} | 0
 2 files changed, 1 insertion(+)
 rename drivers/net/phy/{realtek.c => realtek_main.c} (100%)

diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 39b72b464..ec480e733 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -95,6 +95,7 @@ obj-$(CONFIG_NXP_CBTX_PHY)	+= nxp-cbtx.o
 obj-$(CONFIG_NXP_TJA11XX_PHY)	+= nxp-tja11xx.o
 obj-y				+= qcom/
 obj-$(CONFIG_QSEMI_PHY)		+= qsemi.o
+realtek-y += realtek_main.o
 obj-$(CONFIG_REALTEK_PHY)	+= realtek.o
 obj-$(CONFIG_RENESAS_PHY)	+= uPD60620.o
 obj-$(CONFIG_ROCKCHIP_PHY)	+= rockchip.o
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek_main.c
similarity index 100%
rename from drivers/net/phy/realtek.c
rename to drivers/net/phy/realtek_main.c
-- 
2.47.1

Re: [PATCH net-next 2/3] net: phy: realtek: rename realtek.c to realtek_main.c

[Russell King (Oracle)]

On Fri, Jan 10, 2025 at 12:47:39PM +0100, Heiner Kallweit wrote:
> In preparation of adding a source file with hwmon support, rename
> realtek.c to realtek_main.c.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/phy/Makefile                      | 1 +
>  drivers/net/phy/{realtek.c => realtek_main.c} | 0
>  2 files changed, 1 insertion(+)
>  rename drivers/net/phy/{realtek.c => realtek_main.c} (100%)

Is it worth considering a vendor subdirectory when PHYs end up with
multiple source files?

We already have aquantia, mediatek, mscc, and qcom. Should we be
considering it for this as well?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 2/3] net: phy: realtek: rename realtek.c to realtek_main.c

[Heiner Kallweit]

On 10.01.2025 13:40, Russell King (Oracle) wrote:
> On Fri, Jan 10, 2025 at 12:47:39PM +0100, Heiner Kallweit wrote:
>> In preparation of adding a source file with hwmon support, rename
>> realtek.c to realtek_main.c.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/phy/Makefile                      | 1 +
>>  drivers/net/phy/{realtek.c => realtek_main.c} | 0
>>  2 files changed, 1 insertion(+)
>>  rename drivers/net/phy/{realtek.c => realtek_main.c} (100%)
> 
> Is it worth considering a vendor subdirectory when PHYs end up with
> multiple source files?
> 
> We already have aquantia, mediatek, mscc, and qcom. Should we be
> considering it for this as well?
> 
Yes, I think this would make sense. I'll submit a v2.

[PATCH net-next 3/3] net: phy: realtek: add hwmon support for temp sensor on RTL822x

[Heiner Kallweit]

This adds hwmon support for the temperature sensor on RTL822x. It's
available on the standalone versions of the PHY's, and on the integrated
PHY's in RTL8125B/RTL8125D/RTL8126.

Notes:
- over-temp threshold is set by PHY power-on default or BIOS / boot loader
  and it's read-only
- over-temp alarm remains set, even if temperature drops below threshold

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/Kconfig         |  6 +++
 drivers/net/phy/Makefile        |  1 +
 drivers/net/phy/realtek.h       | 10 ++++
 drivers/net/phy/realtek_hwmon.c | 83 +++++++++++++++++++++++++++++++++
 drivers/net/phy/realtek_main.c  | 12 +++++
 5 files changed, 112 insertions(+)
 create mode 100644 drivers/net/phy/realtek.h
 create mode 100644 drivers/net/phy/realtek_hwmon.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index dc625f2b3..eae53ef88 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -355,6 +355,12 @@ config REALTEK_PHY
 	help
 	  Supports the Realtek 821x PHY.
 
+config REALTEK_PHY_HWMON
+	def_bool REALTEK_PHY && HWMON
+	depends on !(REALTEK_PHY=y && HWMON=m)
+	help
+	  Optional hwmon support for the temperature sensor
+
 config RENESAS_PHY
 	tristate "Renesas PHYs"
 	help
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index ec480e733..ca369a5c9 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -96,6 +96,7 @@ obj-$(CONFIG_NXP_TJA11XX_PHY)	+= nxp-tja11xx.o
 obj-y				+= qcom/
 obj-$(CONFIG_QSEMI_PHY)		+= qsemi.o
 realtek-y += realtek_main.o
+realtek-$(CONFIG_REALTEK_PHY_HWMON) += realtek_hwmon.o
 obj-$(CONFIG_REALTEK_PHY)	+= realtek.o
 obj-$(CONFIG_RENESAS_PHY)	+= uPD60620.o
 obj-$(CONFIG_ROCKCHIP_PHY)	+= rockchip.o
diff --git a/drivers/net/phy/realtek.h b/drivers/net/phy/realtek.h
new file mode 100644
index 000000000..a39b44fa1
--- /dev/null
+++ b/drivers/net/phy/realtek.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef REALTEK_H
+#define REALTEK_H
+
+#include <linux/phy.h>
+
+int rtl822x_hwmon_init(struct phy_device *phydev);
+
+#endif /* REALTEK_H */
diff --git a/drivers/net/phy/realtek_hwmon.c b/drivers/net/phy/realtek_hwmon.c
new file mode 100644
index 000000000..d3284c83f
--- /dev/null
+++ b/drivers/net/phy/realtek_hwmon.c
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0+
+ *
+ * HWMON support for Realtek PHY's
+ *
+ * Author: Heiner Kallweit <hkallweit1@gmail.com>
+ */
+
+#include <linux/hwmon.h>
+#include <linux/phy.h>
+
+#include "realtek.h"
+
+#define RTL822X_VND2_TSALRM				0xa662
+#define RTL822X_VND2_TSRR				0xbd84
+#define RTL822X_VND2_TSSR				0xb54c
+
+static int rtl822x_hwmon_get_temp(int raw)
+{
+	if (raw >= 512)
+		raw -= 1024;
+
+	return 1000 * raw / 2;
+}
+
+static int rtl822x_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+			      u32 attr, int channel, long *val)
+{
+	struct phy_device *phydev = dev_get_drvdata(dev);
+	int raw;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		raw = phy_read_mmd(phydev, MDIO_MMD_VEND2, RTL822X_VND2_TSRR) & 0x3ff;
+		*val = rtl822x_hwmon_get_temp(raw);
+		break;
+	case hwmon_temp_max:
+		/* Chip reduces speed to 1G if threshold is exceeded */
+		raw = phy_read_mmd(phydev, MDIO_MMD_VEND2, RTL822X_VND2_TSSR) >> 6;
+		*val = rtl822x_hwmon_get_temp(raw);
+		break;
+	case hwmon_temp_alarm:
+		raw = phy_read_mmd(phydev, MDIO_MMD_VEND2, RTL822X_VND2_TSALRM);
+		*val = !!(raw & 3);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static const struct hwmon_ops rtl822x_hwmon_ops = {
+	.visible = 0444,
+	.read = rtl822x_hwmon_read,
+};
+
+static const struct hwmon_channel_info * const rtl822x_hwmon_info[] = {
+	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_ALARM),
+	NULL
+};
+
+static const struct hwmon_chip_info rtl822x_hwmon_chip_info = {
+	.ops = &rtl822x_hwmon_ops,
+	.info = rtl822x_hwmon_info,
+};
+
+int rtl822x_hwmon_init(struct phy_device *phydev)
+{
+	struct device *hwdev, *dev = &phydev->mdio.dev;
+	const char *name;
+
+	/* Ensure over-temp alarm is reset. */
+	phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, RTL822X_VND2_TSALRM, 3);
+
+	name = devm_hwmon_sanitize_name(dev, dev_name(dev));
+	if (IS_ERR(name))
+		return PTR_ERR(name);
+
+	hwdev = devm_hwmon_device_register_with_info(dev, name, phydev,
+						     &rtl822x_hwmon_chip_info,
+						     NULL);
+	return PTR_ERR_OR_ZERO(hwdev);
+}
diff --git a/drivers/net/phy/realtek_main.c b/drivers/net/phy/realtek_main.c
index af9874143..38149958d 100644
--- a/drivers/net/phy/realtek_main.c
+++ b/drivers/net/phy/realtek_main.c
@@ -14,6 +14,8 @@
 #include <linux/delay.h>
 #include <linux/clk.h>
 
+#include "realtek.h"
+
 #define RTL821x_PHYSR				0x11
 #define RTL821x_PHYSR_DUPLEX			BIT(13)
 #define RTL821x_PHYSR_SPEED			GENMASK(15, 14)
@@ -820,6 +822,15 @@ static int rtl822x_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,
 	return ret;
 }
 
+static int rtl822x_probe(struct phy_device *phydev)
+{
+	if (IS_ENABLED(CONFIG_REALTEK_PHY_HWMON) &&
+	    phydev->phy_id != RTL_GENERIC_PHYID)
+		return rtl822x_hwmon_init(phydev);
+
+	return 0;
+}
+
 static int rtl822xb_config_init(struct phy_device *phydev)
 {
 	bool has_2500, has_sgmii;
@@ -1519,6 +1530,7 @@ static struct phy_driver realtek_drvs[] = {
 		.match_phy_device = rtl_internal_nbaset_match_phy_device,
 		.name           = "Realtek Internal NBASE-T PHY",
 		.flags		= PHY_IS_INTERNAL,
+		.probe		= rtl822x_probe,
 		.get_features   = rtl822x_get_features,
 		.config_aneg    = rtl822x_config_aneg,
 		.read_status    = rtl822x_read_status,
-- 
2.47.1

Re: [PATCH net-next 3/3] net: phy: realtek: add hwmon support for temp sensor on RTL822x

[Andrew Lunn]

> - over-temp alarm remains set, even if temperature drops below threshold

> +int rtl822x_hwmon_init(struct phy_device *phydev)
> +{
> +	struct device *hwdev, *dev = &phydev->mdio.dev;
> +	const char *name;
> +
> +	/* Ensure over-temp alarm is reset. */
> +	phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, RTL822X_VND2_TSALRM, 3);

So it is possible to clear the alarm.

I know you wanted to experiment with this some more....

If the alarm is still set, does that prevent the PHY renegotiating the
higher link speed? If you clear the alarm, does that allow it to
renegotiate the higher link speed? Or is a down/up still required?
Does an down/up clear the alarm if the temperature is below the
threshold?

Also, does HWMON support clearing alarms? Writing a 0 to the file? Or
are they supported to self clear on read?

I'm wondering if we are heading towards ABI issues? You have defined:

- over-temp alarm remains set, even if temperature drops below threshold

so that kind of eliminates the possibility of implementing self
clearing any time in the future. Explicit clearing via a write is
probably O.K, because the user needs to take an explicit action.  Are
there other ABI issues i have not thought about.

	Andrew

Re: [PATCH net-next 3/3] net: phy: realtek: add hwmon support for temp sensor on RTL822x

[Heiner Kallweit]

On 10.01.2025 22:10, Andrew Lunn wrote:
>> - over-temp alarm remains set, even if temperature drops below threshold
> 
>> +int rtl822x_hwmon_init(struct phy_device *phydev)
>> +{
>> +	struct device *hwdev, *dev = &phydev->mdio.dev;
>> +	const char *name;
>> +
>> +	/* Ensure over-temp alarm is reset. */
>> +	phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, RTL822X_VND2_TSALRM, 3);
> 
> So it is possible to clear the alarm.
> 
> I know you wanted to experiment with this some more....
> 
> If the alarm is still set, does that prevent the PHY renegotiating the
> higher link speed? If you clear the alarm, does that allow it to
> renegotiate the higher link speed? Or is a down/up still required?
> Does an down/up clear the alarm if the temperature is below the
> threshold?
> 
I tested wrt one of your previous questions, when exceeding the
temperature threshold the chip actually removes 2.5Gbps from the
advertisement register.

If the alarm is set, the chip won't switch back automatically to
2.5Gbps even if the temperature drops below the alarm threshold.

When clearing the alarm the chip adds 2.5Gbps back to the advertisement
register. Worth to be mentioned:
The temperature is checked only if the link speed is 2.5Gbps.
Therefore the chip thinks it's safe to add back the 2.5Gbps mode
when the alarm is cleared.

What I didn't test is whether it's possible to manually add 2.5Gbps
to the advertisement register whilst the alarm is set.
But I assume that's the case.

> Also, does HWMON support clearing alarms? Writing a 0 to the file? Or
> are they supported to self clear on read?
> 
Documentation/hwmon/sysfs-interface.rst states that the alarm
is a read-only attribute:

+-------------------------------+-----------------------+
| **`in[0-*]_alarm`,		| Channel alarm		|
| `curr[1-*]_alarm`,		|			|
| `power[1-*]_alarm`,		|   - 0: no alarm	|
| `fan[1-*]_alarm`,		|   - 1: alarm		|
| `temp[1-*]_alarm`**		|			|
|				|   RO			|
+-------------------------------+-----------------------+

Self-clearing is neither mentioned in the documentation nor
implemented in hwmon core.

@Guenter:
If alarm would just mean "current value > alarm threshold", then we
wouldn't need an extra alarm attribute, as this is something which
can be checked in user space.
Has it ever been considered that a user may have to explicitly ack
an alarm to clear it? Would you consider it an ABI violation if
alarm is configured as R/W for being able to clear the alarm?

> I'm wondering if we are heading towards ABI issues? You have defined:
> 
> - over-temp alarm remains set, even if temperature drops below threshold
> 
> so that kind of eliminates the possibility of implementing self
> clearing any time in the future. Explicit clearing via a write is
> probably O.K, because the user needs to take an explicit action.  Are
> there other ABI issues i have not thought about.
> 
> 	Andrew

Heiner

Re: [PATCH net-next 3/3] net: phy: realtek: add hwmon support for temp sensor on RTL822x

[Guenter Roeck]

On 1/10/25 13:41, Heiner Kallweit wrote:
> On 10.01.2025 22:10, Andrew Lunn wrote:
>>> - over-temp alarm remains set, even if temperature drops below threshold
>>
>>> +int rtl822x_hwmon_init(struct phy_device *phydev)
>>> +{
>>> +	struct device *hwdev, *dev = &phydev->mdio.dev;
>>> +	const char *name;
>>> +
>>> +	/* Ensure over-temp alarm is reset. */
>>> +	phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, RTL822X_VND2_TSALRM, 3);
>>
>> So it is possible to clear the alarm.
>>
>> I know you wanted to experiment with this some more....
>>
>> If the alarm is still set, does that prevent the PHY renegotiating the
>> higher link speed? If you clear the alarm, does that allow it to
>> renegotiate the higher link speed? Or is a down/up still required?
>> Does an down/up clear the alarm if the temperature is below the
>> threshold?
>>
> I tested wrt one of your previous questions, when exceeding the
> temperature threshold the chip actually removes 2.5Gbps from the
> advertisement register.
> 
> If the alarm is set, the chip won't switch back automatically to
> 2.5Gbps even if the temperature drops below the alarm threshold.
> 
> When clearing the alarm the chip adds 2.5Gbps back to the advertisement
> register. Worth to be mentioned:
> The temperature is checked only if the link speed is 2.5Gbps.
> Therefore the chip thinks it's safe to add back the 2.5Gbps mode
> when the alarm is cleared.
> 
> What I didn't test is whether it's possible to manually add 2.5Gbps
> to the advertisement register whilst the alarm is set.
> But I assume that's the case.
> 
>> Also, does HWMON support clearing alarms? Writing a 0 to the file? Or
>> are they supported to self clear on read?
>>
> Documentation/hwmon/sysfs-interface.rst states that the alarm
> is a read-only attribute:
> 
> +-------------------------------+-----------------------+
> | **`in[0-*]_alarm`,		| Channel alarm		|
> | `curr[1-*]_alarm`,		|			|
> | `power[1-*]_alarm`,		|   - 0: no alarm	|
> | `fan[1-*]_alarm`,		|   - 1: alarm		|
> | `temp[1-*]_alarm`**		|			|
> |				|   RO			|
> +-------------------------------+-----------------------+
> 
> Self-clearing is neither mentioned in the documentation nor
> implemented in hwmon core.

I would argue that self clearing is implied in "RO". This isn't a hwmon
core problem, it needs to be implemented in drivers. Many chips auto-clear
alarm attributes on read. For those this is automatic. Others need
to explicitly implement clearing alarms.

> 
> @Guenter:
> If alarm would just mean "current value > alarm threshold", then we
> wouldn't need an extra alarm attribute, as this is something which
> can be checked in user space.

Alarm attributes, if implemented properly and if a chip supports interrupts,
should generate sysfs and udev events to inform userspace. An alarm
doesn't just mean "current value > alarm threshold", it can also mean that
the current value was above the threshold at some point since the attribute
was read the last time. For that to work, the attribute must be sticky
until read.

FWIW, I am sure you'll find lots of drivers not implementing this properly,
so there is no need to search for those and use them as precedent.

If you want to support alarm attributes or not is obviously your call,
but they should be self clearing if implemented. I don't want to get complaints
along the line of "the alarm attribute is set but doesn't clear even though
the temperature (or voltage, or whatever) is below the threshold".

> Has it ever been considered that a user may have to explicitly ack
> an alarm to clear it? Would you consider it an ABI violation if
> alarm is configured as R/W for being able to clear the alarm?
> 

Yes.

Guenter

>> I'm wondering if we are heading towards ABI issues? You have defined:
>>
>> - over-temp alarm remains set, even if temperature drops below threshold
>>
>> so that kind of eliminates the possibility of implementing self
>> clearing any time in the future. Explicit clearing via a write is
>> probably O.K, because the user needs to take an explicit action.  Are
>> there other ABI issues i have not thought about.
>>
>> 	Andrew
> 
> Heiner

Re: [PATCH net-next 3/3] net: phy: realtek: add hwmon support for temp sensor on RTL822x

[Heiner Kallweit]

On 11.01.2025 01:20, Guenter Roeck wrote:
> On 1/10/25 13:41, Heiner Kallweit wrote:
>> On 10.01.2025 22:10, Andrew Lunn wrote:
>>>> - over-temp alarm remains set, even if temperature drops below threshold
>>>
>>>> +int rtl822x_hwmon_init(struct phy_device *phydev)
>>>> +{
>>>> +    struct device *hwdev, *dev = &phydev->mdio.dev;
>>>> +    const char *name;
>>>> +
>>>> +    /* Ensure over-temp alarm is reset. */
>>>> +    phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, RTL822X_VND2_TSALRM, 3);
>>>
>>> So it is possible to clear the alarm.
>>>
>>> I know you wanted to experiment with this some more....
>>>
>>> If the alarm is still set, does that prevent the PHY renegotiating the
>>> higher link speed? If you clear the alarm, does that allow it to
>>> renegotiate the higher link speed? Or is a down/up still required?
>>> Does an down/up clear the alarm if the temperature is below the
>>> threshold?
>>>
>> I tested wrt one of your previous questions, when exceeding the
>> temperature threshold the chip actually removes 2.5Gbps from the
>> advertisement register.
>>
>> If the alarm is set, the chip won't switch back automatically to
>> 2.5Gbps even if the temperature drops below the alarm threshold.
>>
>> When clearing the alarm the chip adds 2.5Gbps back to the advertisement
>> register. Worth to be mentioned:
>> The temperature is checked only if the link speed is 2.5Gbps.
>> Therefore the chip thinks it's safe to add back the 2.5Gbps mode
>> when the alarm is cleared.
>>
>> What I didn't test is whether it's possible to manually add 2.5Gbps
>> to the advertisement register whilst the alarm is set.
>> But I assume that's the case.
>>
>>> Also, does HWMON support clearing alarms? Writing a 0 to the file? Or
>>> are they supported to self clear on read?
>>>
>> Documentation/hwmon/sysfs-interface.rst states that the alarm
>> is a read-only attribute:
>>
>> +-------------------------------+-----------------------+
>> | **`in[0-*]_alarm`,        | Channel alarm        |
>> | `curr[1-*]_alarm`,        |            |
>> | `power[1-*]_alarm`,        |   - 0: no alarm    |
>> | `fan[1-*]_alarm`,        |   - 1: alarm        |
>> | `temp[1-*]_alarm`**        |            |
>> |                |   RO            |
>> +-------------------------------+-----------------------+
>>
>> Self-clearing is neither mentioned in the documentation nor
>> implemented in hwmon core.
> 
> I would argue that self clearing is implied in "RO". This isn't a hwmon
> core problem, it needs to be implemented in drivers. Many chips auto-clear
> alarm attributes on read. For those this is automatic. Others need
> to explicitly implement clearing alarms.
> 
Thanks a lot for the clarifications. Wrt RO and self-clearing see following
snippet from a PHY datasheet. These namings are quite common IMO. 
I think using RC in the alarm attribute description would be clearer.

Type Description
LH Latch high. If the status is high, this field is set to ‘1’ and remains set.
RC Read-cleared. The register field is cleared after read.
RO Read only.
WO Write only.
RW Read and Write.
SC Self-cleared. Writing a ‘1’ to this register field causes the function to be activated immediately, and then
the field will be automatically cleared to ‘0’.


>>
>> @Guenter:
>> If alarm would just mean "current value > alarm threshold", then we
>> wouldn't need an extra alarm attribute, as this is something which
>> can be checked in user space.
> 
> Alarm attributes, if implemented properly and if a chip supports interrupts,
> should generate sysfs and udev events to inform userspace. An alarm
> doesn't just mean "current value > alarm threshold", it can also mean that
> the current value was above the threshold at some point since the attribute
> was read the last time. For that to work, the attribute must be sticky
> until read.
> 
> FWIW, I am sure you'll find lots of drivers not implementing this properly,
> so there is no need to search for those and use them as precedent.
> 
> If you want to support alarm attributes or not is obviously your call,
> but they should be self clearing if implemented. I don't want to get complaints
> along the line of "the alarm attribute is set but doesn't clear even though
> the temperature (or voltage, or whatever) is below the threshold".
> 
>> Has it ever been considered that a user may have to explicitly ack
>> an alarm to clear it? Would you consider it an ABI violation if
>> alarm is configured as R/W for being able to clear the alarm?
>>
> 
> Yes.
> 
> Guenter
> 
>>> I'm wondering if we are heading towards ABI issues? You have defined:
>>>
>>> - over-temp alarm remains set, even if temperature drops below threshold
>>>
>>> so that kind of eliminates the possibility of implementing self
>>> clearing any time in the future. Explicit clearing via a write is
>>> probably O.K, because the user needs to take an explicit action.  Are
>>> there other ABI issues i have not thought about.
>>>
>>>     Andrew
>>
>> Heiner
> 

Re: [PATCH net-next 3/3] net: phy: realtek: add hwmon support for temp sensor on RTL822x

[Guenter Roeck]

On 1/10/25 13:10, Andrew Lunn wrote:
>> - over-temp alarm remains set, even if temperature drops below threshold
> 
>> +int rtl822x_hwmon_init(struct phy_device *phydev)
>> +{
>> +	struct device *hwdev, *dev = &phydev->mdio.dev;
>> +	const char *name;
>> +
>> +	/* Ensure over-temp alarm is reset. */
>> +	phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, RTL822X_VND2_TSALRM, 3);
> 
> So it is possible to clear the alarm.
> 
> I know you wanted to experiment with this some more....
> 
> If the alarm is still set, does that prevent the PHY renegotiating the
> higher link speed? If you clear the alarm, does that allow it to
> renegotiate the higher link speed? Or is a down/up still required?
> Does an down/up clear the alarm if the temperature is below the
> threshold?
> 
> Also, does HWMON support clearing alarms? Writing a 0 to the file? Or
> are they supported to self clear on read?
> 

Alarm attributes are supposed to self clear on read unless the condition
persists.

> I'm wondering if we are heading towards ABI issues? You have defined:
> 
> - over-temp alarm remains set, even if temperature drops below threshold
> 
> so that kind of eliminates the possibility of implementing self
> clearing any time in the future. Explicit clearing via a write is
> probably O.K, because the user needs to take an explicit action.  Are
> there other ABI issues i have not thought about.
> 

Alarm attributes are supposed to be read-only.

Guenter

Re: [PATCH net-next 3/3] net: phy: realtek: add hwmon support for temp sensor on RTL822x

[Heiner Kallweit]

On 10.01.2025 22:10, Andrew Lunn wrote:
>> - over-temp alarm remains set, even if temperature drops below threshold
> 
>> +int rtl822x_hwmon_init(struct phy_device *phydev)
>> +{
>> +	struct device *hwdev, *dev = &phydev->mdio.dev;
>> +	const char *name;
>> +
>> +	/* Ensure over-temp alarm is reset. */
>> +	phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, RTL822X_VND2_TSALRM, 3);
> 
> So it is possible to clear the alarm.
> 
> I know you wanted to experiment with this some more....
> 
> If the alarm is still set, does that prevent the PHY renegotiating the
> higher link speed? If you clear the alarm, does that allow it to
> renegotiate the higher link speed? Or is a down/up still required?
> Does an down/up clear the alarm if the temperature is below the
> threshold?
> 
> Also, does HWMON support clearing alarms? Writing a 0 to the file? Or
> are they supported to self clear on read?
> 
> I'm wondering if we are heading towards ABI issues? You have defined:
> 
> - over-temp alarm remains set, even if temperature drops below threshold
> 
> so that kind of eliminates the possibility of implementing self
> clearing any time in the future. Explicit clearing via a write is
> probably O.K, because the user needs to take an explicit action.  Are
> there other ABI issues i have not thought about.
> 

According to Guenters feedback the alarm attribute must not be written
and is expected to be self-clearing on read.
If we would clear the alarm in the chip on alarm attribute read, then
we can have the following ugly scenario:

1. Temperature threshold is exceeded and chip reduces speed to 1Gbps
2. Temperature is falling below alarm threshold
3. User uses "sensors" to check the current temperature
4. The implicit alarm attribute read causes the chip to clear the
   alarm and re-enable 2.5Gbps speed, resulting in the temperature
   alarm threshold being exceeded very soon again.

What isn't nice here is that it's not transparent to the user that
a read-only command from his perspective causes the protective measure
of the chip to be cancelled.

There's no existing hwmon attribute meant to be used by the user
to clear a hw alarm once he took measures to protect the chip
from overheating.

> 	Andrew

Heiner

Re: [PATCH net-next 3/3] net: phy: realtek: add hwmon support for temp sensor on RTL822x

[Andrew Lunn]

> According to Guenters feedback the alarm attribute must not be written
> and is expected to be self-clearing on read.
> If we would clear the alarm in the chip on alarm attribute read, then
> we can have the following ugly scenario:
> 
> 1. Temperature threshold is exceeded and chip reduces speed to 1Gbps
> 2. Temperature is falling below alarm threshold
> 3. User uses "sensors" to check the current temperature
> 4. The implicit alarm attribute read causes the chip to clear the
>    alarm and re-enable 2.5Gbps speed, resulting in the temperature
>    alarm threshold being exceeded very soon again.
> 
> What isn't nice here is that it's not transparent to the user that
> a read-only command from his perspective causes the protective measure
> of the chip to be cancelled.
> 
> There's no existing hwmon attribute meant to be used by the user
> to clear a hw alarm once he took measures to protect the chip
> from overheating.

It is generally not the kernels job to implement policy. User space
should be doing that.

I see two different possible policies, and there are maybe others:

1) The user is happy with one second outages every so often as the
chip cycles between too hot and down shifting, and cool enough to
upshift back to the higher speeds.

2) The user prefers to have reliable, slower connectivity and needs to
explicitly do something like down/up the interface to get it back to
the higher speed.

I personally would say, from a user support view, 2) is better. A one
time 1 second break in connectivity and a kernel message is going to
cause less issues.

Maybe the solution is that the hwmon alarm attribute is not directly
the hardware bit, but a software interpretation of the system state.
When the alarm fires, copy it into a software alarm state, but leave
the hardware alarm alone. A hwmon read clears the software state, but
leaves the hardware alone. A down/up of the interface will then clear
both the software and hardware alarm state.

Anybody wanting policy 1) would then need a daemon polling the state
and taking action. 2) would be the default.

How easy is it for you to get into the alarm state? Did you need an
environment chamber/oven, or is it happening for you with just lots of
continuous traffic at typical room temperature? Are we talking about
cheap USB dangles in a sealed plastic case with poor thermal design
are going to be doing this all the time?

	Andrew

Re: [PATCH net-next 3/3] net: phy: realtek: add hwmon support for temp sensor on RTL822x

[Heiner Kallweit]

On 11.01.2025 18:00, Andrew Lunn wrote:
>> According to Guenters feedback the alarm attribute must not be written
>> and is expected to be self-clearing on read.
>> If we would clear the alarm in the chip on alarm attribute read, then
>> we can have the following ugly scenario:
>>
>> 1. Temperature threshold is exceeded and chip reduces speed to 1Gbps
>> 2. Temperature is falling below alarm threshold
>> 3. User uses "sensors" to check the current temperature
>> 4. The implicit alarm attribute read causes the chip to clear the
>>    alarm and re-enable 2.5Gbps speed, resulting in the temperature
>>    alarm threshold being exceeded very soon again.
>>
>> What isn't nice here is that it's not transparent to the user that
>> a read-only command from his perspective causes the protective measure
>> of the chip to be cancelled.
>>
>> There's no existing hwmon attribute meant to be used by the user
>> to clear a hw alarm once he took measures to protect the chip
>> from overheating.
> 
> It is generally not the kernels job to implement policy. User space
> should be doing that.
> 
> I see two different possible policies, and there are maybe others:
> 
> 1) The user is happy with one second outages every so often as the
> chip cycles between too hot and down shifting, and cool enough to
> upshift back to the higher speeds.
> 
> 2) The user prefers to have reliable, slower connectivity and needs to
> explicitly do something like down/up the interface to get it back to
> the higher speed.
> 
This seems to be exactly how I do it currently.

> I personally would say, from a user support view, 2) is better. A one
> time 1 second break in connectivity and a kernel message is going to
> cause less issues.
> 
> Maybe the solution is that the hwmon alarm attribute is not directly
> the hardware bit, but a software interpretation of the system state.
> When the alarm fires, copy it into a software alarm state, but leave
> the hardware alarm alone. A hwmon read clears the software state, but
> leaves the hardware alone. A down/up of the interface will then clear
> both the software and hardware alarm state.
> 
Not clearing the alarm on read is better from a user perspective IMO
(at least for this specific PHY).
As long as the alarm is active, the chip forces a downshift. 

> Anybody wanting policy 1) would then need a daemon polling the state
> and taking action. 2) would be the default.
> 
> How easy is it for you to get into the alarm state? Did you need an
> environment chamber/oven, or is it happening for you with just lots of
> continuous traffic at typical room temperature? Are we talking about
> cheap USB dangles in a sealed plastic case with poor thermal design
> are going to be doing this all the time?
> 
I have a M.2 card with RTL8126 (w/o heat sink) and an external RJ45 port.
This card sits in a slot underneath the mainboard of a mini PC. At 2.5Gbps
it makes a big difference whether EEE is active. With EEE it reaches 54°C,
w/o EEE temperature quickly goes over 70°C. For tests I add a PHY write
to the code which sets the over-temp threshold to 60°C. Then I can
easily trigger overheating by disabling EEE.

On my system the over-temp threshold set by the BIOS (?) is 120°C.
Even w/o heat sink I can hardly imagine that this threshold is ever
reached.

> 	Andrew

Heiner

Re: [PATCH net-next 3/3] net: phy: realtek: add hwmon support for temp sensor on RTL822x

[Andrew Lunn]

> On my system the over-temp threshold set by the BIOS (?) is 120°C.
> Even w/o heat sink I can hardly imagine that this threshold is ever
> reached.

So this is to some extent a theoretical problem, in your setup. So i
would not spend too much time on it.

	Andrew

Re: [PATCH net-next 3/3] net: phy: realtek: add hwmon support for temp sensor on RTL822x

[Heiner Kallweit]

On 11.01.2025 18:44, Andrew Lunn wrote:
>> On my system the over-temp threshold set by the BIOS (?) is 120°C.
>> Even w/o heat sink I can hardly imagine that this threshold is ever
>> reached.
> 
> So this is to some extent a theoretical problem, in your setup. So i
> would not spend too much time on it.
> 
Yes, I think I'll omit the alarm feature and just expose current
temperature and over-temp threshold.

> 	Andrew

Heiner

Re: [PATCH net-next 3/3] net: phy: realtek: add hwmon support for temp sensor on RTL822x

[kernel test robot]

Hi Heiner,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Heiner-Kallweit/net-phy-realtek-add-support-for-reading-MDIO_MMD_VEND2-regs-on-RTL8125-RTL8126/20250110-195043
base:   net-next/main
patch link:    https://lore.kernel.org/r/dbfeb139-808f-4345-afe8-830b7f4da26a%40gmail.com
patch subject: [PATCH net-next 3/3] net: phy: realtek: add hwmon support for temp sensor on RTL822x
config: parisc-randconfig-r072-20250111 (https://download.01.org/0day-ci/archive/20250111/202501111618.Lro85HiT-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250111/202501111618.Lro85HiT-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501111618.Lro85HiT-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

>> drivers/net/phy/realtek_hwmon.c:3:10: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'support'
       3 |  * HWMON support for Realtek PHY's
         |          ^~~~~~~
>> drivers/net/phy/realtek_hwmon.c:3:33: warning: missing terminating ' character
       3 |  * HWMON support for Realtek PHY's
         |                                 ^
>> drivers/net/phy/realtek_hwmon.c:3:33: error: missing terminating ' character
       3 |  * HWMON support for Realtek PHY's
         |                                 ^~
>> drivers/net/phy/realtek_hwmon.c:5:39: error: stray '@' in program
       5 |  * Author: Heiner Kallweit <hkallweit1@gmail.com>
         |                                       ^
   In file included from include/uapi/asm-generic/types.h:7,
                    from ./arch/parisc/include/generated/uapi/asm/types.h:1,
                    from include/linux/bitops.h:5,
                    from include/linux/hwmon.h:15,
                    from drivers/net/phy/realtek_hwmon.c:8:
>> include/asm-generic/int-ll64.h:16:9: error: unknown type name '__s8'; did you mean '__u8'?
      16 | typedef __s8  s8;
         |         ^~~~
         |         __u8
   In file included from include/linux/kernel.h:27,
                    from arch/parisc/include/asm/bug.h:5,
                    from include/linux/bug.h:5,
                    from include/linux/thread_info.h:13,
                    from include/asm-generic/preempt.h:5,
                    from ./arch/parisc/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:79,
                    from include/linux/spinlock.h:56,
                    from include/linux/phy.h:15,
                    from drivers/net/phy/realtek_hwmon.c:9:
>> include/linux/math.h:112:9: error: unknown type name '__s8'; did you mean '__u8'?
     112 |         __##type numerator;                             \
         |         ^~
   include/linux/math.h:115:1: note: in expansion of macro '__STRUCT_FRACT'
     115 | __STRUCT_FRACT(s8)
         | ^~~~~~~~~~~~~~
   include/linux/math.h:113:9: error: unknown type name '__s8'; did you mean '__u8'?
     113 |         __##type denominator;                           \
         |         ^~
   include/linux/math.h:115:1: note: in expansion of macro '__STRUCT_FRACT'
     115 | __STRUCT_FRACT(s8)
         | ^~~~~~~~~~~~~~
   In file included from include/linux/quota.h:42,
                    from include/linux/fs.h:271,
                    from include/linux/compat.h:17,
                    from include/linux/ethtool.h:17,
                    from include/linux/phy.h:16:
>> include/uapi/linux/dqblk_xfs.h:54:9: error: unknown type name '__s8'; did you mean '__u8'?
      54 |         __s8            d_version;      /* version of this structure */
         |         ^~~~
         |         __u8
   include/uapi/linux/dqblk_xfs.h:55:9: error: unknown type name '__s8'; did you mean '__u8'?
      55 |         __s8            d_flags;        /* FS_{USER,PROJ,GROUP}_QUOTA */
         |         ^~~~
         |         __u8
   include/uapi/linux/dqblk_xfs.h:70:9: error: unknown type name '__s8'; did you mean '__u8'?
      70 |         __s8            d_itimer_hi;    /* upper 8 bits of timer values */
         |         ^~~~
         |         __u8
   include/uapi/linux/dqblk_xfs.h:71:9: error: unknown type name '__s8'; did you mean '__u8'?
      71 |         __s8            d_btimer_hi;
         |         ^~~~
         |         __u8
   include/uapi/linux/dqblk_xfs.h:72:9: error: unknown type name '__s8'; did you mean '__u8'?
      72 |         __s8            d_rtbtimer_hi;
         |         ^~~~
         |         __u8
   include/uapi/linux/dqblk_xfs.h:73:9: error: unknown type name '__s8'; did you mean '__u8'?
      73 |         __s8            d_padding2;     /* padding2 - for future use */
         |         ^~~~
         |         __u8
   include/uapi/linux/dqblk_xfs.h:166:9: error: unknown type name '__s8'; did you mean '__u8'?
     166 |         __s8            qs_version;     /* version number for future changes */
         |         ^~~~
         |         __u8
   include/uapi/linux/dqblk_xfs.h:168:9: error: unknown type name '__s8'; did you mean '__u8'?
     168 |         __s8            qs_pad;         /* unused */
         |         ^~~~
         |         __u8
   include/uapi/linux/dqblk_xfs.h:210:9: error: unknown type name '__s8'; did you mean '__u8'?
     210 |         __s8                    qs_version;     /* version for future changes */
         |         ^~~~
         |         __u8
   In file included from include/linux/ethtool.h:20:
>> include/uapi/linux/ethtool.h:2523:9: error: unknown type name '__s8'; did you mean '__u8'?
    2523 |         __s8    link_mode_masks_nwords;
         |         ^~~~
         |         __u8
--
   realtek_hwmon.c:3:10: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'support'
       3 |  * HWMON support for Realtek PHY's
         |          ^~~~~~~
   realtek_hwmon.c:3:33: warning: missing terminating ' character
       3 |  * HWMON support for Realtek PHY's
         |                                 ^
   realtek_hwmon.c:3:33: error: missing terminating ' character
       3 |  * HWMON support for Realtek PHY's
         |                                 ^~
   realtek_hwmon.c:5:39: error: stray '@' in program
       5 |  * Author: Heiner Kallweit <hkallweit1@gmail.com>
         |                                       ^
   In file included from include/uapi/asm-generic/types.h:7,
                    from arch/parisc/include/generated/uapi/asm/types.h:1,
                    from include/linux/bitops.h:5,
                    from include/linux/hwmon.h:15,
                    from realtek_hwmon.c:8:
>> include/asm-generic/int-ll64.h:16:9: error: unknown type name '__s8'; did you mean '__u8'?
      16 | typedef __s8  s8;
         |         ^~~~
         |         __u8
   In file included from include/linux/kernel.h:27,
                    from arch/parisc/include/asm/bug.h:5,
                    from include/linux/bug.h:5,
                    from include/linux/thread_info.h:13,
                    from include/asm-generic/preempt.h:5,
                    from arch/parisc/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:79,
                    from include/linux/spinlock.h:56,
                    from include/linux/phy.h:15,
                    from realtek_hwmon.c:9:
>> include/linux/math.h:112:9: error: unknown type name '__s8'; did you mean '__u8'?
     112 |         __##type numerator;                             \
         |         ^~
   include/linux/math.h:115:1: note: in expansion of macro '__STRUCT_FRACT'
     115 | __STRUCT_FRACT(s8)
         | ^~~~~~~~~~~~~~
   include/linux/math.h:113:9: error: unknown type name '__s8'; did you mean '__u8'?
     113 |         __##type denominator;                           \
         |         ^~
   include/linux/math.h:115:1: note: in expansion of macro '__STRUCT_FRACT'
     115 | __STRUCT_FRACT(s8)
         | ^~~~~~~~~~~~~~
   In file included from include/linux/quota.h:42,
                    from include/linux/fs.h:271,
                    from include/linux/compat.h:17,
                    from include/linux/ethtool.h:17,
                    from include/linux/phy.h:16:
>> include/uapi/linux/dqblk_xfs.h:54:9: error: unknown type name '__s8'; did you mean '__u8'?
      54 |         __s8            d_version;      /* version of this structure */
         |         ^~~~
         |         __u8
   include/uapi/linux/dqblk_xfs.h:55:9: error: unknown type name '__s8'; did you mean '__u8'?
      55 |         __s8            d_flags;        /* FS_{USER,PROJ,GROUP}_QUOTA */
         |         ^~~~
         |         __u8
   include/uapi/linux/dqblk_xfs.h:70:9: error: unknown type name '__s8'; did you mean '__u8'?
      70 |         __s8            d_itimer_hi;    /* upper 8 bits of timer values */
         |         ^~~~
         |         __u8
   include/uapi/linux/dqblk_xfs.h:71:9: error: unknown type name '__s8'; did you mean '__u8'?
      71 |         __s8            d_btimer_hi;
         |         ^~~~
         |         __u8
   include/uapi/linux/dqblk_xfs.h:72:9: error: unknown type name '__s8'; did you mean '__u8'?
      72 |         __s8            d_rtbtimer_hi;
         |         ^~~~
         |         __u8
   include/uapi/linux/dqblk_xfs.h:73:9: error: unknown type name '__s8'; did you mean '__u8'?
      73 |         __s8            d_padding2;     /* padding2 - for future use */
         |         ^~~~
         |         __u8
   include/uapi/linux/dqblk_xfs.h:166:9: error: unknown type name '__s8'; did you mean '__u8'?
     166 |         __s8            qs_version;     /* version number for future changes */
         |         ^~~~
         |         __u8
   include/uapi/linux/dqblk_xfs.h:168:9: error: unknown type name '__s8'; did you mean '__u8'?
     168 |         __s8            qs_pad;         /* unused */
         |         ^~~~
         |         __u8
   include/uapi/linux/dqblk_xfs.h:210:9: error: unknown type name '__s8'; did you mean '__u8'?
     210 |         __s8                    qs_version;     /* version for future changes */
         |         ^~~~
         |         __u8
   In file included from include/linux/ethtool.h:20:
>> include/uapi/linux/ethtool.h:2523:9: error: unknown type name '__s8'; did you mean '__u8'?
    2523 |         __s8    link_mode_masks_nwords;
         |         ^~~~
         |         __u8


vim +3 drivers/net/phy/realtek_hwmon.c

   > 3	 * HWMON support for Realtek PHY's
     4	 *
   > 5	 * Author: Heiner Kallweit <hkallweit1@gmail.com>
     6	 */
     7	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next 3/3] net: phy: realtek: add hwmon support for temp sensor on RTL822x

[kernel test robot]

Hi Heiner,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Heiner-Kallweit/net-phy-realtek-add-support-for-reading-MDIO_MMD_VEND2-regs-on-RTL8125-RTL8126/20250110-195043
base:   net-next/main
patch link:    https://lore.kernel.org/r/dbfeb139-808f-4345-afe8-830b7f4da26a%40gmail.com
patch subject: [PATCH net-next 3/3] net: phy: realtek: add hwmon support for temp sensor on RTL822x
config: i386-randconfig-013-20250111 (https://download.01.org/0day-ci/archive/20250111/202501111711.i1W0TGwl-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250111/202501111711.i1W0TGwl-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501111711.i1W0TGwl-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/net/phy/realtek_hwmon.c:3:4: error: type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int [-Wimplicit-int]
       2 |  *
         |  int
       3 |  * HWMON support for Realtek PHY's
         |    ^
>> drivers/net/phy/realtek_hwmon.c:3:9: error: expected ';' after top level declarator
       3 |  * HWMON support for Realtek PHY's
         |         ^
         |         ;
   drivers/net/phy/realtek_hwmon.c:3:33: warning: missing terminating ' character [-Winvalid-pp-token]
       3 |  * HWMON support for Realtek PHY's
         |                                 ^
   In file included from drivers/net/phy/realtek_hwmon.c:8:
   In file included from include/linux/hwmon.h:15:
   In file included from include/linux/bitops.h:5:
   In file included from ./arch/x86/include/generated/uapi/asm/types.h:1:
   In file included from include/uapi/asm-generic/types.h:7:
   include/asm-generic/int-ll64.h:16:9: error: unknown type name '__s8'; did you mean '__u8'?
      16 | typedef __s8  s8;
         |         ^~~~
         |         __u8
   include/uapi/asm-generic/int-ll64.h:21:23: note: '__u8' declared here
      21 | typedef unsigned char __u8;
         |                       ^
   In file included from drivers/net/phy/realtek_hwmon.c:9:
   In file included from include/linux/phy.h:15:
   In file included from include/linux/spinlock.h:60:
   In file included from include/linux/thread_info.h:60:
   In file included from arch/x86/include/asm/thread_info.h:59:
   In file included from arch/x86/include/asm/cpufeature.h:5:
   In file included from arch/x86/include/asm/processor.h:35:
   In file included from include/linux/math64.h:6:
   include/linux/math.h:115:1: error: unknown type name '__s8'; did you mean '__u8'?
     115 | __STRUCT_FRACT(s8)
         | ^
   include/linux/math.h:112:2: note: expanded from macro '__STRUCT_FRACT'
     112 |         __##type numerator;                             \
         |         ^
   <scratch space>:221:1: note: expanded from here
     221 | __s8
         | ^
   include/uapi/asm-generic/int-ll64.h:21:23: note: '__u8' declared here
      21 | typedef unsigned char __u8;
         |                       ^
   In file included from drivers/net/phy/realtek_hwmon.c:9:
   In file included from include/linux/phy.h:15:
   In file included from include/linux/spinlock.h:60:
   In file included from include/linux/thread_info.h:60:
   In file included from arch/x86/include/asm/thread_info.h:59:
   In file included from arch/x86/include/asm/cpufeature.h:5:
   In file included from arch/x86/include/asm/processor.h:35:
   In file included from include/linux/math64.h:6:
   include/linux/math.h:115:1: error: unknown type name '__s8'; did you mean '__u8'?
     115 | __STRUCT_FRACT(s8)
         | ^
   include/linux/math.h:113:2: note: expanded from macro '__STRUCT_FRACT'
     113 |         __##type denominator;                           \
         |         ^
   <scratch space>:222:1: note: expanded from here
     222 | __s8
         | ^
   include/uapi/asm-generic/int-ll64.h:21:23: note: '__u8' declared here
      21 | typedef unsigned char __u8;
         |                       ^
   In file included from drivers/net/phy/realtek_hwmon.c:9:
   In file included from include/linux/phy.h:16:
   In file included from include/linux/ethtool.h:17:
   In file included from include/linux/compat.h:17:
   In file included from include/linux/fs.h:33:
   In file included from include/linux/percpu-rwsem.h:7:
   In file included from include/linux/rcuwait.h:6:
   In file included from include/linux/sched/signal.h:6:
   include/linux/signal.h:98:11: warning: array index 3 is past the end of the array (that has type 'unsigned long[2]') [-Warray-bounds]
      98 |                 return (set->sig[3] | set->sig[2] |
         |                         ^        ~
   arch/x86/include/asm/signal.h:24:2: note: array 'sig' declared here
      24 |         unsigned long sig[_NSIG_WORDS];
         |         ^
   In file included from drivers/net/phy/realtek_hwmon.c:9:
   In file included from include/linux/phy.h:16:
   In file included from include/linux/ethtool.h:17:
   In file included from include/linux/compat.h:17:
   In file included from include/linux/fs.h:33:
   In file included from include/linux/percpu-rwsem.h:7:
   In file included from include/linux/rcuwait.h:6:
   In file included from include/linux/sched/signal.h:6:
   include/linux/signal.h:98:25: warning: array index 2 is past the end of the array (that has type 'unsigned long[2]') [-Warray-bounds]
      98 |                 return (set->sig[3] | set->sig[2] |
         |                                       ^        ~
   arch/x86/include/asm/signal.h:24:2: note: array 'sig' declared here
      24 |         unsigned long sig[_NSIG_WORDS];
         |         ^
   In file included from drivers/net/phy/realtek_hwmon.c:9:
   In file included from include/linux/phy.h:16:
   In file included from include/linux/ethtool.h:17:
   In file included from include/linux/compat.h:17:
   In file included from include/linux/fs.h:33:
   In file included from include/linux/percpu-rwsem.h:7:
   In file included from include/linux/rcuwait.h:6:
   In file included from include/linux/sched/signal.h:6:
   include/linux/signal.h:114:11: warning: array index 3 is past the end of the array (that has type 'const unsigned long[2]') [-Warray-bounds]
     114 |                 return  (set1->sig[3] == set2->sig[3]) &&
         |                          ^         ~
   arch/x86/include/asm/signal.h:24:2: note: array 'sig' declared here
      24 |         unsigned long sig[_NSIG_WORDS];
         |         ^


vim +/int +3 drivers/net/phy/realtek_hwmon.c

   > 3	 * HWMON support for Realtek PHY's
     4	 *
     5	 * Author: Heiner Kallweit <hkallweit1@gmail.com>
     6	 */
     7	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki