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

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

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

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

--

eth: fbnic: report temperature and voltage alarms via hwmon

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

fbnic_hwmon_is_visible() returns 0444 unconditionally:

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

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

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

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