On Thu, 28 Mar 2024, Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx> wrote: > Hi Arun, > > ... > >> - drmm_mutex_init(&xe->drm, &xe->sb_lock); >> - drmm_mutex_init(&xe->drm, &xe->display.backlight.lock); >> - drmm_mutex_init(&xe->drm, &xe->display.audio.mutex); >> - drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex); >> - drmm_mutex_init(&xe->drm, &xe->display.pps.mutex); >> - drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex); >> + if (drmm_mutex_init(&xe->drm, &xe->sb_lock) || >> + drmm_mutex_init(&xe->drm, &xe->display.backlight.lock) || >> + drmm_mutex_init(&xe->drm, &xe->display.audio.mutex) || >> + drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex) || >> + drmm_mutex_init(&xe->drm, &xe->display.pps.mutex) || >> + drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex)) >> + return -ENOMEM; > > why not extract the value from drmm_mutex_init()? it would make > the code a bit more complex, but better than forcing a -ENOMEM > return. > > err = drmm_mutex_init(...) > if (err) > return err; > > err = drmm_mutex_init(...) > if (err) > return err; > > err = drmm_mutex_init(...) > if (err) > return err; > > ... > > On the other hand drmm_mutex_init(), as of now returns only > -ENOMEM, but it's a bad practice to assume it will always do. I'd > rather prefer not to check the error value at all. And round and round we go. This is exactly what v1 was [1], but it's not clear because the patch doesn't have a changelog. This is all utterly ridiculous compared to *why* we even have or use drmm_mutex_init(). Managed initialization causes more trouble here than it gains us. Gah. BR, Jani. [1] https://lore.kernel.org/r/ki4ynsl4nmhavf63vzdlt2xkedjo7p7iouzvcksvki3okgz6ak@twlznnlo3g22 > > Andi > >> xe->enabled_irq_mask = ~0; >> >> err = drmm_add_action_or_reset(&xe->drm, display_destroy, NULL); >> -- >> 2.25.1 -- Jani Nikula, Intel
