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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Uninitialized sensor thresholds default to S32_MIN, causing alarm checks to always evaluate to true and trigger incorrect alarms.
- [Low] Missing IS_REACHABLE(CONFIG_HWMON) guard in fbnic_hwmon_notify_event causes a linker error when CONFIG_HWMON is disabled.

Pre-existing issues:
- [High] Error path in fbnic_fw_request_mbx saves a freed IRQ vector, leading to a double-free on device removal.
--

--- Patch [6]: [PATCH net-next 6/6] eth: fbnic: firmware notifies hwmon on sensor threshold events ---
commit 47c1b3bcb6116f975f3ccbd306542f2c5e9bd4f2
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.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index c5cddd9cef124..eb910ba47f378 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
[ ... ]
> @@ -214,22 +216,72 @@ void fbnic_hwmon_register(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;
> +
> +	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;

[Severity: Medium]
Will this evaluate to true if the firmware does not provide a critical
threshold?

It appears that fbnic_fw_parse_cap_resp() initializes these thresholds to the
sentinel value FBNIC_SENSOR_NO_DATA (S32_MIN) when they aren't provided. 

Since S32_MIN is the minimum possible 32-bit integer, any valid temperature
reading will be greater than S32_MIN, causing this condition to incorrectly
evaluate to true and preventing subsequent valid threshold checks from being
evaluated.

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

[Severity: Low]
Does this result in a linker error if CONFIG_HWMON is disabled?

Since fbnic_hwmon_notify_event() is built unconditionally when CONFIG_FBNIC
is enabled, and hwmon_notify_event() is declared in include/linux/hwmon.h
without a fallback stub for !CONFIG_HWMON, calling it without an
IS_REACHABLE(CONFIG_HWMON) guard will cause the linker to fail to resolve
hwmon_notify_event().

>  }

[Severity: High]
This is a pre-existing issue, but does the error path in
fbnic_fw_request_mbx() save a freed IRQ vector?

drivers/net/ethernet/meta/fbnic/fbnic_irq.c:fbnic_fw_request_mbx() {
    ...
    if (err)
        free_irq(vector, fbd);

    fbd->fw_msix_vector = vector;
    ...
}

If a device resumes from suspend and this path fails, fbd->fw_msix_vector
is left holding the freed vector number. 

Later, when the device is removed, fbnic_remove() invokes fbnic_fw_free_mbx().
Since fbd->fw_msix_vector is non-zero, it skips the safety check and attempts
to free the already freed vector, leading to a double-free.

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