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

[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_mbx_init()
  (drivers/net/ethernet/meta/fbnic/fbnic_fw.c:112-113) resets…

--

eth: fbnic: cache hwmon sensor readings

This commit caches the last TSENE temperature and voltage readings in
struct fbnic_dev. Hwmon reads are served from that cache for the rest of
the current jiffy. The cache is seeded with the FBNIC_SENSOR_NO_DATA
sentinel in fbnic_mbx_init(), so the first read always goes to firmware.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index 49b8813a35643..82307f212f583 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> @@ -105,6 +105,13 @@ void fbnic_mbx_init(struct fbnic_dev *fbd)
>  	/* Reset FW Capabilities */
>  	memset(&fbd->fw_cap, 0, sizeof(fbd->fw_cap));
>  
> +	/* Seed cache with sentinel so the first read always refreshes. The
> +	 * cache mirrors what firmware reported, so drop it when the mailbox
> +	 * restarts.
> +	 */
> +	fbd->hwmon_cache.temp_mdeg = FBNIC_SENSOR_NO_DATA;
> +	fbd->hwmon_cache.volt_mv = FBNIC_SENSOR_NO_DATA;

[Severity: Low]

Can these stores race with a sysfs read in fbnic_hwmon_sensor_read()?

The hwmon device stays registered during PCI error recovery.
fbnic_hwmon_unregister() is only called from fbnic_remove(). The mailbox
is restarted on that path:

fbnic_err_slot_reset()
  __fbnic_pm_resume()
    fbnic_fw_request_mbx()
      __fbnic_fw_enable_mbx()
        fbnic_mbx_init()

This path runs under devl_lock(). The hwmon read callbacks are only
serialized by the hwmon core lock, so nothing orders the two.

Suppose a reader has already received a successful TSENE completion and is
preempted just before this code in fbnic_hwmon_sensor_read():

	cache->temp_mdeg = fw_cmpl->u.tsene.millidegrees;
	cache->volt_mv = fw_cmpl->u.tsene.millivolts;
	cache->last_read = jiffies;

fbnic_mbx_init() can then store FBNIC_SENSOR_NO_DATA. When the reader
resumes, it writes back the pre-reset readings with a fresh last_read.

Wouldn't that undo the "drop it when the mailbox restarts" invalidation?
KCSAN would also flag these plain concurrent loads and stores.

The practical effect is small. FBNIC depends on 64BIT, so the s32 stores
can't tear, and a stale value would only be served for the rest of one
jiffy.

Could the invalidation be done under hwmon_lock(fbd->hwmon) when
fbd->hwmon is set? Another option is READ_ONCE()/WRITE_ONCE() with a
validity scheme.

The same unlocked pattern appears to remain later in the series, where
temp_alarm and volt_alarm are also seeded in fbnic_mbx_init() without the
lock.

> +
>  	/* Reinitialize mailbox memory */
>  	for (i = 0; i < FBNIC_IPC_MBX_INDICES; i++)
>  		memset(&fbd->mbx[i], 0, sizeof(struct fbnic_fw_mbx));

[ ... ]

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index 6c8c66ab86c13..0c3fe2a372275 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c

[ ... ]

> @@ -72,7 +78,12 @@ static int fbnic_hwmon_sensor_read(struct fbnic_dev *fbd, int id, long *val)
>  		goto exit_cleanup;
>  	}
>  
> -	*val = *sensor;
> +	/* FW returns both readings in one response, cache both. */
> +	cache->temp_mdeg = fw_cmpl->u.tsene.millidegrees;
> +	cache->volt_mv = fw_cmpl->u.tsene.millivolts;
> +	cache->last_read = jiffies;
> +
> +	*val = *cached;

[ ... ]

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