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

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

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

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

--

eth: fbnic: firmware notifies hwmon on sensor threshold events

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

A few questions below.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

fbnic_fw_request_mbx() also leaves a stale value behind:

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

	fbd->fw_msix_vector = vector;

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

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

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

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

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

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

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

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

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

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

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

fbnic_hwmon_temp_read() derives them from separate comparisons:

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

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

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

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

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