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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The hardware monitoring cache expiration check is vulnerable to a 24-day jiffies wraparound bug on 32-bit systems, which can cause the driver to indefinitely serve stale sensor readings.
--

--- Patch [3]: [PATCH net-next 3/6] eth: fbnic: cache hwmon sensor readings ---
commit 4d77f6b947d1d111d9ffbbeb2ef3742b0f54686f
Author: Zinc Lim <[email protected]>
Subject: eth: fbnic: cache hwmon sensor readings

This commit introduces caching for hardware monitoring sensor readings to
reduce firmware mailbox IPC traffic. It caches the temperature and voltage
values and serves them for the remainder of the current jiffy, using a
sentinel value to ensure the first read always triggers a refresh.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
[ ... ]
> @@ -25,26 +26,32 @@ static int fbnic_hwmon_sensor_read(struct fbnic_dev *fbd, int id, long *val)
>  {
> +	struct fbnic_hwmon_cache *cache = &fbd->hwmon_cache;
>  	struct fbnic_fw_completion *fw_cmpl;
>  	int err = 0;
> +	s32 *cached;
>  
>  	switch (id) {
>  	case FBNIC_SENSOR_TEMP:
> +		cached = &cache->temp_mdeg;
>  		break;
>  	case FBNIC_SENSOR_VOLTAGE:
> +		cached = &cache->volt_mv;
>  		break;
>  	default:
> +		return -EINVAL;
> +	}
> +
> +	if (*cached != FBNIC_SENSOR_NO_DATA &&
> +	    time_is_after_eq_jiffies(cache->last_read)) {

[Severity: Medium]
Can this cache expiration check cause stale readings on 32-bit systems due to
jiffies wraparound?

If the hwmon sysfs files are not read for ~24.8 days, the jiffies counter will
advance such that the signed subtraction in time_is_after_eq_jiffies()
underflows. This would cause the macro to evaluate to true, resulting in a
cache hit.

Since a cache hit bypasses the firmware read and does not update last_read,
would the driver continually return the same stale cached value for the next
24.8 days until the difference wraps back to negative?

Given the commit message mentions serving reads "for the remainder of the
current jiffy", should this explicitly check for jiffies == cache->last_read
instead?

> +		*val = *cached;
> +		return 0;
>  	}

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