> -----Original Message----- > From: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx> > Sent: Thursday, March 28, 2024 4:38 PM > To: Murthy, Arun R <arun.r.murthy@xxxxxxxxx> > Cc: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>; Jani Nikula > <jani.nikula@xxxxxxxxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel- > xe@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCHv2] drm/xe/display: check for error on drmm_mutex_init > > Hi Arun, > > > > On Thu, Mar 28, 2024 at 12:33:09PM +0200, Jani Nikula wrote: > > > > On Thu, 28 Mar 2024, Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx> 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 for you comments, will wait to see any more comments and if not will refloat v1 patch version. Thanks and Regards, Arun R Murthy ------------------- > Thanks, > Andi
