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

From: Andi Shyti <andi.shyti@linux.intel.com>
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Andi Shyti <andi.shyti@linux.intel.com>,
	Arun R Murthy <arun.r.murthy@intel.com>,
	intel-gfx@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 11:45:44 +0100	[thread overview]
Message-ID: <ZgVKWBQqEat-3iR3@ashyti-mobl2.lan> (raw)
In-Reply-To: <87r0fufxy2.fsf@intel.com>

Hi Jani,

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, 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.

At the end drmm_mutex_init() should make our life easier.

Andi