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

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

[Zinc Lim]

The firmware capability response carries per-board sensor thresholds,
and the firmware reports which range a sensor has entered whenever it
crosses one of them. This series surfaces both to userspace:

 - 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.
 - 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 does not report are
   not exposed.
 - Parse the range firmware reports, both in the unsolicited threshold
   event and in the sensor read response, and cache it per sensor.
 - Expose temp1_alarm and in0_alarm from that cached range, and notify
   on each crossing through hwmon_notify_event().
 - Derive the range from the reading carried with the event, on old
   firmware that predates range reporting.

Changes from v3:
 - Moved cache seeding to fbnic_mbx_init() in patch 2, so it also runs
   on resume and AER recovery paths rather than only on probe.
 - Dropped previous patch exposing all hwmon attributes unconditionally as
   read-only. Thresholds the firmware does not report are now not visible
   in sysfs.
 - Collapsed the five per-threshold alarm attributes
   (temp1_{min,max,crit}_alarm, in0_{min,max}_alarm) into one alarm per
   sensor, temp1_alarm and in0_alarm. Firmware reports which range a
   sensor is in rather than per-limit flags, so a single attribute
   matches what the hardware reports.
 - That alarm now comes from the reported range rather than from a
   driver-side comparison of a live reading against the thresholds.
 - Split the alarm work into two: patch 4 parses and caches the range,
   from both the unsolicited event and the sensor read response, and
   patch 5 exposes the attributes and notifies on each crossing. The
   previous "firmware notifies hwmon on sensor threshold events" patch 6
   is now folded into those.
 - Added patch 6 for firmware that predates range reporting, which
   flags only that a limit was exceeded.

Changes from v2:
 - Addressed Jakub's patch 3 feedback on jiffy comparison when
   checking cache validity.

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: cache hwmon sensor readings
  eth: fbnic: report temperature and voltage thresholds via hwmon
  eth: fbnic: parse the alarm range reported by firmware
  eth: fbnic: report temperature and voltage alarms via hwmon
  eth: fbnic: derive the alarm range on older firmware

 drivers/net/ethernet/meta/fbnic/fbnic.h       |  11 +
 drivers/net/ethernet/meta/fbnic/fbnic_fw.c    | 140 +++++++-
 drivers/net/ethernet/meta/fbnic/fbnic_fw.h    |  44 +++
 drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c | 332 ++++++++++++++++--
 drivers/net/ethernet/meta/fbnic/fbnic_mac.c   |  55 ---
 drivers/net/ethernet/meta/fbnic/fbnic_mac.h   |   2 -
 6 files changed, 500 insertions(+), 84 deletions(-)

--
2.53.0-Meta

[PATCH net-next v4 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 16271c717a5a..3e1256ffd141 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_mac.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_mac.c
@@ -892,60 +892,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,
@@ -959,7 +905,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 v4 2/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 in fbnic_mbx_init() so the first read always refreshes.

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

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic.h b/drivers/net/ethernet/meta/fbnic/fbnic.h
index 1f9fbd71447f..857a64b518f3 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.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
index 49b8813a3564..82307f212f58 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
@@ -105,6 +105,13 @@ void fbnic_mbx_init(struct fbnic_dev *fbd)
 	/* Reset FW Capabilities */
 	memset(&fbd->fw_cap, 0, sizeof(fbd->fw_cap));
 
+	/* Seed cache with sentinel so the first read always refreshes. The
+	 * cache mirrors what firmware reported, so drop it when the mailbox
+	 * restarts.
+	 */
+	fbd->hwmon_cache.temp_mdeg = FBNIC_SENSOR_NO_DATA;
+	fbd->hwmon_cache.volt_mv = FBNIC_SENSOR_NO_DATA;
+
 	/* Reinitialize mailbox memory */
 	for (i = 0; i < FBNIC_IPC_MBX_INDICES; i++)
 		memset(&fbd->mbx[i], 0, sizeof(struct fbnic_fw_mbx));
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
index f47669129c9d..ddedb7c0e0a7 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;
@@ -59,6 +60,12 @@ enum {
 	(BIT(FBNIC_FW_CAP_F_BMC_TCAM_REINIT) |	\
 	 BIT(FBNIC_FW_CAP_F_BMC_MACDA_SYNC))
 
+/* 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 {
 	unsigned long state;
 	struct {
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
index 6c8c66ab86c1..0c3fe2a37227 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"
@@ -30,26 +31,31 @@ 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 && cache->last_read == jiffies) {
+		*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,
@@ -72,7 +78,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:
-- 
2.53.0-Meta

[PATCH net-next v4 3/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}.

Zero is a valid threshold, so the fields are reset to the
FBNIC_SENSOR_NO_DATA sentinel in fbnic_mbx_init() as
fbnic_fw_parse_cap_resp() can return before it reaches the threshold
assignments.

Older firmware that do not report thresholds result in
fbnic_hwmon_is_visible() not exposing the thresholds in the sysfs,
only temp1_input and in0_input.

The mailbox is reinitialized on resume and AER recovery while the hwmon
device stays registered, so reads also reject the sentinel with -ENODATA
rather than reporting it as a value.

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

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
index 82307f212f58..d9a00a2ef34d 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
@@ -95,15 +95,25 @@ static void fbnic_mbx_reset_desc_ring(struct fbnic_dev *fbd, int mbx_idx)
 					    FBNIC_IPC_MBX_DESC_HOST_CMPL);
 }
 
+static const struct fbnic_threshold fbnic_threshold_no_data = {
+	.min	= FBNIC_SENSOR_NO_DATA,
+	.max	= FBNIC_SENSOR_NO_DATA,
+	.crit	= FBNIC_SENSOR_NO_DATA,
+};
+
 void fbnic_mbx_init(struct fbnic_dev *fbd)
 {
+	struct fbnic_fw_cap fw_cap_reset;
 	int i;
 
 	/* Initialize lock to protect Tx ring */
 	spin_lock_init(&fbd->fw_tx_lock);
 
-	/* Reset FW Capabilities */
-	memset(&fbd->fw_cap, 0, sizeof(fbd->fw_cap));
+	/* Reset FW Capabilities and seed thresholds with sentinel */
+	memset(&fw_cap_reset, 0, sizeof(fw_cap_reset));
+	fw_cap_reset.temp = fbnic_threshold_no_data;
+	fw_cap_reset.volt = fbnic_threshold_no_data;
+	fbd->fw_cap = fw_cap_reset;
 
 	/* Seed cache with sentinel so the first read always refreshes. The
 	 * cache mirrors what firmware reported, so drop it when the mailbox
@@ -595,6 +605,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
 };
 
@@ -742,6 +757,22 @@ static int fbnic_fw_parse_cap_resp(void *opaque, struct fbnic_tlv_msg **results)
 	fbd->fw_cap.anti_rollback_version =
 		fta_get_uint(results, FBNIC_FW_CAP_RESP_ANTI_ROLLBACK_VERSION);
 
+	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 ddedb7c0e0a7..dde2929f8660 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
@@ -66,6 +66,12 @@ enum {
  */
 #define FBNIC_SENSOR_NO_DATA			S32_MIN
 
+struct fbnic_threshold {
+	s32 min;
+	s32 max;
+	s32 crit;
+};
+
 struct fbnic_fw_cap {
 	unsigned long state;
 	struct {
@@ -79,6 +85,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 {
@@ -261,6 +269,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 0c3fe2a37227..4df808f850cf 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
@@ -7,24 +7,43 @@
 #include "fbnic.h"
 #include "fbnic_mac.h"
 
-static int fbnic_hwmon_sensor_id(enum hwmon_sensor_types type)
+static umode_t fbnic_hwmon_thr_visible(s32 thr)
 {
-	if (type == hwmon_temp)
-		return FBNIC_SENSOR_TEMP;
-	if (type == hwmon_in)
-		return FBNIC_SENSOR_VOLTAGE;
-
-	return -EOPNOTSUPP;
+	return thr == FBNIC_SENSOR_NO_DATA ? 0 : 0444;
 }
 
 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;
+	const struct fbnic_dev *fbd = drvdata;
+
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+			return 0444;
+		case hwmon_temp_min:
+			return fbnic_hwmon_thr_visible(fbd->fw_cap.temp.min);
+		case hwmon_temp_max:
+			return fbnic_hwmon_thr_visible(fbd->fw_cap.temp.max);
+		case hwmon_temp_crit:
+			return fbnic_hwmon_thr_visible(fbd->fw_cap.temp.crit);
+		}
+		break;
+	case hwmon_in:
+		switch (attr) {
+		case hwmon_in_input:
+			return 0444;
+		case hwmon_in_min:
+			return fbnic_hwmon_thr_visible(fbd->fw_cap.volt.min);
+		case hwmon_in_max:
+			return fbnic_hwmon_thr_visible(fbd->fw_cap.volt.max);
+		}
+		break;
+	default:
+		break;
+	}
 
 	return 0;
 }
@@ -92,14 +111,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 = {
@@ -108,8 +171,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 v4 4/6] eth: fbnic: parse the alarm range reported by firmware

[Zinc Lim]

Firmware reports which range a sensor is in: normal, below min, above
max, above crit. It sends the range in an unsolicited event on every
trip point crossing, including the crossing back into the normal range,
and in the response to a sensor read. Parse both and keep the last
reported range per sensor, so the driver holds a condition the hardware
reported.

The range is cached from the mailbox thread in both cases, so an event
and a read response cannot be applied out of the order firmware sent
them. It is seeded alongside the rest of the sensor cache in
fbnic_mbx_init(), before the mailbox interrupt is enabled, since an
event can arrive as soon as it is.

Firmware predating range reporting sends ALARM_DEPRECATED for every
threshold event and omits the range from the read response entirely.
Neither is cached, as neither says which limit was crossed.

Subsequent patches in this series will consume the cached range.

Signed-off-by: Zinc Lim <limzhineng2@gmail.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic.h    |  2 +
 drivers/net/ethernet/meta/fbnic/fbnic_fw.c | 84 ++++++++++++++++++++++
 drivers/net/ethernet/meta/fbnic/fbnic_fw.h | 23 ++++++
 3 files changed, 109 insertions(+)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic.h b/drivers/net/ethernet/meta/fbnic/fbnic.h
index 857a64b518f3..93bf7004b67d 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic.h
@@ -26,6 +26,8 @@ struct fbnic_hwmon_cache {
 	unsigned long last_read;
 	s32 temp_mdeg;
 	s32 volt_mv;
+	u8 temp_alarm;
+	u8 volt_alarm;
 };
 
 struct fbnic_dev {
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
index d9a00a2ef34d..7863f6ba1e7d 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
@@ -121,6 +121,8 @@ void fbnic_mbx_init(struct fbnic_dev *fbd)
 	 */
 	fbd->hwmon_cache.temp_mdeg = FBNIC_SENSOR_NO_DATA;
 	fbd->hwmon_cache.volt_mv = FBNIC_SENSOR_NO_DATA;
+	fbd->hwmon_cache.temp_alarm = FBNIC_FW_TSENE_ALARM_UNKNOWN;
+	fbd->hwmon_cache.volt_alarm = FBNIC_FW_TSENE_ALARM_UNKNOWN;
 
 	/* Reinitialize mailbox memory */
 	for (i = 0; i < FBNIC_IPC_MBX_INDICES; i++)
@@ -1466,6 +1468,8 @@ static const struct fbnic_tlv_index fbnic_tsene_read_resp_index[] = {
 	FBNIC_TLV_ATTR_S32(FBNIC_FW_TSENE_THERM),
 	FBNIC_TLV_ATTR_S32(FBNIC_FW_TSENE_VOLT),
 	FBNIC_TLV_ATTR_S32(FBNIC_FW_TSENE_ERROR),
+	FBNIC_TLV_ATTR_U32(FBNIC_FW_TSENE_THERM_ALARM_STATE),
+	FBNIC_TLV_ATTR_U32(FBNIC_FW_TSENE_VOLT_ALARM_STATE),
 	FBNIC_TLV_ATTR_LAST
 };
 
@@ -1476,6 +1480,7 @@ static int fbnic_fw_parse_tsene_read_resp(void *opaque,
 	struct fbnic_dev *fbd = opaque;
 	s32 err_resp;
 	int err = 0;
+	u32 alarm;
 
 	/* Verify we have a completion pointer to provide with data */
 	cmpl_data = fbnic_fw_get_cmpl_by_type(fbd,
@@ -1497,6 +1502,20 @@ static int fbnic_fw_parse_tsene_read_resp(void *opaque,
 	cmpl_data->u.tsene.millivolts =
 		fta_get_sint(results, FBNIC_FW_TSENE_VOLT);
 
+	/* Cache here so that it and the threshold event land in the order firmware
+	 * sent them. Firmware predating alarm state reporting sends neither
+	 * attribute.
+	 */
+	alarm = fbnic_tlv_attr_get_unsigned(results[FBNIC_FW_TSENE_THERM_ALARM_STATE],
+					    FBNIC_FW_TSENE_ALARM_UNKNOWN);
+	if (alarm != FBNIC_FW_TSENE_ALARM_UNKNOWN)
+		WRITE_ONCE(fbd->hwmon_cache.temp_alarm, alarm);
+
+	alarm = fbnic_tlv_attr_get_unsigned(results[FBNIC_FW_TSENE_VOLT_ALARM_STATE],
+					    FBNIC_FW_TSENE_ALARM_UNKNOWN);
+	if (alarm != FBNIC_FW_TSENE_ALARM_UNKNOWN)
+		WRITE_ONCE(fbd->hwmon_cache.volt_alarm, alarm);
+
 msg_err:
 	cmpl_data->result = err_resp ? : err;
 	complete(&cmpl_data->done);
@@ -1689,6 +1708,68 @@ 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_U32(FBNIC_FW_TSENE_THERM_ALARM),
+	FBNIC_TLV_ATTR_U32(FBNIC_FW_TSENE_VOLT_ALARM),
+	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)
+{
+	struct fbnic_dev *fbd = opaque;
+	bool rec_flag = false;
+	u32 alarm;
+	s32 value;
+
+	if (results[FBNIC_FW_TSENE_THERM_ALARM]) {
+		bool have_temp = !!results[FBNIC_FW_TSENE_THERMAL];
+
+		alarm = fta_get_uint(results, FBNIC_FW_TSENE_THERM_ALARM);
+		value = fta_get_sint(results, FBNIC_FW_TSENE_THERMAL);
+		if (!have_temp)
+			dev_err(fbd->dev, "Thermal alarm %u received with no temperature!\n",
+				alarm);
+		else if (alarm != FBNIC_FW_TSENE_ALARM_NORMAL)
+			dev_err(fbd->dev, "Thermal alarm %u, temperature is currently %dmC!\n",
+				alarm, value);
+
+		/* Old firmware reports no range, leave the cache alone */
+		if (alarm != FBNIC_FW_TSENE_ALARM_DEPRECATED)
+			WRITE_ONCE(fbd->hwmon_cache.temp_alarm, alarm);
+		rec_flag = true;
+	}
+
+	if (results[FBNIC_FW_TSENE_VOLT_ALARM]) {
+		bool have_volt = !!results[FBNIC_FW_TSENE_VOLTAGE];
+
+		alarm = fta_get_uint(results, FBNIC_FW_TSENE_VOLT_ALARM);
+		value = fta_get_sint(results, FBNIC_FW_TSENE_VOLTAGE);
+		if (!have_volt)
+			dev_err(fbd->dev, "Voltage alarm %u received with no voltage!\n",
+				alarm);
+		else if (alarm != FBNIC_FW_TSENE_ALARM_NORMAL)
+			dev_err(fbd->dev, "Voltage alarm %u, voltage is currently %dmV!\n",
+				alarm, value);
+
+		/* Old firmware reports no range, leave the cache alone */
+		if (alarm != FBNIC_FW_TSENE_ALARM_DEPRECATED)
+			WRITE_ONCE(fbd->hwmon_cache.volt_alarm, alarm);
+		rec_flag = true;
+	}
+
+	if (!rec_flag) {
+		dev_warn(fbd->dev,
+			 "Threshold exceeded message received but no alarm found!\n");
+		return -EINVAL;
+	}
+
+	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,
@@ -1717,6 +1798,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 dde2929f8660..3b4787277408 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
@@ -239,6 +239,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,
@@ -305,9 +306,31 @@ enum {
 	FBNIC_FW_TSENE_THERM			= 0x0,
 	FBNIC_FW_TSENE_VOLT			= 0x1,
 	FBNIC_FW_TSENE_ERROR			= 0x2,
+	FBNIC_FW_TSENE_THERM_ALARM_STATE	= 0x3,
+	FBNIC_FW_TSENE_VOLT_ALARM_STATE		= 0x4,
 	FBNIC_FW_TSENE_MSG_MAX
 };
 
+enum {
+	FBNIC_FW_TSENE_THERM_ALARM		= 0x0,
+	FBNIC_FW_TSENE_VOLT_ALARM		= 0x1,
+	FBNIC_FW_TSENE_THERMAL			= 0x2,
+	FBNIC_FW_TSENE_VOLTAGE			= 0x3,
+	FBNIC_FW_TSENE_EXCEEDED_MSG_MAX,
+};
+
+enum {
+	FBNIC_FW_TSENE_ALARM_NORMAL		= 0x0,
+	FBNIC_FW_TSENE_ALARM_DEPRECATED		= 0x1,
+	FBNIC_FW_TSENE_ALARM_BELOW_MIN		= 0x2,
+	FBNIC_FW_TSENE_ALARM_ABOVE_MAX		= 0x3,
+	FBNIC_FW_TSENE_ALARM_CRIT		= 0x4,
+	/* Driver internal, never sent by firmware. 0xff so its out of
+	 * range wherever the range is held in a u8.
+	 */
+	FBNIC_FW_TSENE_ALARM_UNKNOWN		= 0xff,
+};
+
 enum {
 	FBNIC_FW_OWNERSHIP_FLAG			= 0x0,
 	FBNIC_FW_OWNERSHIP_TIME			= 0x1,
-- 
2.53.0-Meta

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

[Zinc Lim]

Firmware reports the range a sensor has entered on every threshold
crossing, including the one back into the normal range. Expose that as
temp1_alarm and in0_alarm, so the alarm reflects a condition the
hardware raised and an excursion that starts and ends between two sysfs
reads is still caught. Each crossing wakes anything polling the
attribute through hwmon_notify_event().

Each alarm is only visible if all of its thresholds are reported by the
firmware. Voltage has no crit threshold, only min and max.

Firmware only reports a range when it changes, so a sensor already out of
range when the driver attaches has nothing cached. The first alarm read
resolves that via a sensor read, its response carrying the range.
Old firmware that doesn't report a range leaves the alarm to be
classified from the reading against the thresholds, with no range to
notify on.

fbnic_hwmon_notify_event() runs from the mailbox IRQ, so it can race
registration and teardown. Publish fbd->hwmon with WRITE_ONCE() only
after a successful registration, clear it with WRITE_ONCE(NULL) followed
by synchronize_irq() to drain any in-flight handler, and skip the
notification when the read returns 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    |   8 +-
 drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c | 146 ++++++++++++++++--
 3 files changed, 142 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic.h b/drivers/net/ethernet/meta/fbnic/fbnic.h
index 93bf7004b67d..90e54cffe6a9 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic.h
@@ -200,6 +200,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);
 
 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 7863f6ba1e7d..9485665cba46 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
@@ -1738,8 +1738,10 @@ static int fbnic_fw_parse_threshold_exceeded_resp(void *opaque,
 				alarm, value);
 
 		/* Old firmware reports no range, leave the cache alone */
-		if (alarm != FBNIC_FW_TSENE_ALARM_DEPRECATED)
+		if (alarm != FBNIC_FW_TSENE_ALARM_DEPRECATED) {
 			WRITE_ONCE(fbd->hwmon_cache.temp_alarm, alarm);
+			fbnic_hwmon_notify_event(fbd, FBNIC_SENSOR_TEMP);
+		}
 		rec_flag = true;
 	}
 
@@ -1756,8 +1758,10 @@ static int fbnic_fw_parse_threshold_exceeded_resp(void *opaque,
 				alarm, value);
 
 		/* Old firmware reports no range, leave the cache alone */
-		if (alarm != FBNIC_FW_TSENE_ALARM_DEPRECATED)
+		if (alarm != FBNIC_FW_TSENE_ALARM_DEPRECATED) {
 			WRITE_ONCE(fbd->hwmon_cache.volt_alarm, alarm);
+			fbnic_hwmon_notify_event(fbd, FBNIC_SENSOR_VOLTAGE);
+		}
 		rec_flag = true;
 	}
 
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
index 4df808f850cf..310a2c52bb4d 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
@@ -29,6 +29,10 @@ static umode_t fbnic_hwmon_is_visible(const void *drvdata,
 			return fbnic_hwmon_thr_visible(fbd->fw_cap.temp.max);
 		case hwmon_temp_crit:
 			return fbnic_hwmon_thr_visible(fbd->fw_cap.temp.crit);
+		case hwmon_temp_alarm:
+			return fbnic_hwmon_thr_visible(fbd->fw_cap.temp.min) &
+			       fbnic_hwmon_thr_visible(fbd->fw_cap.temp.max) &
+			       fbnic_hwmon_thr_visible(fbd->fw_cap.temp.crit);
 		}
 		break;
 	case hwmon_in:
@@ -39,6 +43,9 @@ static umode_t fbnic_hwmon_is_visible(const void *drvdata,
 			return fbnic_hwmon_thr_visible(fbd->fw_cap.volt.min);
 		case hwmon_in_max:
 			return fbnic_hwmon_thr_visible(fbd->fw_cap.volt.max);
+		case hwmon_in_alarm:
+			return fbnic_hwmon_thr_visible(fbd->fw_cap.volt.min) &
+			       fbnic_hwmon_thr_visible(fbd->fw_cap.volt.max);
 		}
 		break;
 	default:
@@ -120,6 +127,74 @@ static int fbnic_hwmon_read_threshold(long thr, long *val)
 	return 0;
 }
 
+static bool fbnic_hwmon_at_or_above(long val, s32 thr)
+{
+	return thr != FBNIC_SENSOR_NO_DATA && val >= thr;
+}
+
+static bool fbnic_hwmon_below(long val, s32 thr)
+{
+	return thr != FBNIC_SENSOR_NO_DATA && val < thr;
+}
+
+static u8 fbnic_hwmon_classify(const struct fbnic_threshold *thr, long val)
+{
+	if (fbnic_hwmon_at_or_above(val, thr->crit))
+		return FBNIC_FW_TSENE_ALARM_CRIT;
+	if (fbnic_hwmon_at_or_above(val, thr->max))
+		return FBNIC_FW_TSENE_ALARM_ABOVE_MAX;
+	if (fbnic_hwmon_below(val, thr->min))
+		return FBNIC_FW_TSENE_ALARM_BELOW_MIN;
+
+	return FBNIC_FW_TSENE_ALARM_NORMAL;
+}
+
+static int fbnic_hwmon_alarm_read(struct fbnic_dev *fbd,
+				  enum hwmon_sensor_types type, long *val)
+{
+	struct fbnic_hwmon_cache *cache = &fbd->hwmon_cache;
+	const struct fbnic_threshold *thr;
+	u8 *cached, alarm;
+	long reading;
+	int id, err;
+
+	switch (type) {
+	case hwmon_temp:
+		thr = &fbd->fw_cap.temp;
+		cached = &cache->temp_alarm;
+		id = FBNIC_SENSOR_TEMP;
+		break;
+	case hwmon_in:
+		thr = &fbd->fw_cap.volt;
+		cached = &cache->volt_alarm;
+		id = FBNIC_SENSOR_VOLTAGE;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	/* Paired with WRITE_ONCE() on the mailbox thread. Firmware only
+	 * reports on a threshold crossing, so nothing is cached until the
+	 * first one. Read the sensor, whose response carries the range.
+	 */
+	alarm = READ_ONCE(*cached);
+	if (alarm == FBNIC_FW_TSENE_ALARM_UNKNOWN) {
+		err = fbnic_hwmon_sensor_read(fbd, id, &reading);
+		if (err)
+			return err;
+
+		alarm = READ_ONCE(*cached);
+
+		/* Firmware too old to report a range, derive it instead */
+		if (alarm == FBNIC_FW_TSENE_ALARM_UNKNOWN)
+			alarm = fbnic_hwmon_classify(thr, reading);
+	}
+
+	*val = alarm != FBNIC_FW_TSENE_ALARM_NORMAL;
+
+	return 0;
+}
+
 static int fbnic_hwmon_temp_read(struct fbnic_dev *fbd, u32 attr, long *val)
 {
 	switch (attr) {
@@ -131,6 +206,8 @@ static int fbnic_hwmon_temp_read(struct fbnic_dev *fbd, u32 attr, long *val)
 		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_alarm:
+		return fbnic_hwmon_alarm_read(fbd, hwmon_temp, val);
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -145,6 +222,8 @@ static int fbnic_hwmon_in_read(struct fbnic_dev *fbd, u32 attr, long *val)
 		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_alarm:
+		return fbnic_hwmon_alarm_read(fbd, hwmon_in, val);
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -172,10 +251,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_T_MIN | HWMON_T_MAX |
-			   HWMON_T_CRIT),
+			   HWMON_T_INPUT | HWMON_T_ALARM |
+			   HWMON_T_MIN | HWMON_T_MAX | HWMON_T_CRIT),
 	HWMON_CHANNEL_INFO(in,
-			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX),
+			   HWMON_I_INPUT | HWMON_I_ALARM |
+			   HWMON_I_MIN | HWMON_I_MAX),
 	NULL
 };
 
@@ -186,25 +266,69 @@ 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;
 
-	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)
+{
+	enum hwmon_sensor_types type;
+	struct device *hwmon;
+	u32 attr;
+
+	if (!IS_REACHABLE(CONFIG_HWMON))
+		return;
+
+	switch (id) {
+	case FBNIC_SENSOR_TEMP:
+		type = hwmon_temp;
+		attr = hwmon_temp_alarm;
+		break;
+	case FBNIC_SENSOR_VOLTAGE:
+		type = hwmon_in;
+		attr = hwmon_in_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 (hwmon)
+		hwmon_notify_event(hwmon, type, attr, 0);
 }
-- 
2.53.0-Meta

[PATCH net-next v4 6/6] eth: fbnic: derive the alarm range on older firmware

[Zinc Lim]

Firmware that predates range reporting uses the threshold event to flag
that a limit was exceeded, with the attribute hardcoded to 1, rather than
reporting which range the sensor entered. The range enum reserves that
value as FBNIC_FW_TSENE_ALARM_DEPRECATED. It does not indicate which
limit was crossed, so it is not cached and the alarm classifies a fresh
reading on every sysfs read.

Such firmware re-arms the trip points around the range the reading
landed in, so every crossing interrupts, including the one back to
normal. The event carries the reading that tripped it, so derive the
range from that, against the same capability response thresholds and
boundaries firmware uses. The alarm then tracks crossings there too, and
wakes anything polling the sysfs attribute.

An event can arrive before the capability response that carries those
thresholds, since rx messages are parsed while polling for it. There is
nothing to classify against yet, so the reading would come back as the
normal range and stick there. Leave the range unknown and let the read
path derive it once the thresholds are in.

Signed-off-by: Zinc Lim <limzhineng2@gmail.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic.h       |  1 +
 drivers/net/ethernet/meta/fbnic/fbnic_fw.c    | 18 +++++++++---
 drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c | 29 +++++++++++++++++++
 3 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic.h b/drivers/net/ethernet/meta/fbnic/fbnic.h
index 90e54cffe6a9..8180028b20a2 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic.h
@@ -201,6 +201,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);
+void fbnic_hwmon_old_notify_event(struct fbnic_dev *fbd, int id, s32 value);
 
 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 9485665cba46..cfec30e5cb98 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
@@ -1737,8 +1737,13 @@ static int fbnic_fw_parse_threshold_exceeded_resp(void *opaque,
 			dev_err(fbd->dev, "Thermal alarm %u, temperature is currently %dmC!\n",
 				alarm, value);
 
-		/* Old firmware reports no range, leave the cache alone */
-		if (alarm != FBNIC_FW_TSENE_ALARM_DEPRECATED) {
+		if (alarm == FBNIC_FW_TSENE_ALARM_DEPRECATED) {
+			/* Nothing to derive the range from, so do not guess */
+			if (have_temp)
+				fbnic_hwmon_old_notify_event(fbd,
+							     FBNIC_SENSOR_TEMP,
+							     value);
+		} else {
 			WRITE_ONCE(fbd->hwmon_cache.temp_alarm, alarm);
 			fbnic_hwmon_notify_event(fbd, FBNIC_SENSOR_TEMP);
 		}
@@ -1757,8 +1762,13 @@ static int fbnic_fw_parse_threshold_exceeded_resp(void *opaque,
 			dev_err(fbd->dev, "Voltage alarm %u, voltage is currently %dmV!\n",
 				alarm, value);
 
-		/* Old firmware reports no range, leave the cache alone */
-		if (alarm != FBNIC_FW_TSENE_ALARM_DEPRECATED) {
+		if (alarm == FBNIC_FW_TSENE_ALARM_DEPRECATED) {
+			/* Nothing to derive the range from, so do not guess */
+			if (have_volt)
+				fbnic_hwmon_old_notify_event(fbd,
+							     FBNIC_SENSOR_VOLTAGE,
+							     value);
+		} else {
 			WRITE_ONCE(fbd->hwmon_cache.volt_alarm, alarm);
 			fbnic_hwmon_notify_event(fbd, FBNIC_SENSOR_VOLTAGE);
 		}
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
index 310a2c52bb4d..9fa9eb843c68 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
@@ -302,6 +302,35 @@ void fbnic_hwmon_unregister(struct fbnic_dev *fbd)
 	hwmon_device_unregister(hwmon);
 }
 
+void fbnic_hwmon_old_notify_event(struct fbnic_dev *fbd, int id, s32 value)
+{
+	const struct fbnic_threshold *thr;
+	u8 *cached;
+
+	switch (id) {
+	case FBNIC_SENSOR_TEMP:
+		thr = &fbd->fw_cap.temp;
+		cached = &fbd->hwmon_cache.temp_alarm;
+		break;
+	case FBNIC_SENSOR_VOLTAGE:
+		thr = &fbd->fw_cap.volt;
+		cached = &fbd->hwmon_cache.volt_alarm;
+		break;
+	default:
+		return;
+	}
+
+	/* Event arrived before the capability response containing
+	 * the thresholds.
+	 */
+	if (thr->min == FBNIC_SENSOR_NO_DATA ||
+	    thr->max == FBNIC_SENSOR_NO_DATA)
+		return;
+
+	WRITE_ONCE(*cached, fbnic_hwmon_classify(thr, value));
+	fbnic_hwmon_notify_event(fbd, id);
+}
+
 void fbnic_hwmon_notify_event(struct fbnic_dev *fbd, int id)
 {
 	enum hwmon_sensor_types type;
-- 
2.53.0-Meta

Re: [PATCH net-next v4 2/6] eth: fbnic: cache hwmon sensor readings

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] fbnic_mbx_init()
  (drivers/net/ethernet/meta/fbnic/fbnic_fw.c:112-113) resets…

--

eth: fbnic: cache hwmon sensor readings

This commit caches the last TSENE temperature and voltage readings in
struct fbnic_dev. Hwmon reads are served from that cache for the rest of
the current jiffy. The cache is seeded with the FBNIC_SENSOR_NO_DATA
sentinel in fbnic_mbx_init(), so the first read always goes to firmware.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index 49b8813a35643..82307f212f583 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> @@ -105,6 +105,13 @@ void fbnic_mbx_init(struct fbnic_dev *fbd)
>  	/* Reset FW Capabilities */
>  	memset(&fbd->fw_cap, 0, sizeof(fbd->fw_cap));
>  
> +	/* Seed cache with sentinel so the first read always refreshes. The
> +	 * cache mirrors what firmware reported, so drop it when the mailbox
> +	 * restarts.
> +	 */
> +	fbd->hwmon_cache.temp_mdeg = FBNIC_SENSOR_NO_DATA;
> +	fbd->hwmon_cache.volt_mv = FBNIC_SENSOR_NO_DATA;

[Severity: Low]

Can these stores race with a sysfs read in fbnic_hwmon_sensor_read()?

The hwmon device stays registered during PCI error recovery.
fbnic_hwmon_unregister() is only called from fbnic_remove(). The mailbox
is restarted on that path:

fbnic_err_slot_reset()
  __fbnic_pm_resume()
    fbnic_fw_request_mbx()
      __fbnic_fw_enable_mbx()
        fbnic_mbx_init()

This path runs under devl_lock(). The hwmon read callbacks are only
serialized by the hwmon core lock, so nothing orders the two.

Suppose a reader has already received a successful TSENE completion and is
preempted just before this code in fbnic_hwmon_sensor_read():

	cache->temp_mdeg = fw_cmpl->u.tsene.millidegrees;
	cache->volt_mv = fw_cmpl->u.tsene.millivolts;
	cache->last_read = jiffies;

fbnic_mbx_init() can then store FBNIC_SENSOR_NO_DATA. When the reader
resumes, it writes back the pre-reset readings with a fresh last_read.

Wouldn't that undo the "drop it when the mailbox restarts" invalidation?
KCSAN would also flag these plain concurrent loads and stores.

The practical effect is small. FBNIC depends on 64BIT, so the s32 stores
can't tear, and a stale value would only be served for the rest of one
jiffy.

Could the invalidation be done under hwmon_lock(fbd->hwmon) when
fbd->hwmon is set? Another option is READ_ONCE()/WRITE_ONCE() with a
validity scheme.

The same unlocked pattern appears to remain later in the series, where
temp_alarm and volt_alarm are also seeded in fbnic_mbx_init() without the
lock.

> +
>  	/* Reinitialize mailbox memory */
>  	for (i = 0; i < FBNIC_IPC_MBX_INDICES; i++)
>  		memset(&fbd->mbx[i], 0, sizeof(struct fbnic_fw_mbx));

[ ... ]

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index 6c8c66ab86c13..0c3fe2a372275 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c

[ ... ]

> @@ -72,7 +78,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;

[ ... ]

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

Re: [PATCH net-next v4 3/6] eth: fbnic: report temperature and voltage thresholds via hwmon

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] fbnic_hwmon_is_visible() decides whether temp1_{min,max,crit} and
  in0_{min,max} exist based on fbd->fw_cap.temp/volt.
- [Low] In fbnic_fw_parse_cap_resp(), the new threshold assignments come
  after fbnic_fw_parse_bmc_cap(), and that call returns early on error.
- [Low] The new threshold fields fbd->fw_cap.temp.{min,max,crit} and
  fbd->fw_cap.volt.{min,max} are read and written concurrently with…

--

eth: fbnic: report temperature and voltage thresholds via hwmon

This patch parses temperature (min/max/crit) and voltage (min/max)
thresholds from the firmware capability response and stores them in
fbnic_fw_cap. It then exposes them through hwmon as temp1_{min,max,crit}
and in0_{min,max}. Fields that firmware does not report are seeded with
FBNIC_SENSOR_NO_DATA and are hidden by is_visible.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index 82307f212f583..d9a00a2ef34da 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c

[ ... ]

> @@ -742,6 +757,22 @@ static int fbnic_fw_parse_cap_resp(void *opaque, struct fbnic_tlv_msg **results)
>  	fbd->fw_cap.anti_rollback_version =
>  		fta_get_uint(results, FBNIC_FW_CAP_RESP_ANTI_ROLLBACK_VERSION);
>  
> +	fbd->fw_cap.temp.min =
> +		fbnic_tlv_attr_get_signed(results[FBNIC_FW_CAP_RESP_TEMP_MIN],
> +					  FBNIC_SENSOR_NO_DATA);

[Severity: Low]
Should these threshold assignments come before the BMC parsing step? Just
above them in fbnic_fw_parse_cap_resp() there is an early return:

	err = fbnic_fw_parse_bmc_cap(fbd, results);
	if (err)
		return err;

If the response carries a malformed BMC MAC array, for example a bad
attribute id, a bad length, or more than 4 entries,
fbnic_fw_parse_bmc_addrs()->fbnic_tlv_attr_parse_array() returns -EINVAL
or -ENOSPC. The thresholds are then never stored and stay at the
FBNIC_SENSOR_NO_DATA value seeded by fbnic_mbx_init().

Probe still goes ahead in that case. mgmt.version was already stored at
the top of the function, so fbnic_mbx_poll_tx_ready() leaves its loop
here:

	if (fbd->fw_cap.running.mgmt.version >= MIN_FW_VER_CODE)
		break;

fbnic_probe() then calls fbnic_hwmon_register(). is_visible sees NO_DATA,
so none of the threshold attributes are created for the life of the hwmon
device. In the final series, temp1_alarm and in0_alarm are hidden too.

The commit message says fbnic_fw_parse_cap_resp() "can return before it
reaches the threshold assignments", but only to explain the sentinel.
Would it be better to parse the thresholds before the fallible BMC step,
so valid limits are not lost because an unrelated field failed to parse?

> +	fbd->fw_cap.temp.max =
> +		fbnic_tlv_attr_get_signed(results[FBNIC_FW_CAP_RESP_TEMP_MAX],
> +					  FBNIC_SENSOR_NO_DATA);

[ ... ]

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index 0c3fe2a372275..4df808f850cf8 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c

[ ... ]

>  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;
> +	const struct fbnic_dev *fbd = drvdata;
> +
> +	switch (type) {
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_input:
> +			return 0444;
> +		case hwmon_temp_min:
> +			return fbnic_hwmon_thr_visible(fbd->fw_cap.temp.min);

[Severity: Low]
The hwmon core calls is_visible only once, from
hwmon_device_register_with_info(). That happens when fbnic_probe() calls
fbnic_hwmon_register(). Can the attribute set get out of sync with
fw_cap after that?

On resume, hibernation restore and AER slot reset, the mailbox is
reinitialized while the hwmon device stays registered:

  __fbnic_pm_resume()
    fbnic_fw_request_mbx()
      __fbnic_fw_enable_mbx()
        fbnic_mbx_init()   /* thresholds reset to NO_DATA */

The next capability response then fills the thresholds again.

The patch handles thresholds that go away (reads return -ENODATA). What
about thresholds that show up after probe? They are stored in fw_cap but
never become sysfs attributes until the driver is re-probed.

That could happen if the capability parse at probe failed early, as
described for fbnic_fw_parse_cap_resp() above, and a later resume parses
cleanly. It could also happen if new firmware flashed through devlink
starts running after a hibernation restore or slot reset without a
re-probe.

In the final series the same probe-time snapshot also decides whether
temp1_alarm and in0_alarm exist.

> +		case hwmon_temp_max:
> +			return fbnic_hwmon_thr_visible(fbd->fw_cap.temp.max);

[ ... ]

> @@ -92,14 +111,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;
> +}

[Severity: Low]
Are these threshold reads safe against the mailbox writers? The reads of
fbd->fw_cap.temp.* and fbd->fw_cap.volt.* in fbnic_hwmon_temp_read() and
fbnic_hwmon_in_read() are plain loads, and readers hold only the hwmon
core lock.

The writers are fbnic_mbx_init(), which does a whole-struct copy:

	fbd->fw_cap = fw_cap_reset;

and fbnic_fw_parse_cap_resp(), which uses plain scalar stores.

Both run from __fbnic_pm_resume(). fbnic_err_slot_reset() calls that
during AER recovery while userspace is running and the hwmon sysfs files
still exist. That path holds only the devlink lock, which hwmon readers
never take.

The commit message says reads "reject the sentinel with -ENODATA rather
than reporting it as a value". That relies on each read being a single,
untorn load.

Without READ_ONCE()/WRITE_ONCE(), after inlining the compiler may load
the field once for the thr == FBNIC_SENSOR_NO_DATA test and again for
*val = thr. A memcpy-based struct copy also gives no per-s32 atomicity.
Could a reader then report S32_MIN or a torn value? KCSAN would likely
flag this pair of accesses.

Later patches in the series annotate the lockless alarm cache with
READ_ONCE()/WRITE_ONCE() ("Paired with WRITE_ONCE() on the mailbox
thread"). The threshold fields are still unannotated at the end of the
series.

> +
> +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);

[ ... ]

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

Re: [PATCH net-next v4 4/6] eth: fbnic: parse the alarm range reported by firmware

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The firmware alarm range is a U32 TLV attribute, but
  fbnic_fw_parse_tsene_read_resp() and…

--

eth: fbnic: parse the alarm range reported by firmware

This commit parses the per-sensor alarm range that firmware reports in
two places: the TSENE read response and a new unsolicited
SENSOR_THRESHOLD_EXCEEDED event. It caches the range in u8 fields in
hwmon_cache. Both fields are seeded with FBNIC_FW_TSENE_ALARM_UNKNOWN
in fbnic_mbx_init().

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic.h b/drivers/net/ethernet/meta/fbnic/fbnic.h
> index 857a64b518f38..93bf7004b67d9 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic.h
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic.h
> @@ -26,6 +26,8 @@ struct fbnic_hwmon_cache {
>  	unsigned long last_read;
>  	s32 temp_mdeg;
>  	s32 volt_mv;
> +	u8 temp_alarm;
> +	u8 volt_alarm;
>  };

[ ... ]

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index d9a00a2ef34da..7863f6ba1e7da 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c

[ ... ]

> @@ -1466,6 +1468,8 @@ static const struct fbnic_tlv_index fbnic_tsene_read_resp_index[] = {
>  	FBNIC_TLV_ATTR_S32(FBNIC_FW_TSENE_THERM),
>  	FBNIC_TLV_ATTR_S32(FBNIC_FW_TSENE_VOLT),
>  	FBNIC_TLV_ATTR_S32(FBNIC_FW_TSENE_ERROR),
> +	FBNIC_TLV_ATTR_U32(FBNIC_FW_TSENE_THERM_ALARM_STATE),
> +	FBNIC_TLV_ATTR_U32(FBNIC_FW_TSENE_VOLT_ALARM_STATE),
>  	FBNIC_TLV_ATTR_LAST
>  };

[ ... ]

> @@ -1497,6 +1502,20 @@ static int fbnic_fw_parse_tsene_read_resp(void *opaque,
>  	cmpl_data->u.tsene.millivolts =
>  		fta_get_sint(results, FBNIC_FW_TSENE_VOLT);
>  
> +	/* Cache here so that it and the threshold event land in the order firmware
> +	 * sent them. Firmware predating alarm state reporting sends neither
> +	 * attribute.
> +	 */
> +	alarm = fbnic_tlv_attr_get_unsigned(results[FBNIC_FW_TSENE_THERM_ALARM_STATE],
> +					    FBNIC_FW_TSENE_ALARM_UNKNOWN);
> +	if (alarm != FBNIC_FW_TSENE_ALARM_UNKNOWN)
> +		WRITE_ONCE(fbd->hwmon_cache.temp_alarm, alarm);
> +
> +	alarm = fbnic_tlv_attr_get_unsigned(results[FBNIC_FW_TSENE_VOLT_ALARM_STATE],
> +					    FBNIC_FW_TSENE_ALARM_UNKNOWN);
> +	if (alarm != FBNIC_FW_TSENE_ALARM_UNKNOWN)
> +		WRITE_ONCE(fbd->hwmon_cache.volt_alarm, alarm);

[Severity: Low]

The alarm state is parsed as a U32 attribute but stored in the u8
fields hwmon_cache.temp_alarm and volt_alarm. The only check here is
against FBNIC_FW_TSENE_ALARM_UNKNOWN, and it runs on the u32 before
the value is narrowed.

What happens if firmware sends a value that does not fit in a u8? 0x100
would be cached as 0 (FBNIC_FW_TSENE_ALARM_NORMAL), 0x101 as
FBNIC_FW_TSENE_ALARM_DEPRECATED, and 0x1ff as
FBNIC_FW_TSENE_ALARM_UNKNOWN.

fbnic_fw_parse_threshold_exceeded_resp() has the same pattern. There,
only DEPRECATED is filtered before the store:

		if (alarm != FBNIC_FW_TSENE_ALARM_DEPRECATED)
			WRITE_ONCE(fbd->hwmon_cache.temp_alarm, alarm);

The volt_alarm store in that function works the same way.

This only happens if firmware sends a value outside its documented set
(NORMAL, BELOW_MIN, ABOVE_MAX, CRIT). The worst case looks like a
wrong hwmon alarm value, since the cached byte is only compared and
never used as an index.

Should the value be range checked before it is narrowed into the u8
cache in both parsers?

>  msg_err:
>  	cmpl_data->result = err_resp ? : err;
>  	complete(&cmpl_data->done);

[ ... ]

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

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

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says that exposing the firmware range as
  temp1_alarm/in0_alarm means "an excursion that starts and ends between…
- [Medium] On firmware that does not report a range,
  fbnic_hwmon_alarm_read() produces temp1_alarm/in0_alarm by comparing a…
- [Medium] fbnic_hwmon_register() publishes the new hwmon device to the
  mailbox IRQ thread with a plain WRITE_ONCE(fbd->hwmon, hwmon), which…
- [Medium] hwmon_cache.temp_alarm/volt_alarm are reset to
  FBNIC_FW_TSENE_ALARM_UNKNOWN only in fbnic_mbx_init().
- [Low] fbnic_hwmon_is_visible() hides temp1_alarm unless
  fw_cap.temp.min, max and crit are all reported, and hides in0_alarm…
- [Low] The new synchronize_irq(fbd->fw_msix_vector) in
  fbnic_hwmon_unregister() does not check that the FW mailbox vector is…

--

eth: fbnic: report temperature and voltage alarms via hwmon

This adds temp1_alarm and in0_alarm hwmon attributes to fbnic. They are
backed by the range firmware reports on each threshold crossing. When
nothing is cached, the driver falls back to a sensor read, or to
comparing the reading against the thresholds.

Pollers are woken through hwmon_notify_event() from the mailbox IRQ path.
fbd->hwmon is now published with WRITE_ONCE() and torn down with
synchronize_irq().

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index 4df808f850cf8..310a2c52bb4df 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c

[ ... ]

> @@ -120,6 +127,74 @@ static int fbnic_hwmon_read_threshold(long thr, long *val)

[ ... ]

> +static int fbnic_hwmon_alarm_read(struct fbnic_dev *fbd,
> +				  enum hwmon_sensor_types type, long *val)
> +{

[ ... ]

> +	/* Paired with WRITE_ONCE() on the mailbox thread. Firmware only
> +	 * reports on a threshold crossing, so nothing is cached until the
> +	 * first one. Read the sensor, whose response carries the range.
> +	 */
> +	alarm = READ_ONCE(*cached);
> +	if (alarm == FBNIC_FW_TSENE_ALARM_UNKNOWN) {

[Severity: Medium]
Can this return a range that a previous firmware instance reported?

temp_alarm and volt_alarm are only reset to FBNIC_FW_TSENE_ALARM_UNKNOWN in
fbnic_mbx_init(). That only runs from fbnic_fw_request_mbx(), at probe and
resume.

A firmware reboot while the host mailbox stays up takes a different path:

fbnic_fw_check_heartbeat()
    fbd->firmware_time < fbd->prev_firmware_time
    fw_heartbeat_enabled = false
fbnic_health_check()
    fbnic_fw_config_after_crash()

fbnic_fw_config_after_crash() sends the ownership message and reprograms
the RX filters. It does not touch hwmon_cache.

The commit message says firmware only reports a range when it changes. This
function only queries firmware while the cache is
FBNIC_FW_TSENE_ALARM_UNKNOWN.

After a firmware reboot, could temp1_alarm and in0_alarm keep showing an
alarm that never clears? Or could a stale FBNIC_FW_TSENE_ALARM_NORMAL hide a
sensor that was already out of range when the new firmware booted?

On firmware that reports ranges, reading temp1_input or in0_input would
refresh the cache through the TSENE read response. On old firmware, at the
end of the series, the cache is written by fbnic_hwmon_old_notify_event()
and read responses never refresh it. The stale value would then stay until
the next threshold event.

> +		err = fbnic_hwmon_sensor_read(fbd, id, &reading);
> +		if (err)
> +			return err;
> +
> +		alarm = READ_ONCE(*cached);
> +
> +		/* Firmware too old to report a range, derive it instead */
> +		if (alarm == FBNIC_FW_TSENE_ALARM_UNKNOWN)
> +			alarm = fbnic_hwmon_classify(thr, reading);

[Severity: Medium]
Is this fallback allowed by the hwmon ABI? 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.

On firmware that doesn't report a range, the cache stays at
FBNIC_FW_TSENE_ALARM_UNKNOWN. fbnic_fw_parse_threshold_exceeded_resp() does
not cache FBNIC_FW_TSENE_ALARM_DEPRECATED events, and the read response has
no *_ALARM_STATE attribute. Every read of temp1_alarm or in0_alarm therefore
ends up in fbnic_hwmon_classify().

fbnic_hwmon_is_visible() still exposes the attribute whenever the thresholds
are present. Userspace can't tell an alarm the hardware raised from one the
driver computed. The value adds nothing over comparing temp1_input with
temp1_min, temp1_max and temp1_crit.

The last patch in the series, "eth: fbnic: derive the alarm range on older
firmware", keeps this fallback. It also adds a second driver-side
classification in fbnic_hwmon_old_notify_event().

> +	}
> +
> +	*val = alarm != FBNIC_FW_TSENE_ALARM_NORMAL;

[Severity: Medium]
The commit message says:

    Expose that as temp1_alarm and in0_alarm, so the alarm reflects a
    condition the hardware raised and an excursion that starts and ends
    between two sysfs reads is still caught.

Does the code catch that case? Nothing latches the alarm, and reading it
does not clear anything.

fbnic_fw_parse_threshold_exceeded_resp() overwrites the cache with every
range it receives, including the crossing back to normal:

		if (alarm != FBNIC_FW_TSENE_ALARM_DEPRECATED) {
			WRITE_ONCE(fbd->hwmon_cache.temp_alarm, alarm);
			fbnic_hwmon_notify_event(fbd, FBNIC_SENSOR_TEMP);
		}

Take this sequence:

    read of temp1_alarm returns 0
    firmware sends FBNIC_FW_TSENE_ALARM_ABOVE_MAX
    firmware sends FBNIC_FW_TSENE_ALARM_NORMAL
    read of temp1_alarm

The second read also returns 0.

The hwmon_notify_event() wakeups don't close this gap either. If both events
are handled before the woken poller reads the attribute, the poller still
sees 0. The KOBJ_CHANGE uevent carries only the attribute name, not a value.

The last patch in the series, "eth: fbnic: derive the alarm range on older
firmware", does not add latching either.

> +
> +	return 0;
> +}

[ ... ]

> @@ -186,25 +266,69 @@ static const struct hwmon_chip_info fbnic_chip_info = {
>  
>  void fbnic_hwmon_register(struct fbnic_dev *fbd)
>  {

[ ... ]

> +	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: Medium]
Is WRITE_ONCE() enough to publish the device to the mailbox IRQ thread?

fbnic_probe() enables the mailbox IRQ in fbnic_fw_request_mbx() before it
calls fbnic_hwmon_register(). A threshold event can therefore reach
fbnic_hwmon_notify_event() on another CPU while registration is still in
progress.

On this path, __hwmon_device_register() calls
INIT_LIST_HEAD(&hwdev->tzdata) after device_register() and then returns.
The HWMON_C_REGISTER_TZ block is skipped because chip->info[0]->type is
hwmon_temp, not hwmon_chip.

WRITE_ONCE() has no release semantics. Nothing orders the tzdata list head
stores before the store to fbd->hwmon.

On a weakly ordered CPU such as arm64, with CONFIG_THERMAL_OF enabled, could
the reader see a non-NULL fbd->hwmon and still walk a list head that is
zero from kzalloc()?

fbnic_hwmon_notify_event()
    hwmon_notify_event()
        hwmon_thermal_notify()
            list_for_each_entry(tzdata, &hwdev->tzdata, node)

The window is very narrow, but the commit message relies on this pairing to
make registration safe. Would smp_store_release() with smp_load_acquire(),
or rcu_assign_pointer() with rcu_dereference(), give the ordering it
describes? The code is unchanged at the end of the series.

>  
>  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 fw_msix_vector be 0, or stale, by the time this runs?

hwmon stays registered across suspend and resume. It is only unregistered
from fbnic_remove(). If __fbnic_pm_resume() fails, a later remove can see
one of two values.

The first is 0. fbnic_pm_suspend() calls fbnic_fw_free_mbx(), which sets
0. It stays 0 if fbnic_alloc_irqs() then fails during resume. The
err_free_mbx path also leaves it at 0.

The second is a stale vector, when __fbnic_fw_enable_mbx() fails.
fbnic_fw_request_mbx() frees the IRQ and still stores the vector:

	if (err)
		free_irq(vector, fbd);

	fbd->fw_msix_vector = vector;

fbnic_free_irqs() then releases the PCI vectors.

A later fbnic_remove()->fbnic_hwmon_unregister() would then call
synchronize_irq(0), or synchronize_irq() on a virq that may since belong to
another device.

free_irq() has already drained the fbnic mailbox handler, so this does not
look like a use-after-free, only a wait on an unrelated IRQ. Should the call
be guarded with if (fbd->fw_msix_vector)?

The stale assignment in fbnic_fw_request_mbx() itself predates this patch.

> +
> +	hwmon_device_unregister(hwmon);
> +}
> +
> +void fbnic_hwmon_notify_event(struct fbnic_dev *fbd, int id)
> +{

[ ... ]

> +	/* 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 (hwmon)
> +		hwmon_notify_event(hwmon, type, attr, 0);

[Severity: Low]
Can this send a uevent for an attribute that does not exist?

fbnic_hwmon_is_visible() hides temp1_alarm unless the temperature min, max
and crit thresholds are all reported. It hides in0_alarm unless the voltage
min and max are both reported.

fbnic_fw_parse_cap_resp() parses each threshold as an optional TLV on its
own:

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

fbnic_fw_parse_threshold_exceeded_resp() calls fbnic_hwmon_notify_event()
for every range other than FBNIC_FW_TSENE_ALARM_DEPRECATED. It does not
check visibility first.

hwmon_notify_event() calls sysfs_notify(), which does nothing for a missing
attribute. It then always emits KOBJ_CHANGE with NAME=temp1_alarm or
NAME=in0_alarm.

So firmware that reports ranges but omits, say, the TEMP_CRIT TLV would
produce change uevents for a sysfs attribute that isn't there.

The mismatch remains at the end of the series.
fbnic_hwmon_old_notify_event() checks only min and max, while temp1_alarm
visibility also requires crit.

>  }

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

Re: [PATCH net-next v4 6/6] eth: fbnic: derive the alarm range on older firmware

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] fbnic_fw_parse_threshold_exceeded_resp() decides whether to
  log at dev_err level from the raw firmware attribute (`alarm !=…
- [Medium] The new legacy-event path computes the hwmon alarm by
  comparing a reading against thresholds in software, which contradicts…
- [Medium] Once fbnic_hwmon_old_notify_event() has cached a range for old
  firmware, a later FBNIC_FW_TSENE_ALARM_DEPRECATED event without a…

--

eth: fbnic: derive the alarm range on older firmware

Older firmware sends threshold events with the alarm attribute hardcoded
to FBNIC_FW_TSENE_ALARM_DEPRECATED. With this patch, the driver derives
the range from the reading in the event, using the capability response
thresholds, caches it and notifies hwmon pollers. If the thresholds are
not known yet, the cache is left unknown.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index 9485665cba467..cfec30e5cb988 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> @@ -1737,8 +1737,13 @@ static int fbnic_fw_parse_threshold_exceeded_resp(void *opaque,
>  			dev_err(fbd->dev, "Thermal alarm %u, temperature is currently %dmC!\n",
>  				alarm, value);

[Severity: Medium]
On old firmware, alarm is always FBNIC_FW_TSENE_ALARM_DEPRECATED (1). So
the "alarm != FBNIC_FW_TSENE_ALARM_NORMAL" check above is true for every
event, including the crossing back to normal.

With this patch, fbnic_hwmon_old_notify_event() classifies that same
reading as FBNIC_FW_TSENE_ALARM_NORMAL, caches it and wakes pollers.
temp1_alarm then reads 0.

Won't the same event still print "Thermal alarm 1, temperature is
currently ...mC!" at error level? Here the 1 is the deprecated marker,
not a range. The kernel log and the hwmon alarm attribute then disagree
on whether an alarm is active.

The voltage branch has the same check in front of "Voltage alarm %u,
voltage is currently %dmV!".

Could the log decision use the derived range instead? Two ways would be
to classify before logging, or to have fbnic_hwmon_old_notify_event()
return the range.

>  
> -		/* Old firmware reports no range, leave the cache alone */
> -		if (alarm != FBNIC_FW_TSENE_ALARM_DEPRECATED) {
> +		if (alarm == FBNIC_FW_TSENE_ALARM_DEPRECATED) {
> +			/* Nothing to derive the range from, so do not guess */
> +			if (have_temp)
> +				fbnic_hwmon_old_notify_event(fbd,
> +							     FBNIC_SENSOR_TEMP,
> +							     value);
> +		} else {

[Severity: Medium]
Old firmware can send an event that has FBNIC_FW_TSENE_THERM_ALARM but
no FBNIC_FW_TSENE_THERMAL. What happens to the cached range in that case?

This branch does nothing for such an event. If an earlier event already
cached a range through fbnic_hwmon_old_notify_event(), that range stays.
By the model in the commit message, though, every old-firmware event
marks a crossing.

fbnic_hwmon_alarm_read() only derives the range again when the cache is
unknown:

	alarm = READ_ONCE(*cached);
	if (alarm == FBNIC_FW_TSENE_ALARM_UNKNOWN) {
		err = fbnic_hwmon_sensor_read(fbd, id, &reading);
		...

Would temp1_alarm keep reporting the stale value until the next complete
event, for example 1 after the sensor is back to normal?

Before this patch this was harmless, because old firmware never filled
the cache. The driver already expects this message shape, since it logs
"Thermal alarm %u received with no temperature!". The voltage branch
does the same with have_volt.

Would writing FBNIC_FW_TSENE_ALARM_UNKNOWN in the !have_temp and
!have_volt cases let the read path recover? That would match the commit
message's "Leave the range unknown and let the read path derive it"
approach.

[ ... ]

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index 310a2c52bb4df..9fa9eb843c689 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> @@ -302,6 +302,35 @@ void fbnic_hwmon_unregister(struct fbnic_dev *fbd)
>  	hwmon_device_unregister(hwmon);
>  }
>  
> +void fbnic_hwmon_old_notify_event(struct fbnic_dev *fbd, int id, s32 value)
> +{

[ ... ]

> +	/* Event arrived before the capability response containing
> +	 * the thresholds.
> +	 */
> +	if (thr->min == FBNIC_SENSOR_NO_DATA ||
> +	    thr->max == FBNIC_SENSOR_NO_DATA)
> +		return;
> +
> +	WRITE_ONCE(*cached, fbnic_hwmon_classify(thr, value));

[Severity: Medium]
Documentation/hwmon/sysfs-interface.rst says:

  Alarms are direct indications read from the chips. The drivers do NOT
  make comparisons of readings to thresholds.

Here, fbnic_hwmon_classify() sets the cached alarm by comparing the
reading against the fw_cap crit, max and min thresholds.

Old firmware also sends this event on the return to normal, so the event
by itself does not say whether an alarm is active. The value of
temp1_alarm and in0_alarm therefore comes entirely from the driver's own
comparison.

The earlier patch in this series, "eth: fbnic: report temperature and
voltage alarms via hwmon", already does this kind of classification in
the read path, and this patch extends it to the event path.

Is this consistent with the documented hwmon ABI?

> +	fbnic_hwmon_notify_event(fbd, id);
> +}

[ ... ]

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