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] Unsupported sensor thresholds (FBNIC_SENSOR_NO_DATA) default to S32_MIN, causing incorrect critical/maximum hardware monitoring alarms.
- [Medium] A malformed thermal payload drops valid voltage exceedance notifications.
--

--- Patch [6]: [PATCH net-next v2 6/6] eth: fbnic: firmware notifies hwmon on sensor threshold events ---
commit cb433d6b1a4ff2dfdeefaa87867490a37344d0ed
Author: Zinc Lim <limzhineng2@gmail.com>

eth: fbnic: firmware notifies hwmon on sensor threshold events
    
This commit adds parsing for firmware unsolicited messages about sensor
threshold events (FBNIC_TLV_MSG_ID_SENSOR_THRESHOLD_EXCEEDED_RESP). It
translates these messages into hwmon alarms and notifies userspace listeners
while guarding against concurrent hwmon registration and teardown.

> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c

[ ... ]

> +static int fbnic_fw_parse_threshold_exceeded_resp(void *opaque,
> +						  struct fbnic_tlv_msg **results)
> +{
> +	bool therm_exceeded, volt_exceeded;
> +	struct fbnic_dev *fbd = opaque;
> +	s32 value;
> +
> +	therm_exceeded =
> +		fta_get_sint(results, FBNIC_FW_TSENE_THERM_EXCEEDED_FLAG);
> +	volt_exceeded =
> +		fta_get_sint(results, FBNIC_FW_TSENE_VOLT_EXCEEDED_FLAG);
> +
> +	if (!therm_exceeded && !volt_exceeded) {
> +		dev_err(fbd->dev,
> +			"Threshold exceeded message with no flag set\n");
> +		return -EINVAL;
> +	}
> +
> +	if (therm_exceeded) {
> +		if (!results[FBNIC_FW_TSENE_THERMAL]) {
> +			dev_err(fbd->dev,
> +				"Thermal threshold exceeded but no value received\n");
> +			return -EINVAL;
> +		}

[Severity: Medium]
If a firmware message arrives with both therm_exceeded and volt_exceeded 
flags set, but the thermal attribute is missing, fbnic_fw_parse_threshold_exceeded_resp()
returns -EINVAL early.

Will this cause a valid voltage exceedance notification to be silently dropped?

> +		value = fta_get_sint(results, FBNIC_FW_TSENE_THERMAL);
> +		dev_err(fbd->dev, "Thermal threshold exceeded: %d mC\n", value);
> +		fbnic_hwmon_notify_event(fbd, FBNIC_SENSOR_TEMP, value);
> +	}
> +
> +	if (volt_exceeded) {
> +		if (!results[FBNIC_FW_TSENE_VOLTAGE]) {
> +			dev_err(fbd->dev,
> +				"Voltage threshold exceeded but no value received\n");
> +			return -EINVAL;
> +		}

[ ... ]

> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c

[ ... ]

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

[Severity: High]
If a sensor threshold is not supported by the hardware, it defaults to 
FBNIC_SENSOR_NO_DATA, which evaluates to S32_MIN.

Since any valid positive temperature value will be greater than S32_MIN,
will this unconditionally trigger a false critical alarm when evaluating 
against an unsupported threshold in fbnic_hwmon_notify_event()?

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

[ ... ]

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