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

[PATCH net-next v3 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 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: 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 | 262 ++++++++++++++++--
 drivers/net/ethernet/meta/fbnic/fbnic_mac.c   |  55 ----
 drivers/net/ethernet/meta/fbnic/fbnic_mac.h   |   2 -
 6 files changed, 348 insertions(+), 84 deletions(-)

--
2.53.0-Meta

[PATCH net-next v3 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

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

[sashiko-bot]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908225737.3034403-1-limzhineng2@gmail.com?part=1

[PATCH net-next v3 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

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

[sashiko-bot]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908225737.3034403-1-limzhineng2@gmail.com?part=2

Re: [PATCH net-next v3 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_ops keeps a constant-returning .is_visible callback
  instead of the core-provided static-visibility field.

--

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/hwmon_temp_input
and hwmon_in/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 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;
>  }

[Severity: Low]
This isn't a bug, but is the callback still needed at all here?  After this
change fbnic_hwmon_is_visible() ignores all three of its discriminating
parameters (type, attr and channel) and returns a constant, which is what
the static visibility field in struct hwmon_ops already covers:

include/linux/hwmon.h:
    struct hwmon_ops {
        umode_t visible;
        umode_t (*is_visible)(const void *drvdata, enum hwmon_sensor_types type,
                              u32 attr, int channel);

The kernel-doc there describes visible as "Static visibility. If non-zero,
'is_visible' is ignored", and is_visible as "Mandatory unless 'visible' is
non-zero".  The core honours this in hwmon_is_visible():

drivers/hwmon/hwmon.c:hwmon_is_visible() {
    if (ops->visible)
        return ops->visible;

    return ops->is_visible(drvdata, type, attr, channel);
}

and hwmon_device_register_with_info() accepts a chip that supplies only
ops->visible.

Would it be preferable to delete fbnic_hwmon_is_visible() and write:

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

The callback is still a bare "return 0444;" at the end of the series
(d8d0db421cc5), so nothing later in the series appears to reintroduce
per-attribute discrimination.

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

[PATCH net-next v3 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 | 35 +++++++++++++------
 3 files changed, 39 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..c8e7ff1d36fc 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,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,
@@ -67,7 +73,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 +118,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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Hardware monitoring functionality is implemented directly within the network driver instead of as an auxiliary device in drivers/hwmon/.
--

commit 4fa7201fcb9ec899fe4c2586948bbac1ad1bc36d
Author: Zinc Lim <limzhineng2@gmail.com>

eth: fbnic: cache hwmon sensor readings

This commit adds a caching mechanism for hardware monitoring sensor readings
to reduce the number of firmware mailbox messages. It seeds the cache with
a sentinel value and caches both temperature and voltage readings returned by
a single firmware response.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index 38bb26cb8e6cb..c8e7ff1d36fc1 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
[ ... ]
> @@ -107,6 +118,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);

[Severity: High]
This isn't a bug introduced by this patch, but is a pre-existing structural
issue that was explicitly flagged by the subsystem guidelines.

Should this hardware monitoring functionality be implemented as an auxiliary
device driver residing in drivers/hwmon/ instead of directly within the
network driver?

The hardware monitoring subsystem guidelines state that for devices where the
main functionality is not hardware monitoring (such as a network interface
controller), its hwmon functionality must be implemented as an auxiliary
device driver in drivers/hwmon/. Registering the hwmon device unconditionally
here in fbnic_hwmon_register() using hwmon_device_register_with_info()
bypasses hwmon maintainer review and breaks subsystem layering.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908225737.3034403-1-limzhineng2@gmail.com?part=3

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

[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] Forward-referencing comment on the newly added
  FBNIC_SENSOR_NO_DATA macro in…

--

eth: fbnic: cache hwmon sensor readings

The patch caches the last TSENE temperature and voltage reading in
struct fbnic_dev and serves hwmon reads from that cache for the
remainder of the current jiffy, so a single firmware round-trip can
satisfy a batch of attribute reads. The cache is seeded at registration
with a new FBNIC_SENSOR_NO_DATA sentinel 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 d84723e4cfa3..42a5f83ddb45 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 does the comment describe more than this patch
implements?

At this commit the only users of FBNIC_SENSOR_NO_DATA are the cache seed
in fbnic_hwmon_register():

	fbd->hwmon_cache.temp_mdeg = FBNIC_SENSOR_NO_DATA;
	fbd->hwmon_cache.volt_mv = FBNIC_SENSOR_NO_DATA;

and the cache-validity test in fbnic_hwmon_sensor_read():

	if (*cached != FBNIC_SENSOR_NO_DATA && cache->last_read == jiffies) {

There is no threshold concept yet: struct fbnic_fw_cap has no temperature
or voltage threshold fields, and fbnic_hwmon_info[] registers only
temp1_input and in0_input.

The "a threshold the firmware never populated (older firmware)" wording
only becomes accurate later in the series, once the capability parser
starts using fbnic_tlv_attr_get_signed(..., FBNIC_SENSOR_NO_DATA) and
fbnic_hwmon_read_threshold()/fbnic_hwmon_read_alarm() compare against it.

Would it be clearer to restrict the comment to the cache semantics here
and extend it in the patch that adds the threshold handling?

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

[PATCH net-next v3 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 ff1674eff7ad..e5669dfd9790 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 c8e7ff1d36fc..7dc4d705f566 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)
@@ -87,14 +77,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 = {
@@ -103,8 +137,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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Unsupported threshold attributes are unconditionally exposed in sysfs, violating hwmon is_visible semantics.
--

commit 7ed3607652912ec9e3313daf244c76341872a2fc
Author: Zinc Lim <limzhineng2@gmail.com>

eth: fbnic: report temperature and voltage thresholds via hwmon

The firmware capability response carries per-board temperature
(min/max/crit) and voltage (min/max) thresholds. These are parsed and
exposed through the hwmon interface, returning -ENODATA if the firmware
did not report them.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index c8e7ff1d36fc1..7dc4d705f5662 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 umode_t fbnic_hwmon_is_visible(const void *drvdata,
>  				      enum hwmon_sensor_types type,
>  				      u32 attr, int channel)
>  {
>  	return 0444;
>  }

[Severity: Medium]
Does this unconditionally expose sysfs attributes even when they are not
supported by the firmware?

Since this patch introduces threshold values that might be missing and
represented by FBNIC_SENSOR_NO_DATA, the hwmon is_visible callback should
return 0 for those unsupported attributes rather than having the read
callback unconditionally return -ENODATA. Creating sysfs entries that
cannot be read clutters sysfs and violates the expected hwmon API contract.

Could fbnic_hwmon_is_visible be updated to inspect the cached capabilities
via drvdata and return 0 when the corresponding threshold is set to
FBNIC_SENSOR_NO_DATA?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908225737.3034403-1-limzhineng2@gmail.com?part=4

Re: [PATCH net-next v3 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 promised by the commit
  message is not established on all paths, so userspace can read a bogus…
- [Medium] Unsupported threshold attributes are unconditionally exposed
  in sysfs, violating hwmon is_visible semantics.
- [Low] Unsynchronized access to the new threshold fields.

--

eth: fbnic: report temperature and voltage thresholds via hwmon

The firmware capability response is parsed for per-board temperature
(min/max/crit) and voltage (min/max) thresholds, which are stored in
fbnic_fw_cap and exposed via hwmon as temp1_{min,max,crit} and
in0_{min,max}, with FBNIC_SENSOR_NO_DATA reported as -ENODATA.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index ff1674eff7ad..e5669dfd9790 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);
> +	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);
> +

[Severity: Medium]
Is this the only place the FBNIC_SENSOR_NO_DATA sentinel gets written into
fw_cap.temp/fw_cap.volt?  The commit message says:

  "Values the firmware did not report are stored as the
   FBNIC_SENSOR_NO_DATA sentinel in the capability response parser"

but these assignments sit after all of the parser's error returns, and the
fields start out as 0 rather than the sentinel.  fbnic_mbx_init() does:

drivers/net/ethernet/meta/fbnic/fbnic_fw.c:fbnic_mbx_init() {
	/* Reset FW Capabilities */
	memset(&fbd->fw_cap, 0, sizeof(fbd->fw_cap));
	...
}

so the five new threshold fields are zeroed there, not seeded with
FBNIC_SENSOR_NO_DATA.

Can that window be observed from sysfs?  fbnic_mbx_init() is reached via
fbnic_fw_request_mbx()->__fbnic_fw_enable_mbx()->fbnic_mbx_init(), and
fbnic_fw_request_mbx() is also called post-probe from __fbnic_pm_resume(),
including the fbnic_err_slot_reset() AER recovery path.  Neither
fbnic_pm_suspend() nor fbnic_err_error_detected() calls
fbnic_hwmon_unregister() (only fbnic_remove() does), so
temp1_{min,max,crit} and in0_{min,max} stay readable while
fbnic_mbx_poll_tx_ready() waits for a fresh capability response.  During
that window, does a read return 0 (0 m°C / 0 mV) with success instead of
-ENODATA?

There also looks to be a permanent variant.  fbnic_fw_parse_cap_resp()
commits the version early:

	version = fta_get_uint(results, FBNIC_FW_CAP_RESP_VERSION);
	fbd->fw_cap.running.mgmt.version = version;

and can then return before reaching the assignments above:

		attr = results[FBNIC_FW_CAP_RESP_BMC_MAC_ARRAY];
		if (!attr)
			return -EINVAL;

		err = fbnic_fw_parse_bmc_addrs(fbd->fw_cap.bmc_mac_addr,
					       attr, 4);
		if (err)
			return err;

fbnic_mbx_process_rx_msgs() only warns and hex-dumps on a parser failure, and
because a valid version was already stored, the loop in
fbnic_mbx_poll_tx_ready() terminates with err == 0 and probe continues to
fbnic_hwmon_register().  Would that leave all five thresholds at 0 for the
lifetime of the device, so temp1_max/temp1_crit read as 0 while temp1_input
is positive?

Would it make sense to seed fw_cap.temp/fw_cap.volt with
FBNIC_SENSOR_NO_DATA right after the memset in fbnic_mbx_init(), or to move
these assignments ahead of the parser's early returns?  fbnic_hwmon_register()
already uses that pattern for the read cache:

drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c:fbnic_hwmon_register() {
	/* 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;
	...
}

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index c8e7ff1d36fc..7dc4d705f566 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c

[ ... ]

> @@ -87,14 +77,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]
Should the thresholds really be exposed unconditionally?

fbnic_hwmon_is_visible() still returns 0444 for every (type, attr)
pair, so with the new HWMON_T_MIN/HWMON_T_MAX/HWMON_T_CRIT and
HWMON_I_MIN/HWMON_I_MAX bits in fbnic_hwmon_info[], hwmon creates
temp1_{min,max,crit} and in0_{min,max} even on boards whose firmware
never reported them, and the only signal userspace gets is the
-ENODATA that fbnic_hwmon_read_threshold() returns on the
FBNIC_SENSOR_NO_DATA sentinel.  The commit message states this as the
design ("The thresholds are always exposed"), which is why I'm asking
rather than just flagging it.

The hwmon contract is the other way around: an attribute the device
cannot supply should not be created, and is_visible() returning 0 is
exactly the mechanism for that -- hwmon skips the attribute entirely in
that case.  A file that exists but always fails its read() is worse
than an absent one: libsensors and other consumers surface the read
error instead of quietly skipping the sensor, and every user of
temp1_max now has to special-case -ENODATA.  The drvdata handed to
fbnic_hwmon_is_visible() is the struct fbnic_dev, so it can inspect
fbd->fw_cap.temp.{min,max,crit} and fbd->fw_cap.volt.{min,max} and
return 0 for the sentinel cases without any new plumbing.

Two things I don't know the intended answer to.  First, is_visible() is
only consulted once, when fbnic_hwmon_register() calls
devm_hwmon_device_register_with_info(), so deriving visibility from
fw_cap requires the FW_CAP_RESP to have been parsed before that point
-- is that ordering guaranteed on all probe paths, or would a slow/
absent capability response silently hide the thresholds forever?
Second, fbnic_fw_request_mbx() re-sends the capability request from
__fbnic_pm_resume() and fbnic_err_slot_reset(), so fw_cap can be
re-parsed after registration; if a threshold appears or disappears
across a resume the attribute set is already fixed.  Is the intent to
keep -ENODATA purely as a defensive fallback for that case while
is_visible() reflects the state at registration, or is there a reason
you'd rather always publish the files?

[Severity: Low]
Is any synchronization needed between these reads and the firmware parser
that writes the same fields?

The writer is fbnic_fw_parse_cap_resp(), reached from
fbnic_fw_msix_intr()->fbnic_mbx_poll()->fbnic_mbx_process_rx_msgs(), i.e. the
threaded handler installed by fbnic_fw_request_mbx():

drivers/net/ethernet/meta/fbnic/fbnic_irq.c:fbnic_fw_request_mbx() {
	err = request_threaded_irq(vector, NULL, &fbnic_fw_msix_intr,
				   IRQF_ONESHOT | IRQF_NO_AUTOEN,
				   dev_name(fbd->dev), fbd);
	...
}

The readers above run in process context from the hwmon read callback.
fbd->fw_tx_lock only covers Tx enqueue, and the hwmon core mutex the reader
holds is never taken by the firmware path, so the two contexts share no lock
and there are no READ_ONCE()/WRITE_ONCE() annotations.

The FW_CAP_RESP parser stays registered in fbnic_fw_tlv_parser[] for the
lifetime of the device, and fbnic_fw_request_mbx() re-sends
FBNIC_TLV_MSG_ID_HOST_CAP_REQ from __fbnic_pm_resume() /
fbnic_err_slot_reset(), so a re-parse can land while hwmon is live.

Aligned s32 accesses will not tear, so this looks like a KCSAN-reportable
plain-access race rather than corruption, but can the min/max/crit triple be
read as an inconsistent set, or an attribute flip between a real value and
-ENODATA between two reads?

[ ... ]

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

[PATCH net-next v3 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 7dc4d705f566..11fa8aab0f5b 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
@@ -86,34 +86,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,
@@ -138,10 +188,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

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

[sashiko-bot]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908225737.3034403-1-limzhineng2@gmail.com?part=5

Re: [PATCH net-next v3 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: 1 · Low: 1

- [High] The five new alarm attributes (temp1_min_alarm, temp1_max_alarm,
  temp1_crit_alarm, in0_min_alarm, in0_max_alarm) are synthesized in…
- [Medium] For alarm attributes whose threshold was never populated by
  firmware (fbd->fw_cap.temp.{min,max,crit} / volt.{min,max} ==…
- [Low] The number of hwmon attributes whose read triggers a TSENE
  firmware mailbox round-trip grows from 2 (temp1_input, in0_input) to 7…

--

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 device.  Each alarm is derived by taking a live TSENE sensor reading
and comparing it against the matching threshold stored in fbnic_fw_cap.
Thresholds that firmware did not populate are meant to report -ENODATA.

A few questions on the new alarm attributes below.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index 7dc4d705f566..11fa8aab0f5b 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> @@ -86,34 +86,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 deriving the alarm bits in software compatible with the documented hwmon
alarm semantics?  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() is a stateless comparison of one live sample
against a stored threshold, so nothing is latched.  If the temperature
crosses fbd->fw_cap.temp.crit and falls back before the next read(), does
temp1_crit_alarm read back 0 and lose the excursion entirely?

Looking at the firmware side, the TSENE response only carries the
instantaneous values plus an error code:

drivers/net/ethernet/meta/fbnic/fbnic_fw.c
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),
	...

so there appear to be no alarm/status bits available to report.  Would it
make sense to base these files on latched state fed by the firmware
threshold-exceeded events instead, or to leave the attributes out until
such state exists?

This also seems to interact with the last patch of the series ("eth: fbnic:
firmware notifies hwmon on sensor threshold events"): can that path call
hwmon_notify_event() for temp1_crit_alarm while a subsequent read of the
same file returns 0 because the fresh sample is back under the threshold?

>  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 the firmware round-trip?

The alarm cases break out of the first switch without looking at
fbd->fw_cap.temp.{min,max,crit}, so a permanently unsupported alarm still
enters fbnic_hwmon_sensor_read(), which can block:

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

Older firmware that omits FBNIC_FW_CAP_RESP_TEMP_{MIN,MAX,CRIT} leaves the
sentinel in place:

drivers/net/ethernet/meta/fbnic/fbnic_fw.c:fbnic_fw_parse_cap_resp() {
	...
	fbd->fw_cap.temp.min =
		fbnic_tlv_attr_get_signed(results[FBNIC_FW_CAP_RESP_TEMP_MIN],
					  FBNIC_SENSOR_NO_DATA);
	...
}

In that case, when the mailbox transaction fails, does the attribute report
-ENOMEM/-ETIMEDOUT (with a dev_err() console line) rather than the
-ENODATA promised by the commit message text "A threshold the firmware did
not populate reports -ENODATA for both the threshold attribute and its
alarm"?  The sentinel check looks order independent and could be folded
into the first switch, which would also avoid the discarded firmware read.
The same ordering applies to in0_min_alarm and in0_max_alarm in
fbnic_hwmon_in_read().

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

[ ... ]

> +
> +	err = fbnic_hwmon_sensor_read(fbd, FBNIC_SENSOR_VOLTAGE, val);
> +	if (err)
> +		return err;

[Severity: Low]
The commit message says:

    only attributes that actually need a live value fall through to a single
    sensor read that then feeds input and every alarm

Is that accurate across attributes?  hwmon_attr_show() in
drivers/hwmon/hwmon.c invokes ->read() once per attribute file, so there is
no shared read; the only coalescing is the exact-tick comparison in
fbnic_hwmon_sensor_read():

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

With that, does a sequential sweep of the channel (sensors, or a monitoring
daemon) issue up to seven FBNIC_TLV_MSG_ID_TSENE_READ_REQ transactions
where the pre-patch driver issued two, each serialized and able to wait up
to 10 s?  Could the commit message be reworded, or the cache given a
coarser validity window than a single jiffy?

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

[ ... ]

> @@ -138,10 +188,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
>  };

These five entries create the user-visible alarm files, and their semantics
become part of the ABI once merged, which is why the question above about
latched versus computed alarm state seems worth settling first.

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

[PATCH net-next v3 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 e5669dfd9790..fd96a3962e3f 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 11fa8aab0f5b..2dff59bc0e8b 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
@@ -206,6 +206,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;
 
@@ -213,22 +215,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: [PATCH net-next v3 6/6] eth: fbnic: firmware notifies hwmon on sensor threshold events

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing memory release semantics when publishing the `hwmon` pointer allows concurrent IRQ handler to observe uninitialized memory.
- [Medium] Threshold comparison does not handle FBNIC_SENSOR_NO_DATA for missing firmware capabilities, causing incorrect hwmon alarms.
--

commit 5008b411c2a8813d473926829690169559798a63
Author: Zinc Lim <limzhineng2@gmail.com>

eth: fbnic: firmware notifies hwmon on sensor threshold events
    
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>

[ ... ]

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index 11fa8aab0f5b2..2dff59bc0e8ba 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> @@ -206,6 +206,8 @@ static const struct hwmon_chip_info fbnic_chip_info = {
>  
>  void fbnic_hwmon_register(struct fbnic_dev *fbd)
>  {
[ ... ]
> -	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;
>  	}
> +
> +	WRITE_ONCE(fbd->hwmon, hwmon);

[Severity: Medium]
Could this WRITE_ONCE() allow an active IRQ handler to observe an
incompletely initialized hwmon device pointer?

The FW mailbox IRQ could already be active when fbnic_hwmon_register() is
called. If an unsolicited threshold event arrives right after this
WRITE_ONCE() executes, but before the memory stores that initialize the
hwmon device are globally visible, the IRQ handler might read the pointer
and access uninitialized fields.

Since WRITE_ONCE() provides atomicity but lacks memory release semantics, on
weakly ordered architectures the CPU is permitted to let the pointer store
float past the one-way unlock barrier inside the registration function.
Would it be safer to use smp_store_release() here (and smp_load_acquire()
in the reader) to ensure lockless publication rules are met?

>  }
>  
>  void fbnic_hwmon_unregister(struct fbnic_dev *fbd)
>  {
[ ... ]
> +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;

[Severity: Medium]
Does this threshold comparison handle missing firmware capabilities properly?

If the firmware does not support a specific threshold (e.g., temp.crit), the
corresponding capability field is initialized to FBNIC_SENSOR_NO_DATA,
which expands to S32_MIN (-2147483648).

When a threshold event arrives for a valid threshold (like temp.max), these
sequential checks evaluate the missing threshold first. Any valid positive
temperature will evaluate (val >= fbd->fw_cap.temp.crit) as true since it
is comparing against S32_MIN.

Could this trigger false alarms and shadow lower-priority valid thresholds?
Should the logic validate against FBNIC_SENSOR_NO_DATA before performing
these comparisons?

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

[Severity: Medium]
Does the voltage threshold logic suffer from the same issue where
unsupported limits could trigger false alarms?

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

[Severity: Medium]
To pair with the publication in fbnic_hwmon_register(), should this be an
smp_load_acquire() to prevent reading the pointer before its contents are
fully initialized?

> +	if (attr >= 0 && hwmon)
> +		hwmon_notify_event(hwmon, type, attr, 0);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908225737.3034403-1-limzhineng2@gmail.com?part=6

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

[netdev-bot+sashiko]

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

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

- [High] fbnic_hwmon_notify_event()
  (drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c:248-288) classifies the…
- [Medium] fbnic_hwmon_unregister() open-codes its teardown drain as
  'WRITE_ONCE(fbd->hwmon, NULL); synchronize_irq(fbd->fw_msix_vector);'…
- [Low] fbnic_hwmon_is_visible() returns 0444 unconditionally
  (fbnic_hwmon.c:10-15) and fbnic_hwmon_info[] advertises every…
- [Low] The newly registered hwmon device pointer is published to the
  concurrently running firmware mailbox IRQ thread with a plain…

Pre-existing issues:
- [Medium] A malformed firmware mailbox message can cause
  uninitialized/stale page contents to be dumped into the kernel log.

--

eth: fbnic: firmware notifies hwmon on sensor threshold events

This adds a parser for the unsolicited
FBNIC_TLV_MSG_ID_SENSOR_THRESHOLD_EXCEEDED_RESP firmware message and
translates it into a hwmon alarm notification through
hwmon_notify_event().  It also reworks publish/teardown of fbd->hwmon with
WRITE_ONCE()/READ_ONCE() plus a synchronize_irq() drain in the unregister
path.

No Link: tags present in the commit header.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index e5669dfd9790..fd96a3962e3f 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;
> +	}

[Severity: Medium]

This isn't a problem introduced by this patch, but these new -EINVAL
returns add fresh ways to reach the malformed-message hex dump in
fbnic_mbx_process_rx_msgs(), which can print bytes the firmware never
wrote.

RX mailbox pages are not zeroed:

drivers/net/ethernet/meta/fbnic/fbnic_fw.c:fbnic_mbx_alloc_rx_msgs() {
	...
		msg = (struct fbnic_tlv_msg *)__get_free_page(GFP_KERNEL);
	...
}

and the error path dumps the full firmware-supplied descriptor length
rather than the validated TLV extent:

drivers/net/ethernet/meta/fbnic/fbnic_fw.c:fbnic_mbx_process_rx_msgs() {
	...
			print_hex_dump(KERN_WARNING, "fbnic:",
				       DUMP_PREFIX_OFFSET, 16, 2,
				       msg, length, true);
	...
}

Can this print stale page contents into the kernel log when firmware
advertises a large descriptor length but writes only a short malformed
threshold message?  Would bounding the dump to the parsed TLV length, or
allocating the RX pages with __GFP_ZERO, be preferable?

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

[ ... ]

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index 11fa8aab0f5b..2dff59bc0e8b 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c

[ ... ]

> @@ -213,22 +215,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 WRITE_ONCE() strong enough for publishing this pointer?  The FW mailbox
threaded IRQ is requested in fbnic_probe() before fbnic_hwmon_register()
runs, so fbnic_hwmon_notify_event() can load fbd->hwmon on another CPU and
pass it straight to hwmon_notify_event(), which touches dev->kobj.

WRITE_ONCE()/READ_ONCE() prevent tearing but carry no release/acquire
ordering, so on arm64 or ppc64 the store publishing fbd->hwmon is not
ordered after the stores that initialized the device inside
hwmon_device_register_with_info().  Would smp_store_release() here paired
with smp_load_acquire() in the notify path express the intended ordering?

>  }
>  
>  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: Medium]

Is fbd->fw_msix_vector guaranteed to still name an IRQ owned by this
driver when fbnic_hwmon_unregister() runs?

fbnic_pm_suspend() tears the mailbox IRQ down while hwmon stays
registered:

drivers/net/ethernet/meta/fbnic/fbnic_irq.c:fbnic_fw_free_mbx() {
	...
	free_irq(fbd->fw_msix_vector, fbd);
	fbd->fw_msix_vector = 0;
}

fbnic_hwmon_unregister() is only called from fbnic_remove(), so if
__fbnic_pm_resume() then fails at fbnic_alloc_irqs() the driver stays bound
with fbd->hwmon != NULL and fw_msix_vector == 0.  A later unbind would pass
the !fbd->hwmon guard and call synchronize_irq(0); irq_to_desc(0) is
non-NULL on x86, so this waits on a descriptor owned by another device.

There is a second variant on the request side:

drivers/net/ethernet/meta/fbnic/fbnic_irq.c:fbnic_fw_request_mbx() {
	...
	err = __fbnic_fw_enable_mbx(fbd, vector);
	if (err)
		free_irq(vector, fbd);

	fbd->fw_msix_vector = vector;
	...
}

The vector number is stored even when the IRQ was freed, so after a
resume-time failure and pci_free_irq_vectors() the stored number can be
stale and may have been handed to another driver.

Since fbnic_remove() already does:

drivers/net/ethernet/meta/fbnic/fbnic_pci.c:fbnic_remove() {
	...
	fbnic_hwmon_unregister(fbd);
	fbnic_dbg_fbd_exit(fbd);
	fbnic_devlink_unregister(fbd);
	fbnic_fw_log_disable(fbd);
	fbnic_fw_free_mbx(fbd);
	...
}

would moving fbnic_hwmon_unregister() after fbnic_fw_free_mbx() give the
same barrier without keying it on a mutable vector number?

> +
> +	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: High]

Do these comparisons handle the case where firmware never advertised a
threshold?  fbnic_fw_parse_cap_resp() stores the FBNIC_SENSOR_NO_DATA
sentinel for any threshold the capability response omits:

drivers/net/ethernet/meta/fbnic/fbnic_fw.c:fbnic_fw_parse_cap_resp() {
	...
	fbd->fw_cap.temp.crit =
		fbnic_tlv_attr_get_signed(results[FBNIC_FW_CAP_RESP_TEMP_CRIT],
					  FBNIC_SENSOR_NO_DATA);
	...
}

and that sentinel is the smallest s32:

drivers/net/ethernet/meta/fbnic/fbnic_fw.h:
#define FBNIC_SENSOR_NO_DATA			S32_MIN

So with temp.crit unset, "val >= fbd->fw_cap.temp.crit" is always true and
every threshold event is reported as temp1_crit_alarm.  Does that also make
the temp1_max_alarm arm below unreachable, since the crit test runs first
even when temp.max is a real value?  The voltage case looks the same:
an unset volt.max turns any reported value into in0_max_alarm.

The read paths filter the sentinel explicitly:

drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c:fbnic_hwmon_read_alarm() {
	if (a == FBNIC_SENSOR_NO_DATA || b == FBNIC_SENSOR_NO_DATA)
		return -ENODATA;
	...
}

which means userspace gets woken on an attribute that then reads back as
-ENODATA.  Should the notify path skip thresholds equal to
FBNIC_SENSOR_NO_DATA the same way?

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

[Severity: Low]

This isn't a bug in the new notify path itself, but related to the
attributes it notifies on: fbnic_hwmon_is_visible() returns 0444
unconditionally and fbnic_hwmon_info[] advertises every threshold and
alarm:

drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c:
	HWMON_CHANNEL_INFO(in,
			   HWMON_I_INPUT |
			   HWMON_I_MIN | HWMON_I_MIN_ALARM |
			   HWMON_I_MAX | HWMON_I_MAX_ALARM),

so temp1_{min,max,crit}, in0_{min,max} and the matching *_alarm files exist
even when the capability response omitted the threshold and
fbnic_hwmon_read_threshold() can only ever return -ENODATA.

Documentation/hwmon/sysfs-interface.rst says:

  All entries (except name) are optional, and should only be created in a
  given driver if the chip has the feature.

Could fbnic_hwmon_is_visible() key visibility off the corresponding
capability value, and hide an alarm when its threshold is unavailable?

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