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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Unpopulated alarm attributes trigger unnecessary firmware reads instead of being rejected up front.
- [Medium] Hardware monitoring alarms are improperly computed in software by the driver.
--

--- Patch [5]: [PATCH net-next 5/6] eth: fbnic: report temperature and voltage alarms via hwmon ---
commit 6d735c75df1c02d16911aecd205cd4bf98b0bb3b
Author: Zinc Lim <[email protected]>

eth: fbnic: report temperature and voltage alarms via hwmon

Building on the temperature and voltage thresholds stored in
fbnic_fw_cap, expose alarm attributes through the hwmon interface:
temp1_{min,max,crit}_alarm and in0_{min,max}_alarm.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index 4938f7b391402..c5cddd9cef124 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c

[ ... ]

> @@ -87,34 +87,84 @@ static int fbnic_hwmon_read_threshold(long thr, long *val)
>  	return 0;
>  }
>  
> +static int fbnic_hwmon_read_alarm(long a, long b, long *val)
> +{
> +	if (a == FBNIC_SENSOR_NO_DATA || b == FBNIC_SENSOR_NO_DATA)
> +		return -ENODATA;
> +
> +	*val = a >= b;

[Severity: Medium]
Is it intentional to compute alarms in software by comparing the live
sensor reading against the threshold here?

According to the hwmon sysfs ABI documentation, drivers should not make
comparisons of readings to thresholds themselves. Alarms are expected to be
hardware-latched indications that can catch transient spikes between polls.

If the hardware does not support latched alarms, should these alarm
attributes be omitted entirely so user-space tools can handle their own
software threshold comparisons?

> +	return 0;
> +}
> +
>  static int fbnic_hwmon_temp_read(struct fbnic_dev *fbd, u32 attr, long *val)
>  {
> +	int err;
> +
>  	switch (attr) {
> -	case hwmon_temp_input:
> -		return fbnic_hwmon_sensor_read(fbd, FBNIC_SENSOR_TEMP, val);
>  	case hwmon_temp_min:
>  		return fbnic_hwmon_read_threshold(fbd->fw_cap.temp.min, val);
>  	case hwmon_temp_max:
>  		return fbnic_hwmon_read_threshold(fbd->fw_cap.temp.max, val);
>  	case hwmon_temp_crit:
>  		return fbnic_hwmon_read_threshold(fbd->fw_cap.temp.crit, val);
> +	case hwmon_temp_input:
> +	case hwmon_temp_min_alarm:
> +	case hwmon_temp_max_alarm:
> +	case hwmon_temp_crit_alarm:
> +		break;
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> +
> +	err = fbnic_hwmon_sensor_read(fbd, FBNIC_SENSOR_TEMP, val);

[Severity: Medium]
The commit message notes that unsupported attributes are rejected up front,
but does this code fall through for unpopulated alarm thresholds and trigger
a blocking firmware read before eventually returning -ENODATA?

If fbd->fw_cap.temp.min is FBNIC_SENSOR_NO_DATA, reading
hwmon_temp_min_alarm will hit the break statement above, execute this
sensor read, and then fail later inside fbnic_hwmon_read_alarm().

Could we check the threshold value up front before issuing the mailbox
request?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5