Hi Arun, > > On Thu, Mar 28, 2024 at 12:33:09PM +0200, Jani Nikula wrote: > > > On Thu, 28 Mar 2024, Andi Shyti <andi.sh...@linux.intel.com> wrote: > > > >> - 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, > > The function is also returning -ENOMEM and there is no other error code > returned from the error.
yes, but it's wrong to assume this will always be true. > > > > 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. > > > > ha! funny! I missed v1. > > > > > 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. > > > > As I wrote here: > > > > > > I'd rather prefer not to check the error value at all. > > > > we could rather drop this patch. Checking the error value is always good, > > but > > checking implausible errors with this price is not really worth it. > > This is reported as error from Coverity. My suggestion was also to discard > this error from Coverity but the same API used in other places in our driver > is considering the return value. Strictly speaking, coverity is right and if I have to choose, I'd prefer your v1. v2, in my opinion, is wrong, even if it's more nice and readable. Thanks, Andi
