[PATCH net-next v2 3/6] eth: fbnic: cache hwmon sensor readings

[PATCH net-next v2 3/6] eth: fbnic: cache hwmon sensor readings

[Zinc Lim]

Each hwmon attribute access triggers its own TSENE firmware mailbox
round-trip, so reading the full set of attributes or polling them at a
high rate floods the firmware mailbox with quick, successive IPC messages
for data that barely changes between ticks.

Cache the last temperature and voltage reading and serve reads from it
for the remainder of the current jiffy. A single TSENE response carries
both readings, so one transaction on a miss refreshes both and satisfies
a whole batch of reads. The cache is seeded with the FBNIC_SENSOR_NO_DATA
sentinel at registration so the first read always refreshes, and
concurrent reads are serialized by the hwmon core so no additional
locking is required.

Signed-off-by: Zinc Lim <limzhineng2@gmail.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic.h       |  7 ++++
 drivers/net/ethernet/meta/fbnic/fbnic_fw.h    |  7 ++++
 drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c | 36 +++++++++++++------
 3 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic.h b/drivers/net/ethernet/meta/fbnic/fbnic.h
index d0715695c43e..f647ef07704b 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic.h
@@ -22,11 +22,18 @@ struct fbnic_napi_vector;
 #define FBNIC_MAX_NAPI_VECTORS		128u
 #define FBNIC_MBX_CMPL_SLOTS		4
 
+struct fbnic_hwmon_cache {
+	unsigned long last_read;
+	s32 temp_mdeg;
+	s32 volt_mv;
+};
+
 struct fbnic_dev {
 	struct device *dev;
 	struct net_device *netdev;
 	struct dentry *dbg_fbd;
 	struct device *hwmon;
+	struct fbnic_hwmon_cache hwmon_cache;
 	struct devlink_health_reporter *fw_reporter;
 	struct devlink_health_reporter *otp_reporter;
 
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
index d84723e4cfa3..42a5f83ddb45 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
@@ -6,6 +6,7 @@
 
 #include <linux/completion.h>
 #include <linux/if_ether.h>
+#include <linux/limits.h>
 #include <linux/types.h>
 
 struct fbnic_dev;
@@ -44,6 +45,12 @@ struct fbnic_fw_ver {
 	char commit[FBNIC_FW_CAP_RESP_COMMIT_MAX_SIZE];
 };
 
+/* Sentinel for a sensor value the driver does not have: a threshold the
+ * firmware never populated (older firmware) or a cache entry not yet
+ * refreshed.
+ */
+#define FBNIC_SENSOR_NO_DATA			S32_MIN
+
 struct fbnic_fw_cap {
 	struct {
 		struct fbnic_fw_ver mgmt, bootloader;
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
index 38bb26cb8e6c..f35cb0065093 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
@@ -2,6 +2,7 @@
 /* Copyright (c) Meta Platforms, Inc. and affiliates. */
 
 #include <linux/hwmon.h>
+#include <linux/jiffies.h>
 
 #include "fbnic.h"
 #include "fbnic_mac.h"
@@ -25,26 +26,32 @@ static umode_t fbnic_hwmon_is_visible(const void *drvdata,
 
 static int fbnic_hwmon_sensor_read(struct fbnic_dev *fbd, int id, long *val)
 {
+	struct fbnic_hwmon_cache *cache = &fbd->hwmon_cache;
 	struct fbnic_fw_completion *fw_cmpl;
 	int err = 0;
-	s32 *sensor;
-
-	fw_cmpl = fbnic_fw_alloc_cmpl(FBNIC_TLV_MSG_ID_TSENE_READ_RESP);
-	if (!fw_cmpl)
-		return -ENOMEM;
+	s32 *cached;
 
 	switch (id) {
 	case FBNIC_SENSOR_TEMP:
-		sensor = &fw_cmpl->u.tsene.millidegrees;
+		cached = &cache->temp_mdeg;
 		break;
 	case FBNIC_SENSOR_VOLTAGE:
-		sensor = &fw_cmpl->u.tsene.millivolts;
+		cached = &cache->volt_mv;
 		break;
 	default:
-		err = -EINVAL;
-		goto exit_free;
+		return -EINVAL;
+	}
+
+	if (*cached != FBNIC_SENSOR_NO_DATA &&
+	    time_is_after_eq_jiffies(cache->last_read)) {
+		*val = *cached;
+		return 0;
 	}
 
+	fw_cmpl = fbnic_fw_alloc_cmpl(FBNIC_TLV_MSG_ID_TSENE_READ_RESP);
+	if (!fw_cmpl)
+		return -ENOMEM;
+
 	err = fbnic_fw_xmit_tsene_read_msg(fbd, fw_cmpl);
 	if (err) {
 		dev_err(fbd->dev,
@@ -67,7 +74,12 @@ static int fbnic_hwmon_sensor_read(struct fbnic_dev *fbd, int id, long *val)
 		goto exit_cleanup;
 	}
 
-	*val = *sensor;
+	/* FW returns both readings in one response, cache both. */
+	cache->temp_mdeg = fw_cmpl->u.tsene.millidegrees;
+	cache->volt_mv = fw_cmpl->u.tsene.millivolts;
+	cache->last_read = jiffies;
+
+	*val = *cached;
 exit_cleanup:
 	fbnic_mbx_clear_cmpl(fbd, fw_cmpl);
 exit_free:
@@ -107,6 +119,10 @@ void fbnic_hwmon_register(struct fbnic_dev *fbd)
 	if (!IS_REACHABLE(CONFIG_HWMON))
 		return;
 
+	/* Seed cache with sentinel so the first read always refreshes. */
+	fbd->hwmon_cache.temp_mdeg = FBNIC_SENSOR_NO_DATA;
+	fbd->hwmon_cache.volt_mv = FBNIC_SENSOR_NO_DATA;
+
 	fbd->hwmon = hwmon_device_register_with_info(fbd->dev, "fbnic",
 						     fbd, &fbnic_chip_info,
 						     NULL);
-- 
2.53.0-Meta

[PATCH net-next v2 0/6] eth: fbnic: expand hwmon sensor support

[Zinc Lim]

fbnic currently exposes only the raw temperature and voltage readings
(temp1_input, in0_input) via hwmon. This series builds that out into a
complete sensor interface and hardens the read path.

The firmware capability response already carries per-board sensor
thresholds, and the firmware can asynchronously report when a sensor
crosses one of them. This series surfaces both to userspace and cleans
up the supporting plumbing:

 - Move the sensor read logic out of fbnic_mac, closer to its only
   caller in the hwmon code, and drop the now-unused get_sensor op.
 - Expose all hwmon attributes unconditionally as read-only (0444).
 - Cache the temperature and voltage readings for the current jiffy so
   a burst of attribute reads issues a single firmware round-trip.
 - Parse and expose the temperature (min/max/crit) and voltage
   (min/max) thresholds. Thresholds the firmware did not populate read
   back as -ENODATA.
 - Add the corresponding alarm attributes, computed by comparing a live
   reading against the stored thresholds.
 - Translate the firmware's sensor-threshold-exceeded message into an
   hwmon event so userspace is notified on the relevant attribute.

Changes from v1:
 - Addressed Jakub's patch 6 feedback on including a
   if (!IS_REACHABLE(CONFIG_HWMON)) return; guard.

Zinc Lim (6):
  eth: fbnic: move sensor read logic out of fbnic_mac
  eth: fbnic: expose all hwmon attributes unconditionally as read-only
  eth: fbnic: cache hwmon sensor readings
  eth: fbnic: report temperature and voltage thresholds via hwmon
  eth: fbnic: report temperature and voltage alarms via hwmon
  eth: fbnic: firmware notifies hwmon on sensor threshold events

 drivers/net/ethernet/meta/fbnic/fbnic.h       |   8 +
 drivers/net/ethernet/meta/fbnic/fbnic_fw.c    |  75 +++++
 drivers/net/ethernet/meta/fbnic/fbnic_fw.h    |  30 ++
 drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c | 263 ++++++++++++++++--
 drivers/net/ethernet/meta/fbnic/fbnic_mac.c   |  55 ----
 drivers/net/ethernet/meta/fbnic/fbnic_mac.h   |   2 -
 6 files changed, 349 insertions(+), 84 deletions(-)

--
2.53.0-Meta

[PATCH net-next v2 1/6] eth: fbnic: move sensor read logic out of fbnic_mac

[Zinc Lim]

The sensor read lived behind the fbnic_mac get_sensor op, but it is only
ever used by the hwmon subsystem. Move the read into fbnic_hwmon.c and
call it directly there, closer to where it is used, and drop the
now-unused get_sensor op from struct fbnic_mac. No functional change.

Signed-off-by: Zinc Lim <limzhineng2@gmail.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c | 56 ++++++++++++++++++-
 drivers/net/ethernet/meta/fbnic/fbnic_mac.c   | 55 ------------------
 drivers/net/ethernet/meta/fbnic/fbnic_mac.h   |  2 -
 3 files changed, 54 insertions(+), 59 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
index def8598aceec..6c8c66ab86c1 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
@@ -28,15 +28,67 @@ static umode_t fbnic_hwmon_is_visible(const void *drvdata,
 	return 0;
 }
 
+static int fbnic_hwmon_sensor_read(struct fbnic_dev *fbd, int id, long *val)
+{
+	struct fbnic_fw_completion *fw_cmpl;
+	int err = 0;
+	s32 *sensor;
+
+	fw_cmpl = fbnic_fw_alloc_cmpl(FBNIC_TLV_MSG_ID_TSENE_READ_RESP);
+	if (!fw_cmpl)
+		return -ENOMEM;
+
+	switch (id) {
+	case FBNIC_SENSOR_TEMP:
+		sensor = &fw_cmpl->u.tsene.millidegrees;
+		break;
+	case FBNIC_SENSOR_VOLTAGE:
+		sensor = &fw_cmpl->u.tsene.millivolts;
+		break;
+	default:
+		err = -EINVAL;
+		goto exit_free;
+	}
+
+	err = fbnic_fw_xmit_tsene_read_msg(fbd, fw_cmpl);
+	if (err) {
+		dev_err(fbd->dev,
+			"Failed to transmit TSENE read msg, err %d\n",
+			err);
+		goto exit_free;
+	}
+
+	if (!wait_for_completion_timeout(&fw_cmpl->done, 10 * HZ)) {
+		dev_err(fbd->dev, "Timed out waiting for TSENE read\n");
+		err = -ETIMEDOUT;
+		goto exit_cleanup;
+	}
+
+	/* Handle error returned by firmware */
+	if (fw_cmpl->result) {
+		err = fw_cmpl->result;
+		dev_err(fbd->dev, "%s: Firmware returned error %d\n",
+			__func__, err);
+		goto exit_cleanup;
+	}
+
+	*val = *sensor;
+exit_cleanup:
+	fbnic_mbx_clear_cmpl(fbd, fw_cmpl);
+exit_free:
+	fbnic_fw_put_cmpl(fw_cmpl);
+
+	return err;
+}
+
 static int fbnic_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 			    u32 attr, int channel, long *val)
 {
 	struct fbnic_dev *fbd = dev_get_drvdata(dev);
-	const struct fbnic_mac *mac = fbd->mac;
 	int id;
 
 	id = fbnic_hwmon_sensor_id(type);
-	return id < 0 ? id : mac->get_sensor(fbd, id, val);
+	return id < 0 ? id : fbnic_hwmon_sensor_read(fbd, id, val);
 }
 
 static const struct hwmon_ops fbnic_hwmon_ops = {
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_mac.c b/drivers/net/ethernet/meta/fbnic/fbnic_mac.c
index 53b7a938b4c2..fba2e2efaeb8 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_mac.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_mac.c
@@ -899,60 +899,6 @@ fbnic_mac_get_rmon_stats(struct fbnic_dev *fbd, bool reset,
 			    TMI_STAT_TX_PACKET_9217_MAX_BYTES);
 }
 
-static int fbnic_mac_get_sensor_asic(struct fbnic_dev *fbd, int id,
-				     long *val)
-{
-	struct fbnic_fw_completion *fw_cmpl;
-	int err = 0;
-	s32 *sensor;
-
-	fw_cmpl = fbnic_fw_alloc_cmpl(FBNIC_TLV_MSG_ID_TSENE_READ_RESP);
-	if (!fw_cmpl)
-		return -ENOMEM;
-
-	switch (id) {
-	case FBNIC_SENSOR_TEMP:
-		sensor = &fw_cmpl->u.tsene.millidegrees;
-		break;
-	case FBNIC_SENSOR_VOLTAGE:
-		sensor = &fw_cmpl->u.tsene.millivolts;
-		break;
-	default:
-		err = -EINVAL;
-		goto exit_free;
-	}
-
-	err = fbnic_fw_xmit_tsene_read_msg(fbd, fw_cmpl);
-	if (err) {
-		dev_err(fbd->dev,
-			"Failed to transmit TSENE read msg, err %d\n",
-			err);
-		goto exit_free;
-	}
-
-	if (!wait_for_completion_timeout(&fw_cmpl->done, 10 * HZ)) {
-		dev_err(fbd->dev, "Timed out waiting for TSENE read\n");
-		err = -ETIMEDOUT;
-		goto exit_cleanup;
-	}
-
-	/* Handle error returned by firmware */
-	if (fw_cmpl->result) {
-		err = fw_cmpl->result;
-		dev_err(fbd->dev, "%s: Firmware returned error %d\n",
-			__func__, err);
-		goto exit_cleanup;
-	}
-
-	*val = *sensor;
-exit_cleanup:
-	fbnic_mbx_clear_cmpl(fbd, fw_cmpl);
-exit_free:
-	fbnic_fw_put_cmpl(fw_cmpl);
-
-	return err;
-}
-
 static const struct fbnic_mac fbnic_mac_asic = {
 	.init_regs = fbnic_mac_init_regs,
 	.get_link = fbnic_mac_get_link,
@@ -966,7 +912,6 @@ static const struct fbnic_mac fbnic_mac_asic = {
 	.get_rmon_stats = fbnic_mac_get_rmon_stats,
 	.link_down = fbnic_mac_link_down_asic,
 	.link_up = fbnic_mac_link_up_asic,
-	.get_sensor = fbnic_mac_get_sensor_asic,
 };
 
 /**
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_mac.h b/drivers/net/ethernet/meta/fbnic/fbnic_mac.h
index 10f30e0e8f69..bde2daa65645 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_mac.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_mac.h
@@ -137,8 +137,6 @@ struct fbnic_mac {
 
 	void (*link_down)(struct fbnic_dev *fbd);
 	void (*link_up)(struct fbnic_dev *fbd, bool tx_pause, bool rx_pause);
-
-	int (*get_sensor)(struct fbnic_dev *fbd, int id, long *val);
 };
 
 int fbnic_mac_init(struct fbnic_dev *fbd);
-- 
2.53.0-Meta

[PATCH net-next v2 2/6] eth: fbnic: expose all hwmon attributes unconditionally as read-only

[Zinc Lim]

All fbnic hwmon attributes are read-only and always present, so
fbnic_hwmon_is_visible() can simply return 0444 for everything
instead of matching on the sensor type and attribute. This also
prepares for the threshold and alarm attributes added in the
following patches: they are exposed unconditionally and reads
return attribute values.

Signed-off-by: Zinc Lim <limzhineng2@gmail.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
index 6c8c66ab86c1..38bb26cb8e6c 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
@@ -20,12 +20,7 @@ static umode_t fbnic_hwmon_is_visible(const void *drvdata,
 				      enum hwmon_sensor_types type,
 				      u32 attr, int channel)
 {
-	if (type == hwmon_temp && attr == hwmon_temp_input)
-		return 0444;
-	if (type == hwmon_in && attr == hwmon_in_input)
-		return 0444;
-
-	return 0;
+	return 0444;
 }
 
 static int fbnic_hwmon_sensor_read(struct fbnic_dev *fbd, int id, long *val)
-- 
2.53.0-Meta

[PATCH net-next v2 3/6] eth: fbnic: cache hwmon sensor readings

[Zinc Lim]

Each hwmon attribute access triggers its own TSENE firmware mailbox
round-trip, so reading the full set of attributes or polling them at a
high rate floods the firmware mailbox with quick, successive IPC messages
for data that barely changes between ticks.

Cache the last temperature and voltage reading and serve reads from it
for the remainder of the current jiffy. A single TSENE response carries
both readings, so one transaction on a miss refreshes both and satisfies
a whole batch of reads. The cache is seeded with the FBNIC_SENSOR_NO_DATA
sentinel at registration so the first read always refreshes, and
concurrent reads are serialized by the hwmon core so no additional
locking is required.

Signed-off-by: Zinc Lim <limzhineng2@gmail.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic.h       |  7 ++++
 drivers/net/ethernet/meta/fbnic/fbnic_fw.h    |  7 ++++
 drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c | 36 +++++++++++++------
 3 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic.h b/drivers/net/ethernet/meta/fbnic/fbnic.h
index d0715695c43e..f647ef07704b 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic.h
@@ -22,11 +22,18 @@ struct fbnic_napi_vector;
 #define FBNIC_MAX_NAPI_VECTORS		128u
 #define FBNIC_MBX_CMPL_SLOTS		4
 
+struct fbnic_hwmon_cache {
+	unsigned long last_read;
+	s32 temp_mdeg;
+	s32 volt_mv;
+};
+
 struct fbnic_dev {
 	struct device *dev;
 	struct net_device *netdev;
 	struct dentry *dbg_fbd;
 	struct device *hwmon;
+	struct fbnic_hwmon_cache hwmon_cache;
 	struct devlink_health_reporter *fw_reporter;
 	struct devlink_health_reporter *otp_reporter;
 
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
index d84723e4cfa3..42a5f83ddb45 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
@@ -6,6 +6,7 @@
 
 #include <linux/completion.h>
 #include <linux/if_ether.h>
+#include <linux/limits.h>
 #include <linux/types.h>
 
 struct fbnic_dev;
@@ -44,6 +45,12 @@ struct fbnic_fw_ver {
 	char commit[FBNIC_FW_CAP_RESP_COMMIT_MAX_SIZE];
 };
 
+/* Sentinel for a sensor value the driver does not have: a threshold the
+ * firmware never populated (older firmware) or a cache entry not yet
+ * refreshed.
+ */
+#define FBNIC_SENSOR_NO_DATA			S32_MIN
+
 struct fbnic_fw_cap {
 	struct {
 		struct fbnic_fw_ver mgmt, bootloader;
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
index 38bb26cb8e6c..f35cb0065093 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
@@ -2,6 +2,7 @@
 /* Copyright (c) Meta Platforms, Inc. and affiliates. */
 
 #include <linux/hwmon.h>
+#include <linux/jiffies.h>
 
 #include "fbnic.h"
 #include "fbnic_mac.h"
@@ -25,26 +26,32 @@ static umode_t fbnic_hwmon_is_visible(const void *drvdata,
 
 static int fbnic_hwmon_sensor_read(struct fbnic_dev *fbd, int id, long *val)
 {
+	struct fbnic_hwmon_cache *cache = &fbd->hwmon_cache;
 	struct fbnic_fw_completion *fw_cmpl;
 	int err = 0;
-	s32 *sensor;
-
-	fw_cmpl = fbnic_fw_alloc_cmpl(FBNIC_TLV_MSG_ID_TSENE_READ_RESP);
-	if (!fw_cmpl)
-		return -ENOMEM;
+	s32 *cached;
 
 	switch (id) {
 	case FBNIC_SENSOR_TEMP:
-		sensor = &fw_cmpl->u.tsene.millidegrees;
+		cached = &cache->temp_mdeg;
 		break;
 	case FBNIC_SENSOR_VOLTAGE:
-		sensor = &fw_cmpl->u.tsene.millivolts;
+		cached = &cache->volt_mv;
 		break;
 	default:
-		err = -EINVAL;
-		goto exit_free;
+		return -EINVAL;
+	}
+
+	if (*cached != FBNIC_SENSOR_NO_DATA &&
+	    time_is_after_eq_jiffies(cache->last_read)) {
+		*val = *cached;
+		return 0;
 	}
 
+	fw_cmpl = fbnic_fw_alloc_cmpl(FBNIC_TLV_MSG_ID_TSENE_READ_RESP);
+	if (!fw_cmpl)
+		return -ENOMEM;
+
 	err = fbnic_fw_xmit_tsene_read_msg(fbd, fw_cmpl);
 	if (err) {
 		dev_err(fbd->dev,
@@ -67,7 +74,12 @@ static int fbnic_hwmon_sensor_read(struct fbnic_dev *fbd, int id, long *val)
 		goto exit_cleanup;
 	}
 
-	*val = *sensor;
+	/* FW returns both readings in one response, cache both. */
+	cache->temp_mdeg = fw_cmpl->u.tsene.millidegrees;
+	cache->volt_mv = fw_cmpl->u.tsene.millivolts;
+	cache->last_read = jiffies;
+
+	*val = *cached;
 exit_cleanup:
 	fbnic_mbx_clear_cmpl(fbd, fw_cmpl);
 exit_free:
@@ -107,6 +119,10 @@ void fbnic_hwmon_register(struct fbnic_dev *fbd)
 	if (!IS_REACHABLE(CONFIG_HWMON))
 		return;
 
+	/* Seed cache with sentinel so the first read always refreshes. */
+	fbd->hwmon_cache.temp_mdeg = FBNIC_SENSOR_NO_DATA;
+	fbd->hwmon_cache.volt_mv = FBNIC_SENSOR_NO_DATA;
+
 	fbd->hwmon = hwmon_device_register_with_info(fbd->dev, "fbnic",
 						     fbd, &fbnic_chip_info,
 						     NULL);
-- 
2.53.0-Meta

[PATCH net-next v2 4/6] eth: fbnic: report temperature and voltage thresholds via hwmon

[Zinc Lim]

The firmware capability response carries per-board temperature
(min/max/crit) and voltage (min/max) thresholds. Parse and store them in
fbnic_fw_cap, and expose them through the hwmon interface as
temp1_{min,max,crit} and in0_{min,max}.

The thresholds are always exposed. Values the firmware did not report are
stored as the FBNIC_SENSOR_NO_DATA sentinel in the capability response
parser, and a read of such an attribute returns -ENODATA.

Signed-off-by: Zinc Lim <limzhineng2@gmail.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_fw.c    | 21 ++++++
 drivers/net/ethernet/meta/fbnic/fbnic_fw.h    | 14 ++++
 drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c | 67 ++++++++++++++-----
 3 files changed, 87 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
index 283d25fae79e..d814bd4041a0 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
@@ -575,6 +575,11 @@ static const struct fbnic_tlv_index fbnic_fw_cap_resp_index[] = {
 	FBNIC_TLV_ATTR_STRING(FBNIC_FW_CAP_RESP_UEFI_COMMIT_STR,
 			      FBNIC_FW_CAP_RESP_COMMIT_MAX_SIZE),
 	FBNIC_TLV_ATTR_U32(FBNIC_FW_CAP_RESP_ANTI_ROLLBACK_VERSION),
+	FBNIC_TLV_ATTR_S32(FBNIC_FW_CAP_RESP_TEMP_MIN),
+	FBNIC_TLV_ATTR_S32(FBNIC_FW_CAP_RESP_TEMP_MAX),
+	FBNIC_TLV_ATTR_S32(FBNIC_FW_CAP_RESP_TEMP_CRIT),
+	FBNIC_TLV_ATTR_S32(FBNIC_FW_CAP_RESP_VOLT_MIN),
+	FBNIC_TLV_ATTR_S32(FBNIC_FW_CAP_RESP_VOLT_MAX),
 	FBNIC_TLV_ATTR_LAST
 };
 
@@ -702,6 +707,22 @@ static int fbnic_fw_parse_cap_resp(void *opaque, struct fbnic_tlv_msg **results)
 	/* Always assume we need a BMC reinit */
 	fbd->fw_cap.need_bmc_tcam_reinit = true;
 
+	fbd->fw_cap.temp.min =
+		fbnic_tlv_attr_get_signed(results[FBNIC_FW_CAP_RESP_TEMP_MIN],
+					  FBNIC_SENSOR_NO_DATA);
+	fbd->fw_cap.temp.max =
+		fbnic_tlv_attr_get_signed(results[FBNIC_FW_CAP_RESP_TEMP_MAX],
+					  FBNIC_SENSOR_NO_DATA);
+	fbd->fw_cap.temp.crit =
+		fbnic_tlv_attr_get_signed(results[FBNIC_FW_CAP_RESP_TEMP_CRIT],
+					  FBNIC_SENSOR_NO_DATA);
+	fbd->fw_cap.volt.min =
+		fbnic_tlv_attr_get_signed(results[FBNIC_FW_CAP_RESP_VOLT_MIN],
+					  FBNIC_SENSOR_NO_DATA);
+	fbd->fw_cap.volt.max =
+		fbnic_tlv_attr_get_signed(results[FBNIC_FW_CAP_RESP_VOLT_MAX],
+					  FBNIC_SENSOR_NO_DATA);
+
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
index 42a5f83ddb45..68ffd49e0cdd 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
@@ -51,6 +51,12 @@ struct fbnic_fw_ver {
  */
 #define FBNIC_SENSOR_NO_DATA			S32_MIN
 
+struct fbnic_threshold {
+	s32 min;
+	s32 max;
+	s32 crit;
+};
+
 struct fbnic_fw_cap {
 	struct {
 		struct fbnic_fw_ver mgmt, bootloader;
@@ -67,6 +73,8 @@ struct fbnic_fw_cap {
 	u8	link_speed;
 	u8	link_fec;
 	u32	anti_rollback_version;
+	struct fbnic_threshold temp;
+	struct fbnic_threshold volt;
 };
 
 struct fbnic_fw_completion {
@@ -249,6 +257,12 @@ enum {
 	FBNIC_FW_CAP_RESP_UEFI_VERSION			= 0x11,
 	FBNIC_FW_CAP_RESP_UEFI_COMMIT_STR		= 0x12,
 	FBNIC_FW_CAP_RESP_ANTI_ROLLBACK_VERSION		= 0x15,
+	/* 0x16 and 0x17 are reserved for future use */
+	FBNIC_FW_CAP_RESP_TEMP_MIN			= 0x18,
+	FBNIC_FW_CAP_RESP_TEMP_MAX			= 0x19,
+	FBNIC_FW_CAP_RESP_TEMP_CRIT			= 0x1a,
+	FBNIC_FW_CAP_RESP_VOLT_MIN			= 0x1b,
+	FBNIC_FW_CAP_RESP_VOLT_MAX			= 0x1c,
 	FBNIC_FW_CAP_RESP_MSG_MAX
 };
 
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
index f35cb0065093..4938f7b39140 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
@@ -7,16 +7,6 @@
 #include "fbnic.h"
 #include "fbnic_mac.h"
 
-static int fbnic_hwmon_sensor_id(enum hwmon_sensor_types type)
-{
-	if (type == hwmon_temp)
-		return FBNIC_SENSOR_TEMP;
-	if (type == hwmon_in)
-		return FBNIC_SENSOR_VOLTAGE;
-
-	return -EOPNOTSUPP;
-}
-
 static umode_t fbnic_hwmon_is_visible(const void *drvdata,
 				      enum hwmon_sensor_types type,
 				      u32 attr, int channel)
@@ -88,14 +78,58 @@ static int fbnic_hwmon_sensor_read(struct fbnic_dev *fbd, int id, long *val)
 	return err;
 }
 
+static int fbnic_hwmon_read_threshold(long thr, long *val)
+{
+	if (thr == FBNIC_SENSOR_NO_DATA)
+		return -ENODATA;
+
+	*val = thr;
+	return 0;
+}
+
+static int fbnic_hwmon_temp_read(struct fbnic_dev *fbd, u32 attr, long *val)
+{
+	switch (attr) {
+	case hwmon_temp_input:
+		return fbnic_hwmon_sensor_read(fbd, FBNIC_SENSOR_TEMP, val);
+	case hwmon_temp_min:
+		return fbnic_hwmon_read_threshold(fbd->fw_cap.temp.min, val);
+	case hwmon_temp_max:
+		return fbnic_hwmon_read_threshold(fbd->fw_cap.temp.max, val);
+	case hwmon_temp_crit:
+		return fbnic_hwmon_read_threshold(fbd->fw_cap.temp.crit, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int fbnic_hwmon_in_read(struct fbnic_dev *fbd, u32 attr, long *val)
+{
+	switch (attr) {
+	case hwmon_in_input:
+		return fbnic_hwmon_sensor_read(fbd, FBNIC_SENSOR_VOLTAGE, val);
+	case hwmon_in_min:
+		return fbnic_hwmon_read_threshold(fbd->fw_cap.volt.min, val);
+	case hwmon_in_max:
+		return fbnic_hwmon_read_threshold(fbd->fw_cap.volt.max, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static int fbnic_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 			    u32 attr, int channel, long *val)
 {
 	struct fbnic_dev *fbd = dev_get_drvdata(dev);
-	int id;
 
-	id = fbnic_hwmon_sensor_id(type);
-	return id < 0 ? id : fbnic_hwmon_sensor_read(fbd, id, val);
+	switch (type) {
+	case hwmon_temp:
+		return fbnic_hwmon_temp_read(fbd, attr, val);
+	case hwmon_in:
+		return fbnic_hwmon_in_read(fbd, attr, val);
+	default:
+		return -EOPNOTSUPP;
+	}
 }
 
 static const struct hwmon_ops fbnic_hwmon_ops = {
@@ -104,8 +138,11 @@ static const struct hwmon_ops fbnic_hwmon_ops = {
 };
 
 static const struct hwmon_channel_info *fbnic_hwmon_info[] = {
-	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
-	HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX |
+			   HWMON_T_CRIT),
+	HWMON_CHANNEL_INFO(in,
+			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX),
 	NULL
 };
 
-- 
2.53.0-Meta

[PATCH net-next v2 5/6] eth: fbnic: report temperature and voltage alarms via hwmon

[Zinc Lim]

Building on the temperature and voltage thresholds stored in
fbnic_fw_cap, expose alarm attributes through the hwmon interface:
temp1_{min,max,crit}_alarm and in0_{min,max}_alarm.

Each alarm is computed by taking a live sensor reading and comparing it
against the corresponding stored threshold. The static thresholds
(min/max/crit) are returned first straight from fbnic_fw_cap without a
firmware round-trip, and unsupported attributes are rejected up front, so
only attributes that actually need a live value fall through to a single
sensor read that then feeds input and every alarm.

A threshold the firmware did not populate reports -ENODATA for both the
threshold attribute and its alarm.

Signed-off-by: Zinc Lim <limzhineng2@gmail.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c | 68 +++++++++++++++++--
 1 file changed, 61 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
index 4938f7b39140..c5cddd9cef12 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
@@ -87,34 +87,84 @@ static int fbnic_hwmon_read_threshold(long thr, long *val)
 	return 0;
 }
 
+static int fbnic_hwmon_read_alarm(long a, long b, long *val)
+{
+	if (a == FBNIC_SENSOR_NO_DATA || b == FBNIC_SENSOR_NO_DATA)
+		return -ENODATA;
+
+	*val = a >= b;
+	return 0;
+}
+
 static int fbnic_hwmon_temp_read(struct fbnic_dev *fbd, u32 attr, long *val)
 {
+	int err;
+
 	switch (attr) {
-	case hwmon_temp_input:
-		return fbnic_hwmon_sensor_read(fbd, FBNIC_SENSOR_TEMP, val);
 	case hwmon_temp_min:
 		return fbnic_hwmon_read_threshold(fbd->fw_cap.temp.min, val);
 	case hwmon_temp_max:
 		return fbnic_hwmon_read_threshold(fbd->fw_cap.temp.max, val);
 	case hwmon_temp_crit:
 		return fbnic_hwmon_read_threshold(fbd->fw_cap.temp.crit, val);
+	case hwmon_temp_input:
+	case hwmon_temp_min_alarm:
+	case hwmon_temp_max_alarm:
+	case hwmon_temp_crit_alarm:
+		break;
 	default:
 		return -EOPNOTSUPP;
 	}
+
+	err = fbnic_hwmon_sensor_read(fbd, FBNIC_SENSOR_TEMP, val);
+	if (err)
+		return err;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		return 0;
+	case hwmon_temp_min_alarm:
+		return fbnic_hwmon_read_alarm(fbd->fw_cap.temp.min, *val, val);
+	case hwmon_temp_max_alarm:
+		return fbnic_hwmon_read_alarm(*val, fbd->fw_cap.temp.max, val);
+	case hwmon_temp_crit_alarm:
+		return fbnic_hwmon_read_alarm(*val, fbd->fw_cap.temp.crit, val);
+	}
+
+	return -EOPNOTSUPP;
 }
 
 static int fbnic_hwmon_in_read(struct fbnic_dev *fbd, u32 attr, long *val)
 {
+	int err;
+
 	switch (attr) {
-	case hwmon_in_input:
-		return fbnic_hwmon_sensor_read(fbd, FBNIC_SENSOR_VOLTAGE, val);
 	case hwmon_in_min:
 		return fbnic_hwmon_read_threshold(fbd->fw_cap.volt.min, val);
 	case hwmon_in_max:
 		return fbnic_hwmon_read_threshold(fbd->fw_cap.volt.max, val);
+	case hwmon_in_input:
+	case hwmon_in_min_alarm:
+	case hwmon_in_max_alarm:
+		break;
 	default:
 		return -EOPNOTSUPP;
 	}
+
+	err = fbnic_hwmon_sensor_read(fbd, FBNIC_SENSOR_VOLTAGE, val);
+	if (err)
+		return err;
+
+	switch (attr) {
+	case hwmon_in_input:
+		return 0;
+	case hwmon_in_min_alarm:
+		return fbnic_hwmon_read_alarm(fbd->fw_cap.volt.min, *val, val);
+	case hwmon_in_max_alarm:
+		return fbnic_hwmon_read_alarm(*val, fbd->fw_cap.volt.max, val);
+	}
+
+	return -EOPNOTSUPP;
 }
 
 static int fbnic_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
@@ -139,10 +189,14 @@ static const struct hwmon_ops fbnic_hwmon_ops = {
 
 static const struct hwmon_channel_info *fbnic_hwmon_info[] = {
 	HWMON_CHANNEL_INFO(temp,
-			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX |
-			   HWMON_T_CRIT),
+			   HWMON_T_INPUT |
+			   HWMON_T_MIN | HWMON_T_MIN_ALARM |
+			   HWMON_T_MAX | HWMON_T_MAX_ALARM |
+			   HWMON_T_CRIT | HWMON_T_CRIT_ALARM),
 	HWMON_CHANNEL_INFO(in,
-			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX),
+			   HWMON_I_INPUT |
+			   HWMON_I_MIN | HWMON_I_MIN_ALARM |
+			   HWMON_I_MAX | HWMON_I_MAX_ALARM),
 	NULL
 };
 
-- 
2.53.0-Meta

[PATCH net-next v2 6/6] eth: fbnic: firmware notifies hwmon on sensor threshold events

[Zinc Lim]

The firmware sends an unsolicited message via the new
FBNIC_TLV_MSG_ID_SENSOR_THRESHOLD_EXCEEDED_RESP IPC message when a
temperature or voltage sensor crosses one of its thresholds. Parse this
message and translate it into the corresponding hwmon alarm
(temp1_{min,max,crit}_alarm or in0_{min,max}_alarm) via
hwmon_notify_event(), so userspace listeners are woken on the relevant
sysfs attribute.

fbnic_hwmon_notify_event() is driven from the FW mailbox IRQ path, so it
can run concurrently with hwmon registration and teardown. Guard the
publish/teardown of fbd->hwmon: register publishes it with WRITE_ONCE()
only after a successful registration (and leaves it NULL on failure),
unregister clears it with WRITE_ONCE(NULL) and then
synchronize_irq(fbd->fw_msix_vector) to drain any in-flight mailbox IRQ
before unregistering, and notify_event reads it once with READ_ONCE() and
skips the notification when it is NULL.

Signed-off-by: Zinc Lim <limzhineng2@gmail.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic.h       |  1 +
 drivers/net/ethernet/meta/fbnic/fbnic_fw.c    | 54 ++++++++++++++
 drivers/net/ethernet/meta/fbnic/fbnic_fw.h    |  9 +++
 drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c | 71 ++++++++++++++++---
 4 files changed, 127 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic.h b/drivers/net/ethernet/meta/fbnic/fbnic.h
index f647ef07704b..4a49c20e4a01 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic.h
@@ -192,6 +192,7 @@ void fbnic_fw_free_mbx(struct fbnic_dev *fbd);
 
 void fbnic_hwmon_register(struct fbnic_dev *fbd);
 void fbnic_hwmon_unregister(struct fbnic_dev *fbd);
+void fbnic_hwmon_notify_event(struct fbnic_dev *fbd, int id, long val);
 
 int fbnic_mac_request_irq(struct fbnic_dev *fbd);
 void fbnic_mac_free_irq(struct fbnic_dev *fbd);
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
index d814bd4041a0..6dca38076d7b 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
@@ -1639,6 +1639,57 @@ fbnic_fw_parser_test(void *opaque, struct fbnic_tlv_msg **results)
 	return err;
 }
 
+static const struct fbnic_tlv_index fbnic_threshold_exceeded_resp_index[] = {
+	FBNIC_TLV_ATTR_S32(FBNIC_FW_TSENE_THERM_EXCEEDED_FLAG),
+	FBNIC_TLV_ATTR_S32(FBNIC_FW_TSENE_VOLT_EXCEEDED_FLAG),
+	FBNIC_TLV_ATTR_S32(FBNIC_FW_TSENE_THERMAL),
+	FBNIC_TLV_ATTR_S32(FBNIC_FW_TSENE_VOLTAGE),
+	FBNIC_TLV_ATTR_LAST
+};
+
+static int fbnic_fw_parse_threshold_exceeded_resp(void *opaque,
+						  struct fbnic_tlv_msg **results)
+{
+	bool therm_exceeded, volt_exceeded;
+	struct fbnic_dev *fbd = opaque;
+	s32 value;
+
+	therm_exceeded =
+		fta_get_sint(results, FBNIC_FW_TSENE_THERM_EXCEEDED_FLAG);
+	volt_exceeded =
+		fta_get_sint(results, FBNIC_FW_TSENE_VOLT_EXCEEDED_FLAG);
+
+	if (!therm_exceeded && !volt_exceeded) {
+		dev_err(fbd->dev,
+			"Threshold exceeded message with no flag set\n");
+		return -EINVAL;
+	}
+
+	if (therm_exceeded) {
+		if (!results[FBNIC_FW_TSENE_THERMAL]) {
+			dev_err(fbd->dev,
+				"Thermal threshold exceeded but no value received\n");
+			return -EINVAL;
+		}
+		value = fta_get_sint(results, FBNIC_FW_TSENE_THERMAL);
+		dev_err(fbd->dev, "Thermal threshold exceeded: %d mC\n", value);
+		fbnic_hwmon_notify_event(fbd, FBNIC_SENSOR_TEMP, value);
+	}
+
+	if (volt_exceeded) {
+		if (!results[FBNIC_FW_TSENE_VOLTAGE]) {
+			dev_err(fbd->dev,
+				"Voltage threshold exceeded but no value received\n");
+			return -EINVAL;
+		}
+		value = fta_get_sint(results, FBNIC_FW_TSENE_VOLTAGE);
+		dev_err(fbd->dev, "Voltage threshold exceeded: %d mV\n", value);
+		fbnic_hwmon_notify_event(fbd, FBNIC_SENSOR_VOLTAGE, value);
+	}
+
+	return 0;
+}
+
 static const struct fbnic_tlv_parser fbnic_fw_tlv_parser[] = {
 	FBNIC_TLV_PARSER(TEST, fbnic_tlv_test_index, fbnic_fw_parser_test),
 	FBNIC_TLV_PARSER(FW_CAP_RESP, fbnic_fw_cap_resp_index,
@@ -1667,6 +1718,9 @@ static const struct fbnic_tlv_parser fbnic_fw_tlv_parser[] = {
 	FBNIC_TLV_PARSER(TSENE_READ_RESP,
 			 fbnic_tsene_read_resp_index,
 			 fbnic_fw_parse_tsene_read_resp),
+	FBNIC_TLV_PARSER(SENSOR_THRESHOLD_EXCEEDED_RESP,
+			 fbnic_threshold_exceeded_resp_index,
+			 fbnic_fw_parse_threshold_exceeded_resp),
 	FBNIC_TLV_PARSER(LOG_MSG_REQ,
 			 fbnic_fw_log_req_index,
 			 fbnic_fw_parse_log_req),
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
index 68ffd49e0cdd..87301e608255 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
@@ -227,6 +227,7 @@ enum {
 	FBNIC_TLV_MSG_ID_QSFP_READ_RESP			= 0x39,
 	FBNIC_TLV_MSG_ID_TSENE_READ_REQ			= 0x3C,
 	FBNIC_TLV_MSG_ID_TSENE_READ_RESP		= 0x3D,
+	FBNIC_TLV_MSG_ID_SENSOR_THRESHOLD_EXCEEDED_RESP = 0x40,
 	FBNIC_TLV_MSG_ID_LOG_SEND_LOGS_REQ		= 0x43,
 	FBNIC_TLV_MSG_ID_LOG_MSG_REQ			= 0x44,
 	FBNIC_TLV_MSG_ID_LOG_MSG_RESP			= 0x45,
@@ -296,6 +297,14 @@ enum {
 	FBNIC_FW_TSENE_MSG_MAX
 };
 
+enum {
+	FBNIC_FW_TSENE_THERM_EXCEEDED_FLAG	= 0x0,
+	FBNIC_FW_TSENE_VOLT_EXCEEDED_FLAG	= 0x1,
+	FBNIC_FW_TSENE_THERMAL			= 0x2,
+	FBNIC_FW_TSENE_VOLTAGE			= 0x3,
+	FBNIC_FW_TSENE_EXCEEDED_MSG_MAX,
+};
+
 enum {
 	FBNIC_FW_OWNERSHIP_FLAG			= 0x0,
 	FBNIC_FW_OWNERSHIP_TIME			= 0x1,
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
index c5cddd9cef12..3f33522fcfd2 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
@@ -207,6 +207,8 @@ static const struct hwmon_chip_info fbnic_chip_info = {
 
 void fbnic_hwmon_register(struct fbnic_dev *fbd)
 {
+	struct device *hwmon;
+
 	if (!IS_REACHABLE(CONFIG_HWMON))
 		return;
 
@@ -214,22 +216,75 @@ void fbnic_hwmon_register(struct fbnic_dev *fbd)
 	fbd->hwmon_cache.temp_mdeg = FBNIC_SENSOR_NO_DATA;
 	fbd->hwmon_cache.volt_mv = FBNIC_SENSOR_NO_DATA;
 
-	fbd->hwmon = hwmon_device_register_with_info(fbd->dev, "fbnic",
-						     fbd, &fbnic_chip_info,
-						     NULL);
-	if (IS_ERR(fbd->hwmon)) {
+	hwmon = hwmon_device_register_with_info(fbd->dev, "fbnic", fbd,
+						&fbnic_chip_info, NULL);
+	if (IS_ERR(hwmon)) {
 		dev_notice(fbd->dev,
 			   "Failed to register hwmon device %pe\n",
-			   fbd->hwmon);
-		fbd->hwmon = NULL;
+			   hwmon);
+		return;
 	}
+
+	WRITE_ONCE(fbd->hwmon, hwmon);
 }
 
 void fbnic_hwmon_unregister(struct fbnic_dev *fbd)
 {
+	struct device *hwmon;
+
 	if (!IS_REACHABLE(CONFIG_HWMON) || !fbd->hwmon)
 		return;
 
-	hwmon_device_unregister(fbd->hwmon);
-	fbd->hwmon = NULL;
+	hwmon = fbd->hwmon;
+	/* Pair with READ_ONCE() in fbnic_hwmon_notify_event(). Publish NULL
+	 * and wait for any in-flight FW mailbox IRQ handler to finish so it
+	 * cannot dereference the hwmon device after we unregister it.
+	 */
+	WRITE_ONCE(fbd->hwmon, NULL);
+	synchronize_irq(fbd->fw_msix_vector);
+
+	hwmon_device_unregister(hwmon);
+}
+
+void fbnic_hwmon_notify_event(struct fbnic_dev *fbd, int id, long val)
+{
+	enum hwmon_sensor_types type;
+	struct device *hwmon;
+	s32 attr = -1;
+
+	if (!IS_REACHABLE(CONFIG_HWMON))
+		return;
+
+	switch (id) {
+	case FBNIC_SENSOR_TEMP:
+		type = hwmon_temp;
+
+		if (val <= fbd->fw_cap.temp.min)
+			attr = hwmon_temp_min_alarm;
+		else if (val >= fbd->fw_cap.temp.crit)
+			attr = hwmon_temp_crit_alarm;
+		else if (val >= fbd->fw_cap.temp.max)
+			attr = hwmon_temp_max_alarm;
+
+		break;
+	case FBNIC_SENSOR_VOLTAGE:
+		type = hwmon_in;
+
+		if (val <= fbd->fw_cap.volt.min)
+			attr = hwmon_in_min_alarm;
+		else if (val >= fbd->fw_cap.volt.max)
+			attr = hwmon_in_max_alarm;
+
+		break;
+	default:
+		return;
+	}
+
+	/* Pair with WRITE_ONCE() in fbnic_hwmon_unregister(). Skip the
+	 * notification if hwmon failed to register or has already been torn
+	 * down.
+	 */
+	hwmon = READ_ONCE(fbd->hwmon);
+	if (attr >= 0 && hwmon)
+		hwmon_notify_event(hwmon, type, attr, 0);
 }
-- 
2.53.0-Meta

Re: [net-next,v2,2/6] eth: fbnic: expose all hwmon attributes unconditionally as read-only

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] fbnic_hwmon_is_visible() is reduced to an unconditional `return
  0444;` that ignores all four of its arguments (drvdata, type, attr,…

--

eth: fbnic: expose all hwmon attributes unconditionally as read-only

This changes fbnic_hwmon_is_visible() so that it returns 0444 for every
sensor type and attribute instead of matching on hwmon_temp_input and
hwmon_in_input.  The stated motivation is to prepare for threshold and
alarm attributes added later in the series.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index 6c8c66ab86c13..38bb26cb8e6cb 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> @@ -20,12 +20,7 @@ static umode_t fbnic_hwmon_is_visible(const void *drvdata,
>  				      enum hwmon_sensor_types type,
>  				      u32 attr, int channel)
>  {
> -	if (type == hwmon_temp && attr == hwmon_temp_input)
> -		return 0444;
> -	if (type == hwmon_in && attr == hwmon_in_input)
> -		return 0444;
> -
> -	return 0;
> +	return 0444;
>  }

[Severity: Low]
This isn't a bug, but now that fbnic_hwmon_is_visible() ignores all four
of its parameters (drvdata, type, attr and channel) and returns a constant,
could the static visibility field in struct hwmon_ops be used instead?

include/linux/hwmon.h documents it that way:

  /**
   * struct hwmon_ops - hwmon device operations
   * @visible:	Static visibility. If non-zero, 'is_visible' is ignored.
   * @is_visible: Callback to return attribute visibility. Mandatory unless
   *		'visible' is non-zero.

and hwmon_device_register_with_info() accepts either form, so
fbnic_hwmon_ops could become:

  static const struct hwmon_ops fbnic_hwmon_ops = {
	.visible = 0444,
	.read = fbnic_hwmon_read,
  };

with fbnic_hwmon_is_visible() deleted entirely.  Other drivers such as
drivers/net/phy/realtek/realtek_hwmon.c already use the .visible form.

This shape persists through the end of the series (fbnic_hwmon_is_visible()
is still a plain "return 0444;" installed via .is_visible at the last
patch), so a later patch does not convert it.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831212234.1212971-1-limzhineng2%40gmail.com

Re: [net-next,v2,3/6] eth: fbnic: cache hwmon sensor readings

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] Using `time_is_after_eq_jiffies()` with a past timestamp
  causes up to 24.8 days of stale cache hits due to integer wrap-around.
- [Low] Concern (judged a false positive): the comment added above
  FBNIC_SENSOR_NO_DATA in…

--

eth: fbnic: cache hwmon sensor readings

This adds a small per-device cache for the TSENE temperature and voltage
readings so hwmon attribute reads within the same jiffy are served from
the cache instead of issuing a firmware mailbox round-trip each time.
The cache is seeded with FBNIC_SENSOR_NO_DATA at registration so the
first read always refreshes.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
> index d84723e4cfa36..42a5f83ddb45e 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h

[ ... ]

> @@ -44,6 +45,12 @@ struct fbnic_fw_ver {
>  	char commit[FBNIC_FW_CAP_RESP_COMMIT_MAX_SIZE];
>  };
>  
> +/* Sentinel for a sensor value the driver does not have: a threshold the
> + * firmware never populated (older firmware) or a cache entry not yet
> + * refreshed.
> + */
> +#define FBNIC_SENSOR_NO_DATA			S32_MIN
> +

[Severity: Low]
This isn't a bug, but the comment describes two uses of the sentinel while
only one of them exists at this commit.

Grepping the driver at this commit, FBNIC_SENSOR_NO_DATA is referenced only
by the cache seeding in fbnic_hwmon_register() and by the cache validity
test in fbnic_hwmon_sensor_read():

  fbnic_fw.h:  #define FBNIC_SENSOR_NO_DATA  S32_MIN
  fbnic_hwmon.c:  if (*cached != FBNIC_SENSOR_NO_DATA &&
  fbnic_hwmon.c:  fbd->hwmon_cache.temp_mdeg = FBNIC_SENSOR_NO_DATA;
  fbnic_hwmon.c:  fbd->hwmon_cache.volt_mv = FBNIC_SENSOR_NO_DATA;

struct fbnic_fw_cap has no temperature or voltage threshold members yet, so
"a threshold the firmware never populated (older firmware)" has no consumer
here.

The threshold and alarm users (fbnic_hwmon_read_threshold() and
fbnic_hwmon_read_alarm(), together with struct fbnic_threshold in struct
fbnic_fw_cap) do appear in later patches of this same series, so the wording
is accurate once the whole series is applied. Would it be clearer to
introduce the threshold half of the comment in the patch that adds the
threshold users?

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index 38bb26cb8e6cb..f35cb00650937 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c

[ ... ]

> @@ -25,26 +26,32 @@ static umode_t fbnic_hwmon_is_visible(const void *drvdata,
> +	if (*cached != FBNIC_SENSOR_NO_DATA &&
> +	    time_is_after_eq_jiffies(cache->last_read)) {
> +		*val = *cached;
> +		return 0;
>  	}

[Severity: Medium]
The cache validity test uses the wrong end of the jiffies helper
family. time_is_after_eq_jiffies(a) expands to
time_before_eq(jiffies, a), i.e. it is meant to be applied to a
deadline that lies in the future, but cache->last_read is a timestamp
in the past (it is assigned plain jiffies right after the TSENE
response is parsed). It happens to give the intended "valid for the
remainder of the current jiffy" behaviour only because
time_before_eq() is a signed difference that goes negative one tick
later.

That signed difference is the problem: once jiffies - last_read grows
past LONG_MAX/2 (roughly 24.8 days at HZ=1000 where jiffies is 32
bits) the comparison flips back to true, and it stays true for the
next ~24.8 days. By that point *cached holds a real reading rather
than FBNIC_SENSOR_NO_DATA, so the sentinel check in front of it does
not help either, and fbnic_hwmon_sensor_read() will happily hand out
a temp_mdeg/volt_mv pair that is weeks old for temp_input,
in0_input and every alarm derived from them, with no mailbox traffic
and no error to hint at it. An idle device whose hwmon attributes are
only read occasionally is exactly the case that hits this.

Comparing against a past timestamp with the time_is_*_jiffies()
macros is fragile in general, so please express the intent directly.
Either store an explicit expiry in the future, e.g. keep
cache->expires = jiffies + FBNIC_HWMON_CACHE_TTL and test
time_is_after_jiffies(cache->expires), or, if a single tick really is
the intended lifetime, test cache->last_read == jiffies, which is
exact and wrap-safe.

While you are there: is one jiffy the TTL you actually want? A
userspace sweep of the whole attribute set, or a sensors(1) run on a
loaded box, can easily straddle a tick boundary and still issue
several round-trips, which is the flooding the commit message is
trying to avoid. An explicit millisecond-based TTL would make the
policy visible and would also make the wrap-safe deadline form
natural.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831212234.1212971-1-limzhineng2%40gmail.com

Re: [net-next,v2,4/6] eth: fbnic: report temperature and voltage thresholds via hwmon

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 2 · Low: 1

- [Medium] The FBNIC_SENSOR_NO_DATA sentinel for the new thresholds is
  written only by the five stores at the very tail of…
- [Medium] The five new s32 threshold fields in struct fbnic_fw_cap are
  written from the firmware mailbox context (fbnic_fw_parse_cap_resp(),…
- [Low] struct fbnic_threshold
  (drivers/net/ethernet/meta/fbnic/fbnic_fw.h:54-58) declares…

--

eth: fbnic: report temperature and voltage thresholds via hwmon

The firmware capability response is extended to carry temperature
(min/max/crit) and voltage (min/max) thresholds, which are stored in
struct fbnic_fw_cap. The hwmon channel info is extended so the thresholds
are exposed as temp1_{min,max,crit} and in0_{min,max}, with unreported
values returning -ENODATA.

A few questions on the initialization and locking of the new fields.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index ff1674eff7ad5..e5669dfd97904 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c

[ ... ]

> @@ -702,6 +707,22 @@ static int fbnic_fw_parse_cap_resp(void *opaque, struct fbnic_tlv_msg **results)
>  	/* Always assume we need a BMC reinit */
>  	fbd->fw_cap.need_bmc_tcam_reinit = true;
>  
> +	fbd->fw_cap.temp.min =
> +		fbnic_tlv_attr_get_signed(results[FBNIC_FW_CAP_RESP_TEMP_MIN],
> +					  FBNIC_SENSOR_NO_DATA);
> +	fbd->fw_cap.temp.max =
> +		fbnic_tlv_attr_get_signed(results[FBNIC_FW_CAP_RESP_TEMP_MAX],
> +					  FBNIC_SENSOR_NO_DATA);
> +	fbd->fw_cap.temp.crit =
> +		fbnic_tlv_attr_get_signed(results[FBNIC_FW_CAP_RESP_TEMP_CRIT],
> +					  FBNIC_SENSOR_NO_DATA);

[Severity: Medium]
These five stores are the only place FBNIC_SENSOR_NO_DATA ever reaches the
new fields, and they sit at the very end of fbnic_fw_parse_cap_resp().  Can
the thresholds stay at 0 while the hwmon attributes are already exposed?

Two paths appear to leave them zeroed:

1) The BMC block returns early, after fw_cap.running.mgmt.version has
already been accepted:

drivers/net/ethernet/meta/fbnic/fbnic_fw.c:fbnic_fw_parse_cap_resp() {
	bmc_present = !!results[FBNIC_FW_CAP_RESP_BMC_PRESENT];
	if (bmc_present) {
		attr = results[FBNIC_FW_CAP_RESP_BMC_MAC_ARRAY];
		if (!attr)
			return -EINVAL;

		err = fbnic_fw_parse_bmc_addrs(...);
		if (err)
			return err;
	...
}

That failure is only logged by fbnic_mbx_process_rx_msgs():

	err = fbnic_tlv_msg_parse(fbd, msg, fbnic_fw_tlv_parser);
	if (err) {
		dev_warn(fbd->dev, "Unable to process message: %d\n",
			 err);

fbnic_mbx_poll_tx_ready() still returns 0 because it only waits for
mgmt.version >= MIN_FW_VER_CODE, so fbnic_probe() goes on to call
fbnic_hwmon_register().

2) fbnic_mbx_init() does:

	/* Reset FW Capabilities */
	memset(&fbd->fw_cap, 0, sizeof(fbd->fw_cap));

and it is re-run after probe via __fbnic_pm_resume() ->
fbnic_fw_request_mbx() -> __fbnic_fw_enable_mbx(), while the hwmon device
registered at probe is still live (fbnic_hwmon_unregister() only runs from
fbnic_remove()).  If the mailbox does not come ready again, the thresholds
stay 0 for good.

Since fbnic_hwmon_read_threshold() only maps the exact S32_MIN sentinel to
-ENODATA, userspace would then read temp1_min/temp1_max/temp1_crit and
in0_min/in0_max as genuine 0 mC / 0 mV limits, i.e. a board permanently
above its critical temperature and outside its voltage window.

The commit message says:

  "Values the firmware did not report are stored as the
   FBNIC_SENSOR_NO_DATA sentinel in the capability response parser, and a
   read of such an attribute returns -ENODATA."

Should the fields be seeded with FBNIC_SENSOR_NO_DATA where fw_cap is
established (or at hwmon registration) rather than only on the successful
tail of the parser, so 0 is never published as a limit?

> +	fbd->fw_cap.volt.min =
> +		fbnic_tlv_attr_get_signed(results[FBNIC_FW_CAP_RESP_VOLT_MIN],
> +					  FBNIC_SENSOR_NO_DATA);
> +	fbd->fw_cap.volt.max =
> +		fbnic_tlv_attr_get_signed(results[FBNIC_FW_CAP_RESP_VOLT_MAX],
> +					  FBNIC_SENSOR_NO_DATA);
> +
>  	return 0;
>  }
>  

[Severity: Low]
volt.crit is never assigned here, and there is no
FBNIC_FW_CAP_RESP_VOLT_CRIT attribute, so it keeps the 0 left by the zeroed
allocation and by the memset in fbnic_mbx_init() rather than the
FBNIC_SENSOR_NO_DATA convention every other member of the type follows.

Nothing reads it today (fbnic_hwmon_info declares no HWMON_I_CRIT), so this
is dead state for now.  Would seeding all fbnic_threshold members with
FBNIC_SENSOR_NO_DATA avoid publishing 0 mV as a valid critical voltage if
in0_crit is wired up later?

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
> index 42a5f83ddb45e..68ffd49e0cddb 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
> @@ -51,6 +51,12 @@ struct fbnic_fw_ver {
>   */
>  #define FBNIC_SENSOR_NO_DATA			S32_MIN
>  
> +struct fbnic_threshold {
> +	s32 min;
> +	s32 max;
> +	s32 crit;
> +};
> +

[ ... ]

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index f35cb00650937..4938f7b391402 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c

[ ... ]

> @@ -88,14 +78,58 @@ static int fbnic_hwmon_sensor_read(struct fbnic_dev *fbd, int id, long *val)
>  	return err;
>  }
>  
> +static int fbnic_hwmon_read_threshold(long thr, long *val)
> +{
> +	if (thr == FBNIC_SENSOR_NO_DATA)
> +		return -ENODATA;
> +
> +	*val = thr;
> +	return 0;
> +}
> +
> +static int fbnic_hwmon_temp_read(struct fbnic_dev *fbd, u32 attr, long *val)
> +{
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		return fbnic_hwmon_sensor_read(fbd, FBNIC_SENSOR_TEMP, val);
> +	case hwmon_temp_min:
> +		return fbnic_hwmon_read_threshold(fbd->fw_cap.temp.min, val);
> +	case hwmon_temp_max:
> +		return fbnic_hwmon_read_threshold(fbd->fw_cap.temp.max, val);
> +	case hwmon_temp_crit:
> +		return fbnic_hwmon_read_threshold(fbd->fw_cap.temp.crit, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int fbnic_hwmon_in_read(struct fbnic_dev *fbd, u32 attr, long *val)
> +{
> +	switch (attr) {
> +	case hwmon_in_input:
> +		return fbnic_hwmon_sensor_read(fbd, FBNIC_SENSOR_VOLTAGE, val);
> +	case hwmon_in_min:
> +		return fbnic_hwmon_read_threshold(fbd->fw_cap.volt.min, val);
> +	case hwmon_in_max:
> +		return fbnic_hwmon_read_threshold(fbd->fw_cap.volt.max, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +

[Severity: Medium]
What serializes these plain loads of fbd->fw_cap.temp.{min,max,crit} and
fbd->fw_cap.volt.{min,max} against the writer in
fbnic_fw_parse_cap_resp()?

The writer runs from the firmware mailbox context:

fbnic_fw_msix_intr()
  fbnic_mbx_poll()
    ...
      fbnic_fw_parse_cap_resp()   /* five plain stores */

and also from the service task and from fbnic_mbx_poll_tx_ready() during
probe and PM resume.  The reader here runs in process context from a sysfs
read of temp1_crit and friends.  fw_tx_lock only covers the Tx mailbox
ring, and the hwmon core mutex serializes sysfs and thermal accesses
against each other, not against the mailbox context, so there is no common
lock and no READ_ONCE()/WRITE_ONCE() annotation.  Would KCSAN flag these
accesses?

Beyond the plain-access race, the stores republish the fields one at a time
with no publication protocol, so can a reader see a min from one capability
response combined with a max/crit from the previous one?

And because __fbnic_pm_resume() -> fbnic_fw_request_mbx() ->
__fbnic_fw_enable_mbx() re-runs fbnic_mbx_init(), which memsets fw_cap
while the hwmon device stays registered, can a read landing between that
memset and the parser's repopulation return a transient 0 that
fbnic_hwmon_read_threshold() reports as a valid 0 mC / 0 mV limit?

Note that a bare READ_ONCE()/WRITE_ONCE() pair would not close the
transient-zero window; a lock shared with mailbox Rx processing, an
atomically published snapshot, or snapshotting the thresholds once at
hwmon registration would.

> @@ -104,8 +138,11 @@ static const struct hwmon_ops fbnic_hwmon_ops = {
>  };
>  
>  static const struct hwmon_channel_info *fbnic_hwmon_info[] = {
> -	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
> -	HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
> +	HWMON_CHANNEL_INFO(temp,
> +			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX |
> +			   HWMON_T_CRIT),
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX),
>  	NULL
>  };
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831212234.1212971-1-limzhineng2%40gmail.com

Re: [net-next,v2,5/6] eth: fbnic: report temperature and voltage alarms via hwmon

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 2 · Low: 0

- [High] The new temp1_{min,max,crit}_alarm and in0_{min,max}_alarm
  attributes are synthesized in software by comparing a sampled sensor…
- [Medium] fbnic_hwmon_is_visible() returns 0444 unconditionally
  (drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c:10-15) while…
- [Medium] In fbnic_hwmon_temp_read() and fbnic_hwmon_in_read() the alarm
  attribute cases `break` out of the first switch and unconditionally…

--

eth: fbnic: report temperature and voltage alarms via hwmon

This adds temp1_{min,max,crit}_alarm and in0_{min,max}_alarm to the fbnic
hwmon interface.  Each alarm is derived by taking a sensor reading and
comparing it against the matching threshold cached in fbnic_fw_cap, and
thresholds the firmware never reported return -ENODATA.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index 4938f7b391402..c5cddd9cef124 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> @@ -87,34 +87,84 @@ static int fbnic_hwmon_read_threshold(long thr, long *val)
>  	return 0;
>  }
>  
> +static int fbnic_hwmon_read_alarm(long a, long b, long *val)
> +{
> +	if (a == FBNIC_SENSOR_NO_DATA || b == FBNIC_SENSOR_NO_DATA)
> +		return -ENODATA;
> +
> +	*val = a >= b;
> +	return 0;
> +}

[Severity: High]
Is a driver-side comparison allowed for the *_alarm files?
Documentation/hwmon/sysfs-interface.rst says:

    Alarms are direct indications read from the chips. The drivers do NOT
    make comparisons of readings to thresholds. This allows violations
    between readings to be caught and alarmed.

Here fbnic_hwmon_read_alarm() computes the alarm in software from a single
sample, so a threshold violation that starts and clears between two
userspace reads is never visible, while the file name promises a latched
chip indication.

The effect is amplified by the cache in fbnic_hwmon_sensor_read():

	if (*cached != FBNIC_SENSOR_NO_DATA &&
	    time_is_after_eq_jiffies(cache->last_read)) {
		*val = *cached;
		return 0;
	}

so an alarm read can be answered from a sample taken in a previous jiffy.

The later commit in this series, "eth: fbnic: firmware notifies hwmon on
sensor threshold events", only adds hwmon_notify_event() wakeups and leaves
the sysfs read path computing the alarm in software.  A reader woken by such
a notification can then take a fresh TSENE reading and see the alarm as 0
because the transient already passed.  That commit also shows the firmware
does have a threshold-crossing indication - could the alarm attributes be
backed by that indication instead, or dropped so userspace compares
temp1_input against the already exported temp1_{min,max,crit}?

>  static int fbnic_hwmon_temp_read(struct fbnic_dev *fbd, u32 attr, long *val)
>  {
> +	int err;
> +
>  	switch (attr) {
> -	case hwmon_temp_input:
> -		return fbnic_hwmon_sensor_read(fbd, FBNIC_SENSOR_TEMP, val);
>  	case hwmon_temp_min:
>  		return fbnic_hwmon_read_threshold(fbd->fw_cap.temp.min, val);
>  	case hwmon_temp_max:
>  		return fbnic_hwmon_read_threshold(fbd->fw_cap.temp.max, val);
>  	case hwmon_temp_crit:
>  		return fbnic_hwmon_read_threshold(fbd->fw_cap.temp.crit, val);
> +	case hwmon_temp_input:
> +	case hwmon_temp_min_alarm:
> +	case hwmon_temp_max_alarm:
> +	case hwmon_temp_crit_alarm:
> +		break;
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> +
> +	err = fbnic_hwmon_sensor_read(fbd, FBNIC_SENSOR_TEMP, val);
> +	if (err)
> +		return err;

[Severity: Medium]
Should the FBNIC_SENSOR_NO_DATA test happen before this firmware round-trip?

For the plain min/max/crit attributes the sentinel is checked up front, but
for the alarms the check only happens later, inside fbnic_hwmon_read_alarm().
On firmware that did not report a threshold, every read of temp1_min_alarm
(and in0_min_alarm / in0_max_alarm via fbnic_hwmon_in_read()) allocates a
completion, transmits a TSENE message and can block here:

	if (!wait_for_completion_timeout(&fw_cmpl->done, 10 * HZ)) {
		dev_err(fbd->dev, "Timed out waiting for TSENE read\n");

with the hwmon core's hwdev->lock held, only to return -ENODATA that was
already decidable without any firmware traffic.

There is a second consequence of the ordering: when the sensor read fails,
the attribute returns the transport error (-ETIMEDOUT, -ENOMEM, -EPIPE) plus
a dev_err() line rather than the -ENODATA the missing threshold dictates, so
"unsupported" is reported as a transient failure.

> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		return 0;
> +	case hwmon_temp_min_alarm:
> +		return fbnic_hwmon_read_alarm(fbd->fw_cap.temp.min, *val, val);
> +	case hwmon_temp_max_alarm:
> +		return fbnic_hwmon_read_alarm(*val, fbd->fw_cap.temp.max, val);
> +	case hwmon_temp_crit_alarm:
> +		return fbnic_hwmon_read_alarm(*val, fbd->fw_cap.temp.crit, val);
> +	}
> +
> +	return -EOPNOTSUPP;
>  }

[ ... in0 hunk snipped, same pattern in fbnic_hwmon_in_read() ... ]

> @@ -139,10 +189,14 @@ static const struct hwmon_ops fbnic_hwmon_ops = {
>  
>  static const struct hwmon_channel_info *fbnic_hwmon_info[] = {
>  	HWMON_CHANNEL_INFO(temp,
> -			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX |
> -			   HWMON_T_CRIT),
> +			   HWMON_T_INPUT |
> +			   HWMON_T_MIN | HWMON_T_MIN_ALARM |
> +			   HWMON_T_MAX | HWMON_T_MAX_ALARM |
> +			   HWMON_T_CRIT | HWMON_T_CRIT_ALARM),
>  	HWMON_CHANNEL_INFO(in,
> -			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX),
> +			   HWMON_I_INPUT |
> +			   HWMON_I_MIN | HWMON_I_MIN_ALARM |
> +			   HWMON_I_MAX | HWMON_I_MAX_ALARM),
>  	NULL
>  };

[Severity: Medium]
Could these alarms be hidden instead of created and then always failing?

fbnic_hwmon_is_visible() returns 0444 unconditionally:

	static umode_t fbnic_hwmon_is_visible(const void *drvdata,
					      enum hwmon_sensor_types type,
					      u32 attr, int channel)
	{
		return 0444;
	}

so with these new config bits the five alarm files always exist.  If the
running firmware omits the threshold TLVs, fbnic_fw_parse_cap_resp() stores
FBNIC_SENSOR_NO_DATA and fbnic_hwmon_read_alarm() then returns -ENODATA for
that alarm for the whole lifetime of the device, which libsensors reports as
a chip read error rather than an unsupported feature.

The threshold availability is known when is_visible() runs: fbnic_probe()
calls fbnic_fw_request_mbx(), which polls in fbnic_mbx_poll_tx_ready() until
the FW_CAP_RESP has been parsed, and only afterwards calls
fbnic_hwmon_register().  Would returning 0 from fbnic_hwmon_is_visible() for
alarms whose threshold is FBNIC_SENSOR_NO_DATA (drvdata is the fbd here) be
preferable?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831212234.1212971-1-limzhineng2%40gmail.com

Re: [net-next,v2,6/6] eth: fbnic: firmware notifies hwmon on sensor threshold events

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 4

- [Medium] fbnic_hwmon_notify_event() uses
  fbd->fw_cap.temp.{min,max,crit} and fbd->fw_cap.volt.{min,max} in…
- [Low] The new unsolicited-message handler logs at KERN_ERR
  unconditionally for every firmware threshold event (dev_err…
- [Low] fbnic_hwmon_unregister() calls
  synchronize_irq(fbd->fw_msix_vector) unconditionally, but…
- [Low] temp1_max_alarm and temp1_crit_alarm are independent attributes:
  fbnic_hwmon_temp_read() computes max_alarm as (val >= temp.max) and…
- [Low] fbd->hwmon is published with a plain WRITE_ONCE() after the hwmon
  device has been fully initialized and the sensor cache seeded, and it…

--

eth: fbnic: firmware notifies hwmon on sensor threshold events

This adds a parser for the new unsolicited
FBNIC_TLV_MSG_ID_SENSOR_THRESHOLD_EXCEEDED_RESP firmware message and
translates it into a hwmon alarm notification via hwmon_notify_event().
It also reworks the publish/teardown of fbd->hwmon with WRITE_ONCE(),
READ_ONCE() and synchronize_irq().

A few questions below.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index e5669dfd97904..fd96a3962e3f5 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> @@ -1639,6 +1639,57 @@ fbnic_fw_parser_test(void *opaque, struct fbnic_tlv_msg **results)
>  	return err;
>  }
>  
> +static const struct fbnic_tlv_index fbnic_threshold_exceeded_resp_index[] = {
> +	FBNIC_TLV_ATTR_S32(FBNIC_FW_TSENE_THERM_EXCEEDED_FLAG),
> +	FBNIC_TLV_ATTR_S32(FBNIC_FW_TSENE_VOLT_EXCEEDED_FLAG),
> +	FBNIC_TLV_ATTR_S32(FBNIC_FW_TSENE_THERMAL),
> +	FBNIC_TLV_ATTR_S32(FBNIC_FW_TSENE_VOLTAGE),
> +	FBNIC_TLV_ATTR_LAST
> +};
> +
> +static int fbnic_fw_parse_threshold_exceeded_resp(void *opaque,
> +						  struct fbnic_tlv_msg **results)
> +{
> +	bool therm_exceeded, volt_exceeded;
> +	struct fbnic_dev *fbd = opaque;
> +	s32 value;
> +
> +	therm_exceeded =
> +		fta_get_sint(results, FBNIC_FW_TSENE_THERM_EXCEEDED_FLAG);
> +	volt_exceeded =
> +		fta_get_sint(results, FBNIC_FW_TSENE_VOLT_EXCEEDED_FLAG);
> +
> +	if (!therm_exceeded && !volt_exceeded) {
> +		dev_err(fbd->dev,
> +			"Threshold exceeded message with no flag set\n");
> +		return -EINVAL;
> +	}
> +
> +	if (therm_exceeded) {
> +		if (!results[FBNIC_FW_TSENE_THERMAL]) {
> +			dev_err(fbd->dev,
> +				"Thermal threshold exceeded but no value received\n");
> +			return -EINVAL;
> +		}
> +		value = fta_get_sint(results, FBNIC_FW_TSENE_THERMAL);
> +		dev_err(fbd->dev, "Thermal threshold exceeded: %d mC\n", value);
> +		fbnic_hwmon_notify_event(fbd, FBNIC_SENSOR_TEMP, value);
> +	}
> +
> +	if (volt_exceeded) {
> +		if (!results[FBNIC_FW_TSENE_VOLTAGE]) {
> +			dev_err(fbd->dev,
> +				"Voltage threshold exceeded but no value received\n");
> +			return -EINVAL;
> +		}
> +		value = fta_get_sint(results, FBNIC_FW_TSENE_VOLTAGE);
> +		dev_err(fbd->dev, "Voltage threshold exceeded: %d mV\n", value);
> +		fbnic_hwmon_notify_event(fbd, FBNIC_SENSOR_VOLTAGE, value);
> +	}
> +
> +	return 0;
> +}

[Severity: Low]
Should the logging in fbnic_fw_parse_threshold_exceeded_resp() be rate
limited?

Message ID 0x40 is registered in fbnic_fw_tlv_parser[] and dispatched
straight from fbnic_mbx_process_rx_msgs() with no completion slot and no
host request gating it, so the arrival rate is entirely controlled by the
firmware. Each accepted event emits an unconditional dev_err() and then a
KOBJ_CHANGE uevent through hwmon_notify_event() -> kobject_uevent_env().

The three new -EINVAL returns feed the existing amplifier in
fbnic_mbx_process_rx_msgs():

	if (err) {
		dev_warn(fbd->dev, "Unable to process message: %d\n",
			 err);
		print_hex_dump(KERN_WARNING, "fbnic:",
			       DUMP_PREFIX_OFFSET, 16, 2,
			       msg, length, true);

so a firmware sending a malformed threshold message in a loop produces a
full hex dump per message. Would dev_err_ratelimited() (or _once) be
preferable here?

Also worth noting: in the volt_exceeded path, if the thermal notification
already happened and the voltage value attribute is missing, the function
both notifies and returns -EINVAL, so the same message triggers the hex
dump too.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index c5cddd9cef124..3f33522fcfd2b 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> @@ -214,22 +216,75 @@ void fbnic_hwmon_register(struct fbnic_dev *fbd)
>  	fbd->hwmon_cache.temp_mdeg = FBNIC_SENSOR_NO_DATA;
>  	fbd->hwmon_cache.volt_mv = FBNIC_SENSOR_NO_DATA;
>  
> -	fbd->hwmon = hwmon_device_register_with_info(fbd->dev, "fbnic",
> -						     fbd, &fbnic_chip_info,
> -						     NULL);
> -	if (IS_ERR(fbd->hwmon)) {
> +	hwmon = hwmon_device_register_with_info(fbd->dev, "fbnic", fbd,
> +						&fbnic_chip_info, NULL);
> +	if (IS_ERR(hwmon)) {
>  		dev_notice(fbd->dev,
>  			   "Failed to register hwmon device %pe\n",
> -			   fbd->hwmon);
> -		fbd->hwmon = NULL;
> +			   hwmon);
> +		return;
>  	}
> +
> +	WRITE_ONCE(fbd->hwmon, hwmon);

[Severity: Low]
Is a plain WRITE_ONCE() enough on this publish side?

fbnic_hwmon_register() seeds fbd->hwmon_cache and registers the device,
then publishes the pointer that another CPU dereferences in
fbnic_hwmon_notify_event() -> hwmon_notify_event() -> sysfs_notify(&dev->kobj,
...). WRITE_ONCE() carries no release semantics and READ_ONCE() only gives
address-dependency ordering for loads through the pointer, so the preceding
initialization stores are not ordered against the pointer store on a weakly
ordered architecture.

The in-code comments in fbnic_hwmon_unregister() and
fbnic_hwmon_notify_event() describe the WRITE_ONCE/READ_ONCE pair as
providing that guarantee. Should this be smp_store_release() /
smp_load_acquire() instead, or should the comments be adjusted?

>  }
>  
>  void fbnic_hwmon_unregister(struct fbnic_dev *fbd)
>  {
> +	struct device *hwmon;
> +
>  	if (!IS_REACHABLE(CONFIG_HWMON) || !fbd->hwmon)
>  		return;
>  
> -	hwmon_device_unregister(fbd->hwmon);
> -	fbd->hwmon = NULL;
> +	hwmon = fbd->hwmon;
> +	/* Pair with READ_ONCE() in fbnic_hwmon_notify_event(). Publish NULL
> +	 * and wait for any in-flight FW mailbox IRQ handler to finish so it
> +	 * cannot dereference the hwmon device after we unregister it.
> +	 */
> +	WRITE_ONCE(fbd->hwmon, NULL);
> +	synchronize_irq(fbd->fw_msix_vector);

[Severity: Low]
Can fbd->fw_msix_vector be 0 or stale at this point?

fbnic_fw_free_mbx() treats 0 as the "no IRQ" sentinel and guards on it:

	/* Vector has already been freed */
	if (!fbd->fw_msix_vector)
		return;
	...
	free_irq(fbd->fw_msix_vector, fbd);
	fbd->fw_msix_vector = 0;

That runs on the PM suspend and error-recovery paths without touching
fbd->hwmon, and fbnic_hwmon_unregister() has a single call site in
fbnic_remove(). So an unbind after a failed resume reaches
synchronize_irq() with fw_msix_vector == 0.

fbnic_fw_request_mbx() also leaves a stale value behind:

	err = __fbnic_fw_enable_mbx(fbd, vector);
	if (err)
		free_irq(vector, fbd);

	fbd->fw_msix_vector = vector;

synchronize_irq(0) does not return early when irq_to_desc(0) yields a
descriptor, so the code would wait on an unrelated line rather than the
fbnic FW mailbox IRQ, and the drain the commit message describes is not
performed. Would guarding on fbd->fw_msix_vector != 0 (as
fbnic_fw_free_mbx() does), or ordering the hwmon teardown against the
mailbox teardown, be more robust here?

> +
> +	hwmon_device_unregister(hwmon);
> +}
> +
> +void fbnic_hwmon_notify_event(struct fbnic_dev *fbd, int id, long val)
> +{
> +	enum hwmon_sensor_types type;
> +	struct device *hwmon;
> +	s32 attr = -1;
> +
> +	if (!IS_REACHABLE(CONFIG_HWMON))
> +		return;
> +
> +	switch (id) {
> +	case FBNIC_SENSOR_TEMP:
> +		type = hwmon_temp;
> +
> +		if (val <= fbd->fw_cap.temp.min)
> +			attr = hwmon_temp_min_alarm;
> +		else if (val >= fbd->fw_cap.temp.crit)
> +			attr = hwmon_temp_crit_alarm;
> +		else if (val >= fbd->fw_cap.temp.max)
> +			attr = hwmon_temp_max_alarm;
> +
> +		break;
> +	case FBNIC_SENSOR_VOLTAGE:
> +		type = hwmon_in;
> +
> +		if (val <= fbd->fw_cap.volt.min)
> +			attr = hwmon_in_min_alarm;
> +		else if (val >= fbd->fw_cap.volt.max)
> +			attr = hwmon_in_max_alarm;
> +
> +		break;

[Severity: Medium]
Do these comparisons need to filter FBNIC_SENSOR_NO_DATA first?

fbnic_fw_parse_cap_resp() stores the sentinel when the firmware cap
response omits an optional threshold:

	fbd->fw_cap.temp.min =
		fbnic_tlv_attr_get_signed(results[FBNIC_FW_CAP_RESP_TEMP_MIN],
					  FBNIC_SENSOR_NO_DATA);

and FBNIC_SENSOR_NO_DATA is S32_MIN. Since every real reading is greater
than S32_MIN, with temp.crit unpopulated:

	val = 45000;
	45000 <= temp.min (S32_MIN)   -> false
	45000 >= temp.crit (S32_MIN)  -> true  -> hwmon_temp_crit_alarm

so any thermal event is reported as temp1_crit_alarm regardless of the
actual condition, and the same applies to in0_max_alarm when volt.max is
unpopulated. hwmon_notify_event() for hwmon_temp additionally calls
hwmon_thermal_notify(). A listener woken on temp1_crit_alarm then reads the
attribute and gets -ENODATA back from the peer helper in this same file:

	static int fbnic_hwmon_read_alarm(long a, long b, long *val)
	{
		if (a == FBNIC_SENSOR_NO_DATA || b == FBNIC_SENSOR_NO_DATA)
			return -ENODATA;

The mirror case also loses events: with temp.min == S32_MIN an
under-minimum reading matches no branch at all and nothing is notified.

[Severity: Low]
Should the max and crit alarms be notified independently rather than
selected by an if/else-if chain?

fbnic_hwmon_temp_read() derives them from separate comparisons:

	case hwmon_temp_max_alarm:
		return fbnic_hwmon_read_alarm(*val, fbd->fw_cap.temp.max, val);
	case hwmon_temp_crit_alarm:
		return fbnic_hwmon_read_alarm(*val, fbd->fw_cap.temp.crit, val);

so with temp.max <= temp.crit a reading at or above crit makes both
temp1_max_alarm and temp1_crit_alarm read as 1. Both files are created
(fbnic_hwmon_info[] advertises HWMON_T_MIN_ALARM, HWMON_T_MAX_ALARM and
HWMON_T_CRIT_ALARM, and fbnic_hwmon_is_visible() returns 0444), but the
else-if chain only notifies hwmon_temp_crit_alarm, so a poller on
temp1_max_alarm is never woken even though that attribute just changed from
0 to 1.

The firmware event format in fbnic_threshold_exceeded_resp_index[] carries
only a per-sensor exceeded flag plus a value, with no indication of which
threshold was crossed, so is there anything that justifies picking exactly
one alarm?

> +	default:
> +		return;
> +	}
> +
> +	/* Pair with WRITE_ONCE() in fbnic_hwmon_unregister(). Skip the
> +	 * notification if hwmon failed to register or has already been torn
> +	 * down.
> +	 */
> +	hwmon = READ_ONCE(fbd->hwmon);
> +	if (attr >= 0 && hwmon)
> +		hwmon_notify_event(hwmon, type, attr, 0);
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831212234.1212971-1-limzhineng2%40gmail.com