Re: [PATCH v4 2/2] hwmon: add AMD Promontory 21 xHCI temperature sensor support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Using pm_runtime_put_noidle() can permanently prevent the PCI device from runtime-suspending.
- [Low] The local hwmon->lock mutex is redundant and violates the hwmon subsystem locking architecture.
--

commit 62e33fab546f8b14b0cd80683aa504672db60e64
Author: Jihong Min <hurryman2212@gmail.com>

hwmon: add AMD Promontory 21 xHCI temperature sensor support

This commit adds an auxiliary-bus hwmon driver to expose the temperature
sensor on AMD Promontory 21 xHCI controllers. It reads the sensor value
through a vendor-specific index/data register pair in the PCI MMIO BAR.

> diff --git a/drivers/hwmon/prom21-xhci.c b/drivers/hwmon/prom21-xhci.c
> new file mode 100644
> index 0000000000000..f91303ce3428d
> --- /dev/null
> +++ b/drivers/hwmon/prom21-xhci.c
> @@ -0,0 +1,250 @@

[ ... ]

> +static int prom21_xhci_read_temp_raw_restore_index(struct prom21_xhci *hwmon,
> +						   u8 *raw)
> +{
> +	struct device *dev = &hwmon->pdev->dev;
> +	bool pm_ref;
> +	u32 index;
> +	u32 data;
> +	int ret;
> +
> +	ret = prom21_xhci_pm_get(hwmon, &pm_ref);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&hwmon->lock);

Is this private mutex redundant here?

According to the hardware monitoring subsystem guidelines, the hwmon core
automatically serializes sysfs operations for attributes registered via the
info parameter of hwmon_device_register_with_info().

Since this driver registers its only attribute (temp1_input) via the info
parameter and does not access the hardware from interrupt handlers or other
asynchronous paths, does it need its own serialization?

> +	index = readl(hwmon->regs + PROM21_INDEX);
> +	/* Select the PROM21 temperature register through the vendor index. */
> +	writel(PROM21_TEMP_REG, hwmon->regs + PROM21_INDEX);
> +	data = readl(hwmon->regs + PROM21_DATA);
> +	/* Restore the previous vendor index register value. */
> +	writel(index, hwmon->regs + PROM21_INDEX);
> +	readl(hwmon->regs + PROM21_INDEX);
> +	mutex_unlock(&hwmon->lock);
> +
> +	if (pm_ref) {
> +		/*
> +		 * Drop only the reference taken by pm_runtime_get_if_active().
> +		 * Do not mark the device busy or schedule autosuspend from the
> +		 * hwmon path; sensor polling must not keep the xHCI PCI device
> +		 * active.
> +		 */
> +		pm_runtime_put_noidle(dev);

Can using pm_runtime_put_noidle() permanently prevent the PCI device from
runtime-suspending?

If the xHCI driver drops its own runtime PM reference concurrently (e.g., via
pm_runtime_put_autosuspend()), the PM core's idle check sees usage_count > 0,
returns -EAGAIN, and skips scheduling the autosuspend timer.

This code then calls pm_runtime_put_noidle(), which decrements usage_count
to 0 but intentionally bypasses queueing an idle check or notifying the PM
core.

Could this leave the xHCI controller permanently in the RPM_ACTIVE state with
a usage_count of 0 and no pending autosuspend timer until the next PM state
change?

To safely drop the reference without resetting the autosuspend timer, should
this use pm_runtime_put() instead, which properly queues an asynchronous
idle check?

> +	}
> +
> +	*raw = data & 0xff;
> +	if (!*raw || *raw == 0xff)
> +		return -ENODATA;
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508143910.14673-1-hurryman2212@gmail.com?part=2