| Message ID | fw4bnfdbbolmg5zdzrf7raw5d7vzcxxz3zno3pti6tmnakrnvt@tx3262k6bzfs (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/display: use IS_ERR_OR_NULL macro on DP tunnel mgr creation failure | expand |
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
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;
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; >
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 >
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 >
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;
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(-)