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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 28 Mar 2024, Andi Shyti <andi.shyti@linux.intel.com> wrote:
> Hi Arun,
>
> ...
>
>> -	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.

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.

BR,
Jani.


[1] https://lore.kernel.org/r/ki4ynsl4nmhavf63vzdlt2xkedjo7p7iouzvcksvki3okgz6ak@twlznnlo3g22


>
> Andi
>
>>  	xe->enabled_irq_mask = ~0;
>>  
>>  	err = drmm_add_action_or_reset(&xe->drm, display_destroy, NULL);
>> -- 
>> 2.25.1

-- 
Jani Nikula, Intel