Re: [PATCH net-next v3 3/6] eth: fbnic: cache hwmon sensor readings

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] Forward-referencing comment on the newly added
  FBNIC_SENSOR_NO_DATA macro in…

--

eth: fbnic: cache hwmon sensor readings

The patch caches the last TSENE temperature and voltage reading in
struct fbnic_dev and serves hwmon reads from that cache for the
remainder of the current jiffy, so a single firmware round-trip can
satisfy a batch of attribute reads. The cache is seeded at registration
with a new FBNIC_SENSOR_NO_DATA sentinel so the first read always
refreshes.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
> index d84723e4cfa3..42a5f83ddb45 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h

[ ... ]

> @@ -44,6 +45,12 @@ struct fbnic_fw_ver {
>  	char commit[FBNIC_FW_CAP_RESP_COMMIT_MAX_SIZE];
>  };
>  
> +/* Sentinel for a sensor value the driver does not have: a threshold the
> + * firmware never populated (older firmware) or a cache entry not yet
> + * refreshed.
> + */
> +#define FBNIC_SENSOR_NO_DATA			S32_MIN
> +

[Severity: Low]

This isn't a bug, but does the comment describe more than this patch
implements?

At this commit the only users of FBNIC_SENSOR_NO_DATA are the cache seed
in fbnic_hwmon_register():

	fbd->hwmon_cache.temp_mdeg = FBNIC_SENSOR_NO_DATA;
	fbd->hwmon_cache.volt_mv = FBNIC_SENSOR_NO_DATA;

and the cache-validity test in fbnic_hwmon_sensor_read():

	if (*cached != FBNIC_SENSOR_NO_DATA && cache->last_read == jiffies) {

There is no threshold concept yet: struct fbnic_fw_cap has no temperature
or voltage threshold fields, and fbnic_hwmon_info[] registers only
temp1_input and in0_input.

The "a threshold the firmware never populated (older firmware)" wording
only becomes accurate later in the series, once the capability parser
starts using fbnic_tlv_attr_get_signed(..., FBNIC_SENSOR_NO_DATA) and
fbnic_hwmon_read_threshold()/fbnic_hwmon_read_alarm() compare against it.

Would it be clearer to restrict the comment to the cache semantics here
and extend it in the patch that adds the threshold handling?

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