| 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 |
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 >
> -----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 > >
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 --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);
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(-)