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

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.

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

[ ... ]

[ ... ]

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

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

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

[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