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

[netdev-bot+sashiko@kernel.org]

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

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

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

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

--

eth: fbnic: report temperature and voltage alarms via hwmon

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

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

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

[ ... ]

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

[ ... ]

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

[ ... ]

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

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

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

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

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

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

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

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

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

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

[Severity: Medium]
Is this fallback allowed by the hwmon ABI? Documentation/hwmon/sysfs-interface.rst
says:

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

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

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

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

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

[Severity: Medium]
The commit message says:

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

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

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

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

Take this sequence:

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

The second read also returns 0.

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

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

> +
> +	return 0;
> +}

[ ... ]

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

[ ... ]

> +	hwmon = hwmon_device_register_with_info(fbd->dev, "fbnic",
> +						fbd, &fbnic_chip_info,
> +						NULL);
> +	if (IS_ERR(hwmon)) {
>  		dev_notice(fbd->dev,
>  			   "Failed to register hwmon device %pe\n",
> -			   fbd->hwmon);
> -		fbd->hwmon = NULL;
> +			   hwmon);
> +		return;
>  	}
> +
> +	WRITE_ONCE(fbd->hwmon, hwmon);
>  }

[Severity: Medium]
Is WRITE_ONCE() enough to publish the device to the mailbox IRQ thread?

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

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

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

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

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

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

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

[Severity: Low]
Can fw_msix_vector be 0, or stale, by the time this runs?

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

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

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

	if (err)
		free_irq(vector, fbd);

	fbd->fw_msix_vector = vector;

fbnic_free_irqs() then releases the PCI vectors.

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

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

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

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

[ ... ]

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

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

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

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

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

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

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

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

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

>  }

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