* Re: [PATCH] drm/i915/display: use IS_ERR_OR_NULL macro on DP tunnel mgr creation failure
2024-12-11 9:56 [PATCH] drm/i915/display: use IS_ERR_OR_NULL macro on DP tunnel mgr creation failure Krzysztof Karas
@ 2024-12-11 10:31 ` Andi Shyti
2024-12-11 12:26 ` Krzysztof Karas
2024-12-11 10:38 ` Michal Wajdeczko
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Andi Shyti @ 2024-12-11 10:31 UTC (permalink / raw)
To: Krzysztof Karas; +Cc: intel-gfx, Jani Nikula, Rodrigo Vivi
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] drm/i915/display: use IS_ERR_OR_NULL macro on DP tunnel mgr creation failure
2024-12-11 10:31 ` Andi Shyti
@ 2024-12-11 12:26 ` Krzysztof Karas
0 siblings, 0 replies; 7+ messages in thread
From: Krzysztof Karas @ 2024-12-11 12:26 UTC (permalink / raw)
To: Andi Shyti; +Cc: intel-gfx, Jani Nikula, Rodrigo Vivi
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915/display: use IS_ERR_OR_NULL macro on DP tunnel mgr creation failure
2024-12-11 9:56 [PATCH] drm/i915/display: use IS_ERR_OR_NULL macro on DP tunnel mgr creation failure Krzysztof Karas
2024-12-11 10:31 ` Andi Shyti
@ 2024-12-11 10:38 ` Michal Wajdeczko
2024-12-11 12:21 ` Krzysztof Karas
2024-12-11 12:04 ` ✗ i915.CI.BAT: failure for " Patchwork
2024-12-11 13:01 ` [PATCH] " Imre Deak
3 siblings, 1 reply; 7+ messages in thread
From: Michal Wajdeczko @ 2024-12-11 10:38 UTC (permalink / raw)
To: Krzysztof Karas, intel-gfx; +Cc: Jani Nikula, Rodrigo Vivi
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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915/display: use IS_ERR_OR_NULL macro on DP tunnel mgr creation failure
2024-12-11 10:38 ` Michal Wajdeczko
@ 2024-12-11 12:21 ` Krzysztof Karas
0 siblings, 0 replies; 7+ messages in thread
From: Krzysztof Karas @ 2024-12-11 12:21 UTC (permalink / raw)
To: Michal Wajdeczko; +Cc: intel-gfx, Jani Nikula, Rodrigo Vivi
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;
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* ✗ i915.CI.BAT: failure for drm/i915/display: use IS_ERR_OR_NULL macro on DP tunnel mgr creation failure
2024-12-11 9:56 [PATCH] drm/i915/display: use IS_ERR_OR_NULL macro on DP tunnel mgr creation failure Krzysztof Karas
2024-12-11 10:31 ` Andi Shyti
2024-12-11 10:38 ` Michal Wajdeczko
@ 2024-12-11 12:04 ` Patchwork
2024-12-11 13:01 ` [PATCH] " Imre Deak
3 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2024-12-11 12:04 UTC (permalink / raw)
To: Krzysztof Karas; +Cc: intel-gfx
== 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] drm/i915/display: use IS_ERR_OR_NULL macro on DP tunnel mgr creation failure
2024-12-11 9:56 [PATCH] drm/i915/display: use IS_ERR_OR_NULL macro on DP tunnel mgr creation failure Krzysztof Karas
` (2 preceding siblings ...)
2024-12-11 12:04 ` ✗ i915.CI.BAT: failure for " Patchwork
@ 2024-12-11 13:01 ` Imre Deak
3 siblings, 0 replies; 7+ messages in thread
From: Imre Deak @ 2024-12-11 13:01 UTC (permalink / raw)
To: Krzysztof Karas; +Cc: intel-gfx, Jani Nikula, Rodrigo Vivi
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread