Re: [PATCH net-next v4 4/6] eth: fbnic: parse the alarm range reported by firmware

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The firmware alarm range is a U32 TLV attribute, but
  fbnic_fw_parse_tsene_read_resp() and…

--

eth: fbnic: parse the alarm range reported by firmware

This commit parses the per-sensor alarm range that firmware reports in
two places: the TSENE read response and a new unsolicited
SENSOR_THRESHOLD_EXCEEDED event. It caches the range in u8 fields in
hwmon_cache. Both fields are seeded with FBNIC_FW_TSENE_ALARM_UNKNOWN
in fbnic_mbx_init().

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic.h b/drivers/net/ethernet/meta/fbnic/fbnic.h
> index 857a64b518f38..93bf7004b67d9 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic.h
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic.h
> @@ -26,6 +26,8 @@ struct fbnic_hwmon_cache {
>  	unsigned long last_read;
>  	s32 temp_mdeg;
>  	s32 volt_mv;
> +	u8 temp_alarm;
> +	u8 volt_alarm;
>  };

[ ... ]

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index d9a00a2ef34da..7863f6ba1e7da 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c

[ ... ]

> @@ -1466,6 +1468,8 @@ static const struct fbnic_tlv_index fbnic_tsene_read_resp_index[] = {
>  	FBNIC_TLV_ATTR_S32(FBNIC_FW_TSENE_THERM),
>  	FBNIC_TLV_ATTR_S32(FBNIC_FW_TSENE_VOLT),
>  	FBNIC_TLV_ATTR_S32(FBNIC_FW_TSENE_ERROR),
> +	FBNIC_TLV_ATTR_U32(FBNIC_FW_TSENE_THERM_ALARM_STATE),
> +	FBNIC_TLV_ATTR_U32(FBNIC_FW_TSENE_VOLT_ALARM_STATE),
>  	FBNIC_TLV_ATTR_LAST
>  };

[ ... ]

> @@ -1497,6 +1502,20 @@ static int fbnic_fw_parse_tsene_read_resp(void *opaque,
>  	cmpl_data->u.tsene.millivolts =
>  		fta_get_sint(results, FBNIC_FW_TSENE_VOLT);
>  
> +	/* Cache here so that it and the threshold event land in the order firmware
> +	 * sent them. Firmware predating alarm state reporting sends neither
> +	 * attribute.
> +	 */
> +	alarm = fbnic_tlv_attr_get_unsigned(results[FBNIC_FW_TSENE_THERM_ALARM_STATE],
> +					    FBNIC_FW_TSENE_ALARM_UNKNOWN);
> +	if (alarm != FBNIC_FW_TSENE_ALARM_UNKNOWN)
> +		WRITE_ONCE(fbd->hwmon_cache.temp_alarm, alarm);
> +
> +	alarm = fbnic_tlv_attr_get_unsigned(results[FBNIC_FW_TSENE_VOLT_ALARM_STATE],
> +					    FBNIC_FW_TSENE_ALARM_UNKNOWN);
> +	if (alarm != FBNIC_FW_TSENE_ALARM_UNKNOWN)
> +		WRITE_ONCE(fbd->hwmon_cache.volt_alarm, alarm);

[Severity: Low]

The alarm state is parsed as a U32 attribute but stored in the u8
fields hwmon_cache.temp_alarm and volt_alarm. The only check here is
against FBNIC_FW_TSENE_ALARM_UNKNOWN, and it runs on the u32 before
the value is narrowed.

What happens if firmware sends a value that does not fit in a u8? 0x100
would be cached as 0 (FBNIC_FW_TSENE_ALARM_NORMAL), 0x101 as
FBNIC_FW_TSENE_ALARM_DEPRECATED, and 0x1ff as
FBNIC_FW_TSENE_ALARM_UNKNOWN.

fbnic_fw_parse_threshold_exceeded_resp() has the same pattern. There,
only DEPRECATED is filtered before the store:

		if (alarm != FBNIC_FW_TSENE_ALARM_DEPRECATED)
			WRITE_ONCE(fbd->hwmon_cache.temp_alarm, alarm);

The volt_alarm store in that function works the same way.

This only happens if firmware sends a value outside its documented set
(NORMAL, BELOW_MIN, ABOVE_MAX, CRIT). The worst case looks like a
wrong hwmon alarm value, since the cached byte is only compared and
never used as an index.

Should the value be range checked before it is narrowed into the u8
cache in both parsers?

>  msg_err:
>  	cmpl_data->result = err_resp ? : err;
>  	complete(&cmpl_data->done);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014222225.2803660-1-limzhineng2%40gmail.com