diff mbox series

drm/xe/display: check for error on drmm_mutex_init

Message ID 20240319030341.3907617-1-arun.r.murthy@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/xe/display: check for error on drmm_mutex_init | expand

Commit Message

Arun R Murthy March 19, 2024, 3:03 a.m. UTC
Check return value for drmm_mutex_init as it can fail and return on
failure.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/xe/display/xe_display.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

Comments

Lucas De Marchi March 20, 2024, 12:35 a.m. UTC | #1
On Tue, Mar 19, 2024 at 08:33:41AM +0530, Arun R Murthy wrote:
>Check return value for drmm_mutex_init as it can fail and return on
>failure.
>
>Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>---
> drivers/gpu/drm/xe/display/xe_display.c | 24 ++++++++++++++++++------
> 1 file changed, 18 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
>index e4db069f0db3..c59fa832758d 100644
>--- a/drivers/gpu/drm/xe/display/xe_display.c
>+++ b/drivers/gpu/drm/xe/display/xe_display.c
>@@ -107,12 +107,24 @@ int xe_display_create(struct xe_device *xe)
>
> 	xe->display.hotplug.dp_wq = alloc_ordered_workqueue("xe-dp", 0);
>
>-	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);
>+	err = drmm_mutex_init(&xe->drm, &xe->sb_lock);
>+	if (err)
>+		return err;
>+	err = drmm_mutex_init(&xe->drm, &xe->display.backlight.lock);
>+	if (err)
>+		return err;
>+	err = drmm_mutex_init(&xe->drm, &xe->display.audio.mutex);
>+	if (err)
>+		return err;
>+	err = drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex);
>+	if (err)
>+		return err;
>+	err = drmm_mutex_init(&xe->drm, &xe->display.pps.mutex);
>+	if (err)
>+		return err;
>+	err = drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex);
>+	if (err)
>+		return err;


humn... but not very pretty. What about?

	if ((err = drmm_mutex_init(&xe->drm, &xe->sb_lock)) ||
	    (err = drmm_mutex_init(&xe->drm, &xe->display.backlight.lock)) ||
	    (err = ...))
		return err;

I think there are few places in life for assignment + check in single
statement, but IMO this is one of them where the alternative is uglier
and more error prone.

thoughts?

Lucas De Marchi

> 	xe->enabled_irq_mask = ~0;
>
> 	err = drmm_add_action_or_reset(&xe->drm, display_destroy, NULL);
>-- 
>2.25.1
>
Arun R Murthy March 21, 2024, 5:04 a.m. UTC | #2
> -----Original Message-----
> From: De Marchi, Lucas <lucas.demarchi@intel.com>
> Sent: Wednesday, March 20, 2024 6:06 AM
> To: Murthy, Arun R <arun.r.murthy@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> Subject: Re: [PATCH] drm/xe/display: check for error on drmm_mutex_init
> 
> On Tue, Mar 19, 2024 at 08:33:41AM +0530, Arun R Murthy wrote:
> >Check return value for drmm_mutex_init as it can fail and return on
> >failure.
> >
> >Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> >---
> > drivers/gpu/drm/xe/display/xe_display.c | 24 ++++++++++++++++++------
> > 1 file changed, 18 insertions(+), 6 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/xe/display/xe_display.c
> >b/drivers/gpu/drm/xe/display/xe_display.c
> >index e4db069f0db3..c59fa832758d 100644
> >--- a/drivers/gpu/drm/xe/display/xe_display.c
> >+++ b/drivers/gpu/drm/xe/display/xe_display.c
> >@@ -107,12 +107,24 @@ int xe_display_create(struct xe_device *xe)
> >
> > 	xe->display.hotplug.dp_wq = alloc_ordered_workqueue("xe-dp", 0);
> >
> >-	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);
> >+	err = drmm_mutex_init(&xe->drm, &xe->sb_lock);
> >+	if (err)
> >+		return err;
> >+	err = drmm_mutex_init(&xe->drm, &xe->display.backlight.lock);
> >+	if (err)
> >+		return err;
> >+	err = drmm_mutex_init(&xe->drm, &xe->display.audio.mutex);
> >+	if (err)
> >+		return err;
> >+	err = drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex);
> >+	if (err)
> >+		return err;
> >+	err = drmm_mutex_init(&xe->drm, &xe->display.pps.mutex);
> >+	if (err)
> >+		return err;
> >+	err = drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex);
> >+	if (err)
> >+		return err;
> 
> 
> humn... but not very pretty. What about?
> 
> 	if ((err = drmm_mutex_init(&xe->drm, &xe->sb_lock)) ||
> 	    (err = drmm_mutex_init(&xe->drm, &xe->display.backlight.lock)) ||
> 	    (err = ...))
> 		return err;
> 
> I think there are few places in life for assignment + check in single statement,
> but IMO this is one of them where the alternative is uglier and more error
> prone.
> 
> thoughts?
> 

We should not proceed with the remaining mutex_init in case of failures. As an alternative we can have 
drmm_mutex_init(var1) ? (drmm_mutex_init(var2) ? drmm_mutex_init(var3) : return ret) : return ret;

With the existing one traversing the code is more easier, these optimization might make the code look complex.

Thanks and Regards,
Arun R Murthy
--------------------
> Lucas De Marchi
> 
> > 	xe->enabled_irq_mask = ~0;
> >
> > 	err = drmm_add_action_or_reset(&xe->drm, display_destroy, NULL);
> >--
> >2.25.1
> >
Lucas De Marchi March 21, 2024, 5:43 a.m. UTC | #3
On Thu, Mar 21, 2024 at 05:04:51AM +0000, Murthy, Arun R wrote:
>
>
>> -----Original Message-----
>> From: De Marchi, Lucas <lucas.demarchi@intel.com>
>> Sent: Wednesday, March 20, 2024 6:06 AM
>> To: Murthy, Arun R <arun.r.murthy@intel.com>
>> Cc: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/xe/display: check for error on drmm_mutex_init
>>
>> On Tue, Mar 19, 2024 at 08:33:41AM +0530, Arun R Murthy wrote:
>> >Check return value for drmm_mutex_init as it can fail and return on
>> >failure.
>> >
>> >Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>> >---
>> > drivers/gpu/drm/xe/display/xe_display.c | 24 ++++++++++++++++++------
>> > 1 file changed, 18 insertions(+), 6 deletions(-)
>> >
>> >diff --git a/drivers/gpu/drm/xe/display/xe_display.c
>> >b/drivers/gpu/drm/xe/display/xe_display.c
>> >index e4db069f0db3..c59fa832758d 100644
>> >--- a/drivers/gpu/drm/xe/display/xe_display.c
>> >+++ b/drivers/gpu/drm/xe/display/xe_display.c
>> >@@ -107,12 +107,24 @@ int xe_display_create(struct xe_device *xe)
>> >
>> > 	xe->display.hotplug.dp_wq = alloc_ordered_workqueue("xe-dp", 0);
>> >
>> >-	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);
>> >+	err = drmm_mutex_init(&xe->drm, &xe->sb_lock);
>> >+	if (err)
>> >+		return err;
>> >+	err = drmm_mutex_init(&xe->drm, &xe->display.backlight.lock);
>> >+	if (err)
>> >+		return err;
>> >+	err = drmm_mutex_init(&xe->drm, &xe->display.audio.mutex);
>> >+	if (err)
>> >+		return err;
>> >+	err = drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex);
>> >+	if (err)
>> >+		return err;
>> >+	err = drmm_mutex_init(&xe->drm, &xe->display.pps.mutex);
>> >+	if (err)
>> >+		return err;
>> >+	err = drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex);
>> >+	if (err)
>> >+		return err;
>>
>>
>> humn... but not very pretty. What about?
>>
>> 	if ((err = drmm_mutex_init(&xe->drm, &xe->sb_lock)) ||
>> 	    (err = drmm_mutex_init(&xe->drm, &xe->display.backlight.lock)) ||
>> 	    (err = ...))
>> 		return err;
>>
>> I think there are few places in life for assignment + check in single statement,
>> but IMO this is one of them where the alternative is uglier and more error
>> prone.
>>
>> thoughts?
>>
>
>We should not proceed with the remaining mutex_init in case of failures. As an alternative we can have

with the code above, we are not proceeding with the other drmm_mutex_init() initializations.

foo() || bar() doesn't execute bar() if foo() returned != 0.

Lucas De Marchi

>drmm_mutex_init(var1) ? (drmm_mutex_init(var2) ? drmm_mutex_init(var3) : return ret) : return ret;
>
>With the existing one traversing the code is more easier, these optimization might make the code look complex.
>
>Thanks and Regards,
>Arun R Murthy
>--------------------
>> Lucas De Marchi
>>
>> > 	xe->enabled_irq_mask = ~0;
>> >
>> > 	err = drmm_add_action_or_reset(&xe->drm, display_destroy, NULL);
>> >--
>> >2.25.1
>> >
diff mbox series

Patch

diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
index e4db069f0db3..c59fa832758d 100644
--- a/drivers/gpu/drm/xe/display/xe_display.c
+++ b/drivers/gpu/drm/xe/display/xe_display.c
@@ -107,12 +107,24 @@  int xe_display_create(struct xe_device *xe)
 
 	xe->display.hotplug.dp_wq = alloc_ordered_workqueue("xe-dp", 0);
 
-	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);
+	err = drmm_mutex_init(&xe->drm, &xe->sb_lock);
+	if (err)
+		return err;
+	err = drmm_mutex_init(&xe->drm, &xe->display.backlight.lock);
+	if (err)
+		return err;
+	err = drmm_mutex_init(&xe->drm, &xe->display.audio.mutex);
+	if (err)
+		return err;
+	err = drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex);
+	if (err)
+		return err;
+	err = drmm_mutex_init(&xe->drm, &xe->display.pps.mutex);
+	if (err)
+		return err;
+	err = drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex);
+	if (err)
+		return err;
 	xe->enabled_irq_mask = ~0;
 
 	err = drmm_add_action_or_reset(&xe->drm, display_destroy, NULL);