[PATCHv2] drm/xe/display: check for error on drmm_mutex_init

[Murthy, Arun R]

> -----Original Message-----
> From: Andi Shyti <andi.shyti at linux.intel.com>
> Sent: Thursday, March 28, 2024 4:38 PM
> To: Murthy, Arun R <arun.r.murthy at intel.com>
> Cc: Andi Shyti <andi.shyti at linux.intel.com>; Jani Nikula
> <jani.nikula at linux.intel.com>; intel-gfx at lists.freedesktop.org; intel-
> xe at lists.freedesktop.org
> 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 at 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 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