Re: [PATCH net-next v2 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:
- [High] Unpopulated firmware thresholds (FBNIC_SENSOR_NO_DATA / S32_MIN) are incorrectly evaluated as crossed, causing wrong alarms to be attributed.
- [Medium] Calling synchronize_irq() with an uninitialized or freed IRQ vector (0) can block on unrelated system interrupts.
--

commit 29bf1cd49123c450c788d4c7179ddacd012338e7
Author: Zinc Lim <limzhineng2@gmail.com>

eth: fbnic: firmware notifies hwmon on sensor threshold events

This patch handles firmware notifications for sensor threshold events, parsing
the IPC message and mapping it to the corresponding hwmon alarm. It also adds
concurrency guards to the hwmon registration and teardown paths.

[ ... ]
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index c5cddd9cef124..3f33522fcfd2b 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
[ ... ]
> @@ -214,22 +216,75 @@ void fbnic_hwmon_register(struct fbnic_dev *fbd)
>  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: Medium]
Could this call block on unrelated system interrupts if the vector is 0?

During device suspend, fbnic_pm_suspend() frees the mailbox IRQ and sets
fbd->fw_msix_vector = 0. If the device is subsequently removed (e.g., via
hotplug or driver unload while suspended), fbnic_remove() calls
fbnic_hwmon_unregister() which then unconditionally calls
synchronize_irq(0).

On architectures like x86, IRQ 0 is a valid system interrupt (such as the
PIT timer). Will this cause the driver to pointlessly block waiting for
the timer interrupt handler to finish?

> +
> +	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;

[Severity: High]
Does this logic correctly handle unpopulated firmware thresholds?

If a limit such as fbd->fw_cap.temp.crit is unpopulated, it is stored as
FBNIC_SENSOR_NO_DATA (S32_MIN). Since any valid sensor reading will be
greater than S32_MIN, the condition (val >= fbd->fw_cap.temp.crit) will
evaluate to true.

Can this falsely trigger hwmon_temp_crit_alarm for normal sensor values
and mask other valid alarms like hwmon_temp_max_alarm? Should we verify
that the threshold is valid (!= FBNIC_SENSOR_NO_DATA) before comparing it
against the sensor reading?

> +
> +		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: High]
Does the same issue apply here for missing voltage maximum thresholds?

If fbd->fw_cap.volt.max is FBNIC_SENSOR_NO_DATA (S32_MIN), will this
incorrectly trigger an alarm for any valid voltage reading?

> +
> +		break;
> +	default:
> +		return;
> +	}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831212234.1212971-1-limzhineng2@gmail.com?part=6