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

[sashiko-bot@kernel.org]

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