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

On 5/9/26 06:52, [email protected] wrote:
 > 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.
 > --

Both points make sense.

 > 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?

This was already removed locally after Guenter's review. The driver now 
relies
on hwmon core serialization for its own callbacks.

I also updated the comment around the PROM21 vendor index/data access to 
avoid
implying that the driver can synchronize with firmware, SMM, ACPI, or other
possible users outside this driver.

 > 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?

Yes, that is a good point. I changed the successful read path to use
pm_runtime_put() instead.

The driver still does not runtime-resume the parent device for a hwmon read:
it only reads when pm_runtime_get_if_active() succeeds. But after a 
successful
read, pm_runtime_put() lets the PM core re-evaluate the idle state after the
active-only runtime PM reference is dropped.

I also updated the runtime PM documentation accordingly.

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

Yes. That is what I changed locally.

These changes are already in my v5 work-in-progress branch:

   https://github.com/hurryman2212/linux/tree/prom21_hwmon

Following Mario's advice, I will defer posting v5 to the mailing list 
for a few
days while waiting for more review feedback and doing additional 
self-review.

Sincerely,
Jihong Min