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

[netdev-bot+sashiko@kernel.org]

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] fbnic_hwmon_is_visible() decides whether temp1_{min,max,crit} and
  in0_{min,max} exist based on fbd->fw_cap.temp/volt.
- [Low] In fbnic_fw_parse_cap_resp(), the new threshold assignments come
  after fbnic_fw_parse_bmc_cap(), and that call returns early on error.
- [Low] The new threshold fields fbd->fw_cap.temp.{min,max,crit} and
  fbd->fw_cap.volt.{min,max} are read and written concurrently with…

--

eth: fbnic: report temperature and voltage thresholds via hwmon

This patch parses temperature (min/max/crit) and voltage (min/max)
thresholds from the firmware capability response and stores them in
fbnic_fw_cap. It then exposes them through hwmon as temp1_{min,max,crit}
and in0_{min,max}. Fields that firmware does not report are seeded with
FBNIC_SENSOR_NO_DATA and are hidden by is_visible.

> 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

[ ... ]

> @@ -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: Low]
Should these threshold assignments come before the BMC parsing step? Just
above them in fbnic_fw_parse_cap_resp() there is an early return:

	err = fbnic_fw_parse_bmc_cap(fbd, results);
	if (err)
		return err;

If the response carries a malformed BMC MAC array, for example a bad
attribute id, a bad length, or more than 4 entries,
fbnic_fw_parse_bmc_addrs()->fbnic_tlv_attr_parse_array() returns -EINVAL
or -ENOSPC. The thresholds are then never stored and stay at the
FBNIC_SENSOR_NO_DATA value seeded by fbnic_mbx_init().

Probe still goes ahead in that case. mgmt.version was already stored at
the top of the function, so fbnic_mbx_poll_tx_ready() leaves its loop
here:

	if (fbd->fw_cap.running.mgmt.version >= MIN_FW_VER_CODE)
		break;

fbnic_probe() then calls fbnic_hwmon_register(). is_visible sees NO_DATA,
so none of the threshold attributes are created for the life of the hwmon
device. In the final series, temp1_alarm and in0_alarm are hidden too.

The commit message says fbnic_fw_parse_cap_resp() "can return before it
reaches the threshold assignments", but only to explain the sentinel.
Would it be better to parse the thresholds before the fallible BMC step,
so valid limits are not lost because an unrelated field failed to parse?

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

[ ... ]

>  static umode_t fbnic_hwmon_is_visible(const void *drvdata,
>  				      enum hwmon_sensor_types type,
>  				      u32 attr, int channel)
>  {
> -	if (type == hwmon_temp && attr == hwmon_temp_input)
> -		return 0444;
> -	if (type == hwmon_in && attr == hwmon_in_input)
> -		return 0444;
> +	const struct fbnic_dev *fbd = drvdata;
> +
> +	switch (type) {
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_input:
> +			return 0444;
> +		case hwmon_temp_min:
> +			return fbnic_hwmon_thr_visible(fbd->fw_cap.temp.min);

[Severity: Low]
The hwmon core calls is_visible only once, from
hwmon_device_register_with_info(). That happens when fbnic_probe() calls
fbnic_hwmon_register(). Can the attribute set get out of sync with
fw_cap after that?

On resume, hibernation restore and AER slot reset, the mailbox is
reinitialized while the hwmon device stays registered:

  __fbnic_pm_resume()
    fbnic_fw_request_mbx()
      __fbnic_fw_enable_mbx()
        fbnic_mbx_init()   /* thresholds reset to NO_DATA */

The next capability response then fills the thresholds again.

The patch handles thresholds that go away (reads return -ENODATA). What
about thresholds that show up after probe? They are stored in fw_cap but
never become sysfs attributes until the driver is re-probed.

That could happen if the capability parse at probe failed early, as
described for fbnic_fw_parse_cap_resp() above, and a later resume parses
cleanly. It could also happen if new firmware flashed through devlink
starts running after a hibernation restore or slot reset without a
re-probe.

In the final series the same probe-time snapshot also decides whether
temp1_alarm and in0_alarm exist.

> +		case hwmon_temp_max:
> +			return fbnic_hwmon_thr_visible(fbd->fw_cap.temp.max);

[ ... ]

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

[Severity: Low]
Are these threshold reads safe against the mailbox writers? The reads of
fbd->fw_cap.temp.* and fbd->fw_cap.volt.* in fbnic_hwmon_temp_read() and
fbnic_hwmon_in_read() are plain loads, and readers hold only the hwmon
core lock.

The writers are fbnic_mbx_init(), which does a whole-struct copy:

	fbd->fw_cap = fw_cap_reset;

and fbnic_fw_parse_cap_resp(), which uses plain scalar stores.

Both run from __fbnic_pm_resume(). fbnic_err_slot_reset() calls that
during AER recovery while userspace is running and the hwmon sysfs files
still exist. That path holds only the devlink lock, which hwmon readers
never take.

The commit message says reads "reject the sentinel with -ENODATA rather
than reporting it as a value". That relies on each read being a single,
untorn load.

Without READ_ONCE()/WRITE_ONCE(), after inlining the compiler may load
the field once for the thr == FBNIC_SENSOR_NO_DATA test and again for
*val = thr. A memcpy-based struct copy also gives no per-s32 atomicity.
Could a reader then report S32_MIN or a torn value? KCSAN would likely
flag this pair of accesses.

Later patches in the series annotate the lockless alarm cache with
READ_ONCE()/WRITE_ONCE() ("Paired with WRITE_ONCE() on the mailbox
thread"). The threshold fields are still unannotated at the end of the
series.

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

[ ... ]

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