[PATCH v2 13/14] drm/xe/hwmon: Fix mutex destroy

[Lucas De Marchi]

On Fri, Feb 07, 2025 at 10:20:18AM +0000, Matthew Auld wrote:
>On 07/02/2025 03:27, Lucas De Marchi wrote:
>>On Thu, Feb 06, 2025 at 03:23:31PM -0800, Lucas De Marchi wrote:
>>>Since the mutex is used if userspace kept the fd open, it can't be
>>>released together with the dev cleanup and needs to be delayed. Also,
>
>Where is that? I would have thought here that 
>devm_hwmon_device_register_with_info() would ensure nothing can reach 
>that lock once the physical device is detached, since everything 
>contained in xe->hwmon is tied to that, including the lock. It looks a 
>little weird but seems like it should be fine?

yeah, I t hink you're right: I forgot that in this case we are not
dealing with the file ourselves and are one layer down the stack, with
hwmon doing the necessary thing to protect that. I will just run a test
and probably drop this patch in next rev.

thanks
Lucas De Marchi

>
>>>there's no need to add an action just to release the mutex: use the
>>>specific drmm_mutex_init() that should be smarter if mutex_destroy() is
>>>actually a nop.
>>>
>>>Cc: Badal Nilawar <badal.nilawar at intel.com>
>>>Cc: Karthik Poosa <karthik.poosa at intel.com>
>>>Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>>
>>oops... 3a13c2de442d6bfaef9c102cd1092e6cae22b753
>>
>>the reasoning is true, but the entire thing is in struct devm allocated.
>>Then CI complains:
>>
>><4> [321.873848] WARNING: CPU: 13 PID: 12544 at 
>>kernel/locking/mutex- debug.c:114 mutex_destroy+0x5b/0x60
>> >
>>Lucas De Marchi
>>
>>>---
>>>drivers/gpu/drm/xe/xe_hwmon.c | 10 +---------
>>>1 file changed, 1 insertion(+), 9 deletions(-)
>>>
>>>diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/ 
>>>xe_hwmon.c
>>>index 7f327e3342123..0fc91c184570d 100644
>>>--- a/drivers/gpu/drm/xe/xe_hwmon.c
>>>+++ b/drivers/gpu/drm/xe/xe_hwmon.c
>>>@@ -869,13 +869,6 @@ xe_hwmon_get_preregistration_info(struct 
>>>xe_device *xe)
>>>            xe_hwmon_energy_get(hwmon, channel, &energy);
>>>}
>>>
>>>-static void xe_hwmon_mutex_destroy(void *arg)
>>>-{
>>>-    struct xe_hwmon *hwmon = arg;
>>>-
>>>-    mutex_destroy(&hwmon->hwmon_lock);
>>>-}
>>>-
>>>void xe_hwmon_register(struct xe_device *xe)
>>>{
>>>    struct device *dev = xe->drm.dev;
>>>@@ -895,8 +888,7 @@ void xe_hwmon_register(struct xe_device *xe)
>>>
>>>    xe->hwmon = hwmon;
>>>
>>>-    mutex_init(&hwmon->hwmon_lock);
>>>-    if (devm_add_action_or_reset(dev, xe_hwmon_mutex_destroy, hwmon))
>>>+    if (drmm_mutex_init(&xe->drm, &hwmon->hwmon_lock))
>>>        return;
>>>
>>>    /* There's only one instance of hwmon per device */
>>>-- 
>>>2.48.1
>>>
>