Re: [PATCH net-next v3 6/6] eth: fbnic: firmware notifies hwmon on sensor threshold events

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 1 (1) · Low: 2

- [High] fbnic_hwmon_notify_event()
  (drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c:248-288) classifies the…
- [Medium] fbnic_hwmon_unregister() open-codes its teardown drain as
  'WRITE_ONCE(fbd->hwmon, NULL); synchronize_irq(fbd->fw_msix_vector);'…
- [Low] fbnic_hwmon_is_visible() returns 0444 unconditionally
  (fbnic_hwmon.c:10-15) and fbnic_hwmon_info[] advertises every…
- [Low] The newly registered hwmon device pointer is published to the
  concurrently running firmware mailbox IRQ thread with a plain…

Pre-existing issues:
- [Medium] A malformed firmware mailbox message can cause
  uninitialized/stale page contents to be dumped into the kernel log.

--

eth: fbnic: firmware notifies hwmon on sensor threshold events

This adds a parser for the unsolicited
FBNIC_TLV_MSG_ID_SENSOR_THRESHOLD_EXCEEDED_RESP firmware message and
translates it into a hwmon alarm notification through
hwmon_notify_event().  It also reworks publish/teardown of fbd->hwmon with
WRITE_ONCE()/READ_ONCE() plus a synchronize_irq() drain in the unregister
path.

No Link: tags present in the commit header.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index e5669dfd9790..fd96a3962e3f 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> @@ -1639,6 +1639,57 @@ fbnic_fw_parser_test(void *opaque, struct fbnic_tlv_msg **results)
>  	return err;
>  }
>  
> +static const struct fbnic_tlv_index fbnic_threshold_exceeded_resp_index[] = {
> +	FBNIC_TLV_ATTR_S32(FBNIC_FW_TSENE_THERM_EXCEEDED_FLAG),
> +	FBNIC_TLV_ATTR_S32(FBNIC_FW_TSENE_VOLT_EXCEEDED_FLAG),
> +	FBNIC_TLV_ATTR_S32(FBNIC_FW_TSENE_THERMAL),
> +	FBNIC_TLV_ATTR_S32(FBNIC_FW_TSENE_VOLTAGE),
> +	FBNIC_TLV_ATTR_LAST
> +};
> +
> +static int fbnic_fw_parse_threshold_exceeded_resp(void *opaque,
> +						  struct fbnic_tlv_msg **results)
> +{
> +	bool therm_exceeded, volt_exceeded;
> +	struct fbnic_dev *fbd = opaque;
> +	s32 value;
> +
> +	therm_exceeded =
> +		fta_get_sint(results, FBNIC_FW_TSENE_THERM_EXCEEDED_FLAG);
> +	volt_exceeded =
> +		fta_get_sint(results, FBNIC_FW_TSENE_VOLT_EXCEEDED_FLAG);
> +
> +	if (!therm_exceeded && !volt_exceeded) {
> +		dev_err(fbd->dev,
> +			"Threshold exceeded message with no flag set\n");
> +		return -EINVAL;
> +	}

[Severity: Medium]

This isn't a problem introduced by this patch, but these new -EINVAL
returns add fresh ways to reach the malformed-message hex dump in
fbnic_mbx_process_rx_msgs(), which can print bytes the firmware never
wrote.

RX mailbox pages are not zeroed:

drivers/net/ethernet/meta/fbnic/fbnic_fw.c:fbnic_mbx_alloc_rx_msgs() {
	...
		msg = (struct fbnic_tlv_msg *)__get_free_page(GFP_KERNEL);
	...
}

and the error path dumps the full firmware-supplied descriptor length
rather than the validated TLV extent:

drivers/net/ethernet/meta/fbnic/fbnic_fw.c:fbnic_mbx_process_rx_msgs() {
	...
			print_hex_dump(KERN_WARNING, "fbnic:",
				       DUMP_PREFIX_OFFSET, 16, 2,
				       msg, length, true);
	...
}

Can this print stale page contents into the kernel log when firmware
advertises a large descriptor length but writes only a short malformed
threshold message?  Would bounding the dump to the parsed TLV length, or
allocating the RX pages with __GFP_ZERO, be preferable?

> +
> +	if (therm_exceeded) {
> +		if (!results[FBNIC_FW_TSENE_THERMAL]) {
> +			dev_err(fbd->dev,
> +				"Thermal threshold exceeded but no value received\n");
> +			return -EINVAL;
> +		}
> +		value = fta_get_sint(results, FBNIC_FW_TSENE_THERMAL);
> +		dev_err(fbd->dev, "Thermal threshold exceeded: %d mC\n", value);
> +		fbnic_hwmon_notify_event(fbd, FBNIC_SENSOR_TEMP, value);
> +	}
> +
> +	if (volt_exceeded) {
> +		if (!results[FBNIC_FW_TSENE_VOLTAGE]) {
> +			dev_err(fbd->dev,
> +				"Voltage threshold exceeded but no value received\n");
> +			return -EINVAL;
> +		}
> +		value = fta_get_sint(results, FBNIC_FW_TSENE_VOLTAGE);
> +		dev_err(fbd->dev, "Voltage threshold exceeded: %d mV\n", value);
> +		fbnic_hwmon_notify_event(fbd, FBNIC_SENSOR_VOLTAGE, value);
> +	}
> +
> +	return 0;
> +}
> +

[ ... ]

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index 11fa8aab0f5b..2dff59bc0e8b 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c

[ ... ]

> @@ -213,22 +215,75 @@ void fbnic_hwmon_register(struct fbnic_dev *fbd)
>  	fbd->hwmon_cache.temp_mdeg = FBNIC_SENSOR_NO_DATA;
>  	fbd->hwmon_cache.volt_mv = FBNIC_SENSOR_NO_DATA;
>  
> -	fbd->hwmon = hwmon_device_register_with_info(fbd->dev, "fbnic",
> -						     fbd, &fbnic_chip_info,
> -						     NULL);
> -	if (IS_ERR(fbd->hwmon)) {
> +	hwmon = hwmon_device_register_with_info(fbd->dev, "fbnic", fbd,
> +						&fbnic_chip_info, NULL);
> +	if (IS_ERR(hwmon)) {
>  		dev_notice(fbd->dev,
>  			   "Failed to register hwmon device %pe\n",
> -			   fbd->hwmon);
> -		fbd->hwmon = NULL;
> +			   hwmon);
> +		return;
>  	}
> +
> +	WRITE_ONCE(fbd->hwmon, hwmon);

[Severity: Low]

Is WRITE_ONCE() strong enough for publishing this pointer?  The FW mailbox
threaded IRQ is requested in fbnic_probe() before fbnic_hwmon_register()
runs, so fbnic_hwmon_notify_event() can load fbd->hwmon on another CPU and
pass it straight to hwmon_notify_event(), which touches dev->kobj.

WRITE_ONCE()/READ_ONCE() prevent tearing but carry no release/acquire
ordering, so on arm64 or ppc64 the store publishing fbd->hwmon is not
ordered after the stores that initialized the device inside
hwmon_device_register_with_info().  Would smp_store_release() here paired
with smp_load_acquire() in the notify path express the intended ordering?

>  }
>  
>  void fbnic_hwmon_unregister(struct fbnic_dev *fbd)
>  {
> +	struct device *hwmon;
> +
>  	if (!IS_REACHABLE(CONFIG_HWMON) || !fbd->hwmon)
>  		return;
>  
> -	hwmon_device_unregister(fbd->hwmon);
> -	fbd->hwmon = NULL;
> +	hwmon = fbd->hwmon;
> +	/* Pair with READ_ONCE() in fbnic_hwmon_notify_event(). Publish NULL
> +	 * and wait for any in-flight FW mailbox IRQ handler to finish so it
> +	 * cannot dereference the hwmon device after we unregister it.
> +	 */
> +	WRITE_ONCE(fbd->hwmon, NULL);
> +	synchronize_irq(fbd->fw_msix_vector);

[Severity: Medium]

Is fbd->fw_msix_vector guaranteed to still name an IRQ owned by this
driver when fbnic_hwmon_unregister() runs?

fbnic_pm_suspend() tears the mailbox IRQ down while hwmon stays
registered:

drivers/net/ethernet/meta/fbnic/fbnic_irq.c:fbnic_fw_free_mbx() {
	...
	free_irq(fbd->fw_msix_vector, fbd);
	fbd->fw_msix_vector = 0;
}

fbnic_hwmon_unregister() is only called from fbnic_remove(), so if
__fbnic_pm_resume() then fails at fbnic_alloc_irqs() the driver stays bound
with fbd->hwmon != NULL and fw_msix_vector == 0.  A later unbind would pass
the !fbd->hwmon guard and call synchronize_irq(0); irq_to_desc(0) is
non-NULL on x86, so this waits on a descriptor owned by another device.

There is a second variant on the request side:

drivers/net/ethernet/meta/fbnic/fbnic_irq.c:fbnic_fw_request_mbx() {
	...
	err = __fbnic_fw_enable_mbx(fbd, vector);
	if (err)
		free_irq(vector, fbd);

	fbd->fw_msix_vector = vector;
	...
}

The vector number is stored even when the IRQ was freed, so after a
resume-time failure and pci_free_irq_vectors() the stored number can be
stale and may have been handed to another driver.

Since fbnic_remove() already does:

drivers/net/ethernet/meta/fbnic/fbnic_pci.c:fbnic_remove() {
	...
	fbnic_hwmon_unregister(fbd);
	fbnic_dbg_fbd_exit(fbd);
	fbnic_devlink_unregister(fbd);
	fbnic_fw_log_disable(fbd);
	fbnic_fw_free_mbx(fbd);
	...
}

would moving fbnic_hwmon_unregister() after fbnic_fw_free_mbx() give the
same barrier without keying it on a mutable vector number?

> +
> +	hwmon_device_unregister(hwmon);
> +}
> +
> +void fbnic_hwmon_notify_event(struct fbnic_dev *fbd, int id, long val)
> +{
> +	enum hwmon_sensor_types type;
> +	struct device *hwmon;
> +	s32 attr = -1;
> +
> +	if (!IS_REACHABLE(CONFIG_HWMON))
> +		return;
> +
> +	switch (id) {
> +	case FBNIC_SENSOR_TEMP:
> +		type = hwmon_temp;
> +
> +		if (val <= fbd->fw_cap.temp.min)
> +			attr = hwmon_temp_min_alarm;
> +		else if (val >= fbd->fw_cap.temp.crit)
> +			attr = hwmon_temp_crit_alarm;
> +		else if (val >= fbd->fw_cap.temp.max)
> +			attr = hwmon_temp_max_alarm;
> +
> +		break;
> +	case FBNIC_SENSOR_VOLTAGE:
> +		type = hwmon_in;
> +
> +		if (val <= fbd->fw_cap.volt.min)
> +			attr = hwmon_in_min_alarm;
> +		else if (val >= fbd->fw_cap.volt.max)
> +			attr = hwmon_in_max_alarm;
> +
> +		break;

[Severity: High]

Do these comparisons handle the case where firmware never advertised a
threshold?  fbnic_fw_parse_cap_resp() stores the FBNIC_SENSOR_NO_DATA
sentinel for any threshold the capability response omits:

drivers/net/ethernet/meta/fbnic/fbnic_fw.c:fbnic_fw_parse_cap_resp() {
	...
	fbd->fw_cap.temp.crit =
		fbnic_tlv_attr_get_signed(results[FBNIC_FW_CAP_RESP_TEMP_CRIT],
					  FBNIC_SENSOR_NO_DATA);
	...
}

and that sentinel is the smallest s32:

drivers/net/ethernet/meta/fbnic/fbnic_fw.h:
#define FBNIC_SENSOR_NO_DATA			S32_MIN

So with temp.crit unset, "val >= fbd->fw_cap.temp.crit" is always true and
every threshold event is reported as temp1_crit_alarm.  Does that also make
the temp1_max_alarm arm below unreachable, since the crit test runs first
even when temp.max is a real value?  The voltage case looks the same:
an unset volt.max turns any reported value into in0_max_alarm.

The read paths filter the sentinel explicitly:

drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c:fbnic_hwmon_read_alarm() {
	if (a == FBNIC_SENSOR_NO_DATA || b == FBNIC_SENSOR_NO_DATA)
		return -ENODATA;
	...
}

which means userspace gets woken on an attribute that then reads back as
-ENODATA.  Should the notify path skip thresholds equal to
FBNIC_SENSOR_NO_DATA the same way?

> +	default:
> +		return;
> +	}
> +
> +	/* Pair with WRITE_ONCE() in fbnic_hwmon_unregister(). Skip the
> +	 * notification if hwmon failed to register or has already been torn
> +	 * down.
> +	 */
> +	hwmon = READ_ONCE(fbd->hwmon);
> +	if (attr >= 0 && hwmon)
> +		hwmon_notify_event(hwmon, type, attr, 0);
>  }

[Severity: Low]

This isn't a bug in the new notify path itself, but related to the
attributes it notifies on: fbnic_hwmon_is_visible() returns 0444
unconditionally and fbnic_hwmon_info[] advertises every threshold and
alarm:

drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c:
	HWMON_CHANNEL_INFO(in,
			   HWMON_I_INPUT |
			   HWMON_I_MIN | HWMON_I_MIN_ALARM |
			   HWMON_I_MAX | HWMON_I_MAX_ALARM),

so temp1_{min,max,crit}, in0_{min,max} and the matching *_alarm files exist
even when the capability response omitted the threshold and
fbnic_hwmon_read_threshold() can only ever return -ENODATA.

Documentation/hwmon/sysfs-interface.rst says:

  All entries (except name) are optional, and should only be created in a
  given driver if the chip has the feature.

Could fbnic_hwmon_is_visible() key visibility off the corresponding
capability value, and hide an alarm when its threshold is unavailable?

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