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

From: Andi Shyti <andi.shyti@linux.intel.com>
To: "Murthy, Arun R" <arun.r.murthy@intel.com>
Cc: Andi Shyti <andi.shyti@linux.intel.com>,
	Jani Nikula <jani.nikula@linux.intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"intel-xe@lists.freedesktop.org" <intel-xe@lists.freedesktop.org>
Subject: Re: [PATCHv2] drm/xe/display: check for error on drmm_mutex_init
Date: Thu, 28 Mar 2024 12:07:59 +0100	[thread overview]
Message-ID: <ZgVPj8wHbjj2Oyu_@ashyti-mobl2.lan> (raw)
In-Reply-To: <IA0PR11MB730702DC9B3DBCAFB8396B33BA3B2@IA0PR11MB7307.namprd11.prod.outlook.com>

Hi Arun,

> > On Thu, Mar 28, 2024 at 12:33:09PM +0200, Jani Nikula wrote:
> > > On Thu, 28 Mar 2024, Andi Shyti <andi.shyti@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