[PATCH] drm/i915/display: use IS_ERR_OR_NULL macro on DP tunnel mgr creation failure

[PATCH] drm/i915/display: use IS_ERR_OR_NULL macro on DP tunnel mgr creation failure

[Krzysztof Karas]

drm_dp_tunnel_mgr_create() may return NULL on failure, which will not
be caught via IS_ERR(), so replace it with IS_ERR_OR_NULL() macro.

Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp_tunnel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_tunnel.c b/drivers/gpu/drm/i915/display/intel_dp_tunnel.c
index 94198bc04939..6c960416f776 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_tunnel.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_tunnel.c
@@ -793,7 +793,7 @@ int intel_dp_tunnel_mgr_init(struct intel_display *display)
 	drm_connector_list_iter_end(&connector_list_iter);
 
 	tunnel_mgr = drm_dp_tunnel_mgr_create(display->drm, dp_connectors);
-	if (IS_ERR(tunnel_mgr))
+	if (IS_ERR_OR_NULL(tunnel_mgr))
 		return PTR_ERR(tunnel_mgr);
 
 	display->dp_tunnel_mgr = tunnel_mgr;
-- 
2.34.1

Re: [PATCH] drm/i915/display: use IS_ERR_OR_NULL macro on DP tunnel mgr creation failure

[Andi Shyti]

Hi Krzysztof,

On Wed, Dec 11, 2024 at 09:56:50AM +0000, Krzysztof Karas wrote:
> drm_dp_tunnel_mgr_create() may return NULL on failure, which will not
> be caught via IS_ERR(), so replace it with IS_ERR_OR_NULL() macro.
> 
> Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com>

Fixes: 91888b5b1ad2 ("drm/i915/dp: Add support for DP tunnel BW allocation")
Cc: Imre Deak <imre.deak@intel.com>
Cc: <stable@vger.kernel.org> # v6.9+

> ---
>  drivers/gpu/drm/i915/display/intel_dp_tunnel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_tunnel.c b/drivers/gpu/drm/i915/display/intel_dp_tunnel.c
> index 94198bc04939..6c960416f776 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_tunnel.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_tunnel.c
> @@ -793,7 +793,7 @@ int intel_dp_tunnel_mgr_init(struct intel_display *display)
>  	drm_connector_list_iter_end(&connector_list_iter);
>  
>  	tunnel_mgr = drm_dp_tunnel_mgr_create(display->drm, dp_connectors);
> -	if (IS_ERR(tunnel_mgr))
> +	if (IS_ERR_OR_NULL(tunnel_mgr))

nicely spotted, but the fix is wrong. drm_dp_tunnel_mgr_create()
returns NULL, not an error, so that you can just check:

	if (!tunnel_mgr)
		...

Thanks,
Andi

>  		return PTR_ERR(tunnel_mgr);
>  
>  	display->dp_tunnel_mgr = tunnel_mgr;
> -- 
> 2.34.1

Re: [PATCH] drm/i915/display: use IS_ERR_OR_NULL macro on DP tunnel mgr creation failure

[Michal Wajdeczko]

On 11.12.2024 10:56, Krzysztof Karas wrote:
> drm_dp_tunnel_mgr_create() may return NULL on failure, which will not
> be caught via IS_ERR(), so replace it with IS_ERR_OR_NULL() macro.
> 
> Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_tunnel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_tunnel.c b/drivers/gpu/drm/i915/display/intel_dp_tunnel.c
> index 94198bc04939..6c960416f776 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_tunnel.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_tunnel.c
> @@ -793,7 +793,7 @@ int intel_dp_tunnel_mgr_init(struct intel_display *display)
>  	drm_connector_list_iter_end(&connector_list_iter);
>  
>  	tunnel_mgr = drm_dp_tunnel_mgr_create(display->drm, dp_connectors);
> -	if (IS_ERR(tunnel_mgr))
> +	if (IS_ERR_OR_NULL(tunnel_mgr))
>  		return PTR_ERR(tunnel_mgr);

this still will not work as expected, since in case of NULL it will
return 0 (success) instead of "a negative error code" as described in
the documentation of the intel_dp_tunnel_mgr_init()

OTOH the documentation of drm_dp_tunnel_mgr_create() says: "Returns a
pointer to the tunnel manager if created successfully or NULL in case of
an error" so more appropriate fix seems to be:

-	if (IS_ERR(tunnel_mgr))
- 		return PTR_ERR(tunnel_mgr);
+	if (!tunnel_mgr)
+ 		return -ENOMEM;

but then it will not work with the drm_dp_tunnel_mgr_create() stub which
actually returns undocumented ERR_PTR(-EOPNOTSUPP)

so unless you are ready to update implementation and documentation of
the drm_dp_tunnel_mgr_create() to return ERR_PTR instead of NULL in case
of error, the fix IMO should look more like:

+	if (!tunnel_mgr)
+ 		return -ENOMEM;

and keep existing IS_ERR check

>  
>  	display->dp_tunnel_mgr = tunnel_mgr;

✗ i915.CI.BAT: failure for drm/i915/display: use IS_ERR_OR_NULL macro on DP tunnel mgr creation failure

[Patchwork]

== Series Details ==

Series: drm/i915/display: use IS_ERR_OR_NULL macro on DP tunnel mgr creation failure
URL   : https://patchwork.freedesktop.org/series/142404/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_15819 -> Patchwork_142404v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_142404v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_142404v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142404v1/index.html

Participating hosts (46 -> 45)
------------------------------

  Missing    (1): fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_142404v1:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_flip@basic-flip-vs-modeset@c-dp1:
    - bat-apl-1:          [PASS][1] -> [DMESG-WARN][2] +1 other test dmesg-warn
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15819/bat-apl-1/igt@kms_flip@basic-flip-vs-modeset@c-dp1.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142404v1/bat-apl-1/igt@kms_flip@basic-flip-vs-modeset@c-dp1.html

  
Known issues
------------

  Here are the changes found in Patchwork_142404v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live:
    - bat-mtlp-8:         [PASS][3] -> [ABORT][4] ([i915#12061]) +1 other test abort
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15819/bat-mtlp-8/igt@i915_selftest@live.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142404v1/bat-mtlp-8/igt@i915_selftest@live.html
    - bat-arls-5:         NOTRUN -> [ABORT][5] ([i915#12061])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142404v1/bat-arls-5/igt@i915_selftest@live.html

  * igt@i915_selftest@live@workarounds:
    - bat-arlh-3:         [PASS][6] -> [ABORT][7] ([i915#12061]) +1 other test abort
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15819/bat-arlh-3/igt@i915_selftest@live@workarounds.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142404v1/bat-arlh-3/igt@i915_selftest@live@workarounds.html
    - bat-arls-5:         [PASS][8] -> [ABORT][9] ([i915#12061])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15819/bat-arls-5/igt@i915_selftest@live@workarounds.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142404v1/bat-arls-5/igt@i915_selftest@live@workarounds.html

  * igt@kms_addfb_basic@too-high:
    - fi-cfl-8109u:       [PASS][10] -> [DMESG-WARN][11] ([i915#11621]) +52 other tests dmesg-warn
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15819/fi-cfl-8109u/igt@kms_addfb_basic@too-high.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142404v1/fi-cfl-8109u/igt@kms_addfb_basic@too-high.html

  * igt@kms_flip@basic-flip-vs-dpms:
    - bat-apl-1:          [PASS][12] -> [DMESG-WARN][13] ([i915#12921])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15819/bat-apl-1/igt@kms_flip@basic-flip-vs-dpms.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142404v1/bat-apl-1/igt@kms_flip@basic-flip-vs-dpms.html

  * igt@kms_flip@basic-flip-vs-modeset@a-dp1:
    - bat-apl-1:          [PASS][14] -> [DMESG-WARN][15] ([i915#12918]) +1 other test dmesg-warn
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15819/bat-apl-1/igt@kms_flip@basic-flip-vs-modeset@a-dp1.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142404v1/bat-apl-1/igt@kms_flip@basic-flip-vs-modeset@a-dp1.html

  * igt@kms_pipe_crc_basic@read-crc:
    - bat-dg2-11:         [PASS][16] -> [SKIP][17] ([i915#9197]) +1 other test skip
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15819/bat-dg2-11/igt@kms_pipe_crc_basic@read-crc.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142404v1/bat-dg2-11/igt@kms_pipe_crc_basic@read-crc.html

  
#### Possible fixes ####

  * igt@core_auth@basic-auth:
    - fi-cfl-8109u:       [DMESG-WARN][18] ([i915#11621]) -> [PASS][19] +1 other test pass
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15819/fi-cfl-8109u/igt@core_auth@basic-auth.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142404v1/fi-cfl-8109u/igt@core_auth@basic-auth.html

  * igt@i915_pm_rpm@module-reload:
    - bat-dg2-11:         [FAIL][20] ([i915#12903]) -> [PASS][21]
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15819/bat-dg2-11/igt@i915_pm_rpm@module-reload.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142404v1/bat-dg2-11/igt@i915_pm_rpm@module-reload.html
    - bat-dg1-7:          [FAIL][22] ([i915#12903]) -> [PASS][23]
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15819/bat-dg1-7/igt@i915_pm_rpm@module-reload.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142404v1/bat-dg1-7/igt@i915_pm_rpm@module-reload.html

  * igt@i915_selftest@live@workarounds:
    - bat-mtlp-6:         [ABORT][24] ([i915#12061]) -> [PASS][25] +1 other test pass
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15819/bat-mtlp-6/igt@i915_selftest@live@workarounds.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142404v1/bat-mtlp-6/igt@i915_selftest@live@workarounds.html

  * igt@kms_cursor_legacy@basic-flip-before-cursor-atomic:
    - fi-kbl-7567u:       [DMESG-WARN][26] ([i915#12920]) -> [PASS][27]
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15819/fi-kbl-7567u/igt@kms_cursor_legacy@basic-flip-before-cursor-atomic.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142404v1/fi-kbl-7567u/igt@kms_cursor_legacy@basic-flip-before-cursor-atomic.html

  
#### Warnings ####

  * igt@gem_exec_gttfill@basic:
    - fi-pnv-d510:        [SKIP][28] -> [ABORT][29] ([i915#13169])
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15819/fi-pnv-d510/igt@gem_exec_gttfill@basic.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142404v1/fi-pnv-d510/igt@gem_exec_gttfill@basic.html

  
  [i915#11621]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11621
  [i915#12061]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12061
  [i915#12903]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12903
  [i915#12918]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12918
  [i915#12920]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12920
  [i915#12921]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12921
  [i915#13169]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13169
  [i915#9197]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9197


Build changes
-------------

  * Linux: CI_DRM_15819 -> Patchwork_142404v1

  CI-20190529: 20190529
  CI_DRM_15819: 03abf2f5f12e0f13a3a28bf70ccf2d88ec2a707b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_8147: df65b61f81a5cc919c10ff9c5ed516b45364135c @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_142404v1: 03abf2f5f12e0f13a3a28bf70ccf2d88ec2a707b @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142404v1/index.html

Re: [PATCH] drm/i915/display: use IS_ERR_OR_NULL macro on DP tunnel mgr creation failure

[Krzysztof Karas]

Thanks for review!

> > --- a/drivers/gpu/drm/i915/display/intel_dp_tunnel.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_tunnel.c
> > @@ -793,7 +793,7 @@ int intel_dp_tunnel_mgr_init(struct intel_display *display)
> >  	drm_connector_list_iter_end(&connector_list_iter);
> >  
> >  	tunnel_mgr = drm_dp_tunnel_mgr_create(display->drm, dp_connectors);
> > -	if (IS_ERR(tunnel_mgr))
> > +	if (IS_ERR_OR_NULL(tunnel_mgr))
> >  		return PTR_ERR(tunnel_mgr);
> 
> this still will not work as expected, since in case of NULL it will
> return 0 (success) instead of "a negative error code" as described in
> the documentation of the intel_dp_tunnel_mgr_init()
Good catch, we should not return 0 here then.

> 
> OTOH the documentation of drm_dp_tunnel_mgr_create() says: "Returns a
> pointer to the tunnel manager if created successfully or NULL in case of
> an error" so more appropriate fix seems to be:
> 
> -	if (IS_ERR(tunnel_mgr))
> - 		return PTR_ERR(tunnel_mgr);
> +	if (!tunnel_mgr)
> + 		return -ENOMEM;
> 
> but then it will not work with the drm_dp_tunnel_mgr_create() stub which
> actually returns undocumented ERR_PTR(-EOPNOTSUPP)
> 
> so unless you are ready to update implementation and documentation of
> the drm_dp_tunnel_mgr_create() to return ERR_PTR instead of NULL in case
> of error
I considered that and I think this would be overall a better solution,
but as I understand functions in drm_dp_tunnel.c file generally try to
return NULLs, whenever kzalloc/kcalloc fails to allocate, so we'd have
that one odd out here. Though, other ones are 'static', so maybe there
is no need for concern, as they are not going to be exposed.

I'll update drm_dp_tunnel_mgr_create() to return error pointers.

>, the fix IMO should look more like:
> 
> +	if (!tunnel_mgr)
> + 		return -ENOMEM;
> 
> and keep existing IS_ERR check
I do not think it is good to have the caller assume error code from just
a generic NULL. If anything changes in drm_dp_tunnel_mgr_create() and
something else than allocation would be allowed to fail, then ENOMEM
would no longer be appropriate here.


Krzysztof Karas

> 
> >  
> >  	display->dp_tunnel_mgr = tunnel_mgr;
> 

Re: [PATCH] drm/i915/display: use IS_ERR_OR_NULL macro on DP tunnel mgr creation failure

[Krzysztof Karas]

Thanks for review!

> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_tunnel.c b/drivers/gpu/drm/i915/display/intel_dp_tunnel.c
> > index 94198bc04939..6c960416f776 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_tunnel.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_tunnel.c
> > @@ -793,7 +793,7 @@ int intel_dp_tunnel_mgr_init(struct intel_display *display)
> >  	drm_connector_list_iter_end(&connector_list_iter);
> >  
> >  	tunnel_mgr = drm_dp_tunnel_mgr_create(display->drm, dp_connectors);
> > -	if (IS_ERR(tunnel_mgr))
> > +	if (IS_ERR_OR_NULL(tunnel_mgr))
> 
> nicely spotted, but the fix is wrong. drm_dp_tunnel_mgr_create()
> returns NULL, not an error, so that you can just check:
> 
> 	if (!tunnel_mgr)
> 		...
I thought about this too, but then that would ignore the return from
drm_dp_tunnel_mgr_create() stub in drm_dp_tunnel.h (the one returning
ERR_PTR(-ENOTSUPP) if CONFIG_DRM_DISPLAY_DP_TUNNEL is not enabled).

Krzysztof Karas
> 

Re: [PATCH] drm/i915/display: use IS_ERR_OR_NULL macro on DP tunnel mgr creation failure

[Imre Deak]

On Wed, Dec 11, 2024 at 09:56:50AM +0000, Krzysztof Karas wrote:
> drm_dp_tunnel_mgr_create() may return NULL on failure, which will not
> be caught via IS_ERR(), so replace it with IS_ERR_OR_NULL() macro.
> 
> Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_tunnel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_tunnel.c b/drivers/gpu/drm/i915/display/intel_dp_tunnel.c
> index 94198bc04939..6c960416f776 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_tunnel.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_tunnel.c
> @@ -793,7 +793,7 @@ int intel_dp_tunnel_mgr_init(struct intel_display *display)
>  	drm_connector_list_iter_end(&connector_list_iter);
>  
>  	tunnel_mgr = drm_dp_tunnel_mgr_create(display->drm, dp_connectors);
> -	if (IS_ERR(tunnel_mgr))
> +	if (IS_ERR_OR_NULL(tunnel_mgr))
>  		return PTR_ERR(tunnel_mgr);

Thanks for spotting this. As Michal pointed out, instead of the above
drm_dp_tunnel_mgr_create() should be fixed to return PTR_ERR(-ENOMEM) in
case of an error.

>  
>  	display->dp_tunnel_mgr = tunnel_mgr;
> -- 
> 2.34.1
>