Re: [PATCH net-next v4 3/6] eth: fbnic: report temperature and voltage thresholds via hwmon

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] fbnic: data race in lockless hwmon threshold reads
--

commit 9e534754533bccab7e48537807cfa9b4798362f0
Author: Zinc Lim <limzhineng2@gmail.com>

    eth: fbnic: report temperature and voltage thresholds via hwmon
    
    The firmware capability response carries per-board temperature
    (min/max/crit) and voltage (min/max) thresholds. Parse and store them in
    fbnic_fw_cap, and expose them through the hwmon interface as
    temp1_{min,max,crit} and in0_{min,max}.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index 82307f212f583..d9a00a2ef34da 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
[ ... ]
> @@ -95,15 +95,25 @@ static void fbnic_mbx_reset_desc_ring(struct fbnic_dev *fbd, int mbx_idx)
>  void fbnic_mbx_init(struct fbnic_dev *fbd)
>  {
> +	struct fbnic_fw_cap fw_cap_reset;
>  	int i;
>  
>  	/* Initialize lock to protect Tx ring */
>  	spin_lock_init(&fbd->fw_tx_lock);
>  
> -	/* Reset FW Capabilities */
> -	memset(&fbd->fw_cap, 0, sizeof(fbd->fw_cap));
> +	/* Reset FW Capabilities and seed thresholds with sentinel */
> +	memset(&fw_cap_reset, 0, sizeof(fw_cap_reset));
> +	fw_cap_reset.temp = fbnic_threshold_no_data;
> +	fw_cap_reset.volt = fbnic_threshold_no_data;
> +	fbd->fw_cap = fw_cap_reset;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: Medium]
Could this structure assignment result in a data race?

Since fbnic_mbx_init() is invoked during AER recovery, it performs a
non-atomic overwrite of the capability structure. The hwmon device stays
registered, and user-space tasks are not frozen, meaning concurrent sysfs
reads of the thresholds might read partially updated or garbage values.

[ ... ]
> @@ -742,6 +757,22 @@ static int fbnic_fw_parse_cap_resp(void *opaque, struct fbnic_tlv_msg **results)
>  	fbd->fw_cap.anti_rollback_version =
>  		fta_get_uint(results, FBNIC_FW_CAP_RESP_ANTI_ROLLBACK_VERSION);
>  
> +	fbd->fw_cap.temp.min =
> +		fbnic_tlv_attr_get_signed(results[FBNIC_FW_CAP_RESP_TEMP_MIN],
> +					  FBNIC_SENSOR_NO_DATA);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: Medium]
Similarly, does this lockless update in the NAPI/IRQ handler risk tearing
if it races with lockless hwmon sysfs reads?

> +	fbd->fw_cap.temp.max =
> +		fbnic_tlv_attr_get_signed(results[FBNIC_FW_CAP_RESP_TEMP_MAX],
> +					  FBNIC_SENSOR_NO_DATA);
[ ... ]
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index 0c3fe2a372275..4df808f850cf8 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
[ ... ]
> @@ -92,14 +111,58 @@ static int fbnic_hwmon_sensor_read(struct fbnic_dev *fbd, int id, long *val)
>  	return err;
>  }
>  
> +static int fbnic_hwmon_read_threshold(long thr, long *val)
> +{
> +	if (thr == FBNIC_SENSOR_NO_DATA)
> +		return -ENODATA;
> +
> +	*val = thr;
> +	return 0;
> +}
> +
> +static int fbnic_hwmon_temp_read(struct fbnic_dev *fbd, u32 attr, long *val)
> +{
> +	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);
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: Medium]
Could this lockless read return an inconsistent value to userspace if it
executes concurrently with the struct assignment in fbnic_mbx_init() or
the updates in fbnic_fw_parse_cap_resp()?

> +	case hwmon_temp_max:
> +		return fbnic_hwmon_read_threshold(fbd->fw_cap.temp.max, val);

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