| Message ID | x46u4zhhpnxgohyguhqsc4d73sbjwipebxp5uiwkopejsy6dpz@v3eysonfbmk2 (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] drm/display: use ERR_PTR on DP tunnel manager creation fail | expand |
Hi Krzysztof, On Wed, Dec 11, 2024 at 02:52:20PM +0000, Krzysztof Karas wrote: > Instead of returning a generic NULL on error from drm_dp_tunnel_mgr_create(), > use error pointers with informative codes. This will also trigger IS_ERR() in > current caller (intel_dp_tunnerl_mgr_init()) instead of bypassing it via NULL > pointer. I was about to suggest fixing either this or his counterpart in the header file. Please send this in a series along with the previous patch in order to let people understand why you are sending this. Besides, I think you can improve the explanation here, which is the different behaviour of drm_dp_tunnel_mgr_create() depending on the CONFIG_DRM_DISPLAY_DP_TUNNEL config flag. You can add to both of them my r-b. Did you check who is maintaining this file? Andi > v2: use error codes inside drm_dp_tunnel_mgr_create() instead of handling > on caller's side (Michal, Imre) > > Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com> > --- > drivers/gpu/drm/display/drm_dp_tunnel.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/display/drm_dp_tunnel.c b/drivers/gpu/drm/display/drm_dp_tunnel.c > index 48b2df120086..90fe07a89260 100644 > --- a/drivers/gpu/drm/display/drm_dp_tunnel.c > +++ b/drivers/gpu/drm/display/drm_dp_tunnel.c > @@ -1896,8 +1896,8 @@ static void destroy_mgr(struct drm_dp_tunnel_mgr *mgr) > * > * Creates a DP tunnel manager for @dev. > * > - * Returns a pointer to the tunnel manager if created successfully or NULL in > - * case of an error. > + * Returns a pointer to the tunnel manager if created successfully or error > + * pointer in case of failure. > */ > struct drm_dp_tunnel_mgr * > drm_dp_tunnel_mgr_create(struct drm_device *dev, int max_group_count) > @@ -1907,7 +1907,7 @@ drm_dp_tunnel_mgr_create(struct drm_device *dev, int max_group_count) > > mgr = kzalloc(sizeof(*mgr), GFP_KERNEL); > if (!mgr) > - return NULL; > + return ERR_PTR(-ENOMEM); > > mgr->dev = dev; > init_waitqueue_head(&mgr->bw_req_queue); > @@ -1916,7 +1916,7 @@ drm_dp_tunnel_mgr_create(struct drm_device *dev, int max_group_count) > if (!mgr->groups) { > kfree(mgr); > > - return NULL; > + return ERR_PTR(-ENOMEM); > } > > #ifdef CONFIG_DRM_DISPLAY_DP_TUNNEL_STATE_DEBUG > @@ -1927,7 +1927,7 @@ drm_dp_tunnel_mgr_create(struct drm_device *dev, int max_group_count) > if (!init_group(mgr, &mgr->groups[i])) { > destroy_mgr(mgr); > > - return NULL; > + return ERR_PTR(-ENOMEM); > } > > mgr->group_count++; > -- > 2.34.1
On Wed, Dec 11, 2024 at 04:18:06PM +0100, Andi Shyti wrote: > Hi Krzysztof, > > On Wed, Dec 11, 2024 at 02:52:20PM +0000, Krzysztof Karas wrote: > > Instead of returning a generic NULL on error from drm_dp_tunnel_mgr_create(), > > use error pointers with informative codes. This will also trigger IS_ERR() in > > current caller (intel_dp_tunnerl_mgr_init()) instead of bypassing it via NULL > > pointer. > > I was about to suggest fixing either this or his counterpart in > the header file. > > Please send this in a series along with the previous patch in > order to let people understand why you are sending this. Sorry, this patch works without the other, I got confused. It's fine, then: Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Thanks, Andi
Hi Krzysztof, On Wed, Dec 11, 2024 at 02:52:20PM +0000, Krzysztof Karas wrote: > Instead of returning a generic NULL on error from drm_dp_tunnel_mgr_create(), > use error pointers with informative codes. This will also trigger IS_ERR() in > current caller (intel_dp_tunnerl_mgr_init()) instead of bypassing it via NULL > pointer. > > v2: use error codes inside drm_dp_tunnel_mgr_create() instead of handling > on caller's side (Michal, Imre) > > Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com> Please: 1. run checkpatch.pl before sending patches. 2. don't ignore previous reviews: I asked you to add the fixes tag and two Cc 3. run checkpatch.pl before sending patches. 4. Because this is a fix you should Cc also the stable mailing list (for real!). 5. Find the proper maintainers to Cc, this patch is outside Intel's territory; consider using, but not strictly, get_maintainer.pl And, in case you forget: 6. run checkpatch.pl before sending patches. Looking forward for a v3. Andi
> Please: > > 1. run checkpatch.pl before sending patches. > 2. don't ignore previous reviews: I asked you to add the fixes > tag and two Cc > 3. run checkpatch.pl before sending patches. > 4. Because this is a fix you should Cc also the stable mailing > list (for real!). > 5. Find the proper maintainers to Cc, this patch is outside > Intel's territory; consider using, but not strictly, > get_maintainer.pl > I did overlook these, didn't I? > And, in case you forget: > > 6. run checkpatch.pl before sending patches. > > Looking forward for a v3. Will do. Krzysztof > > Andi
diff --git a/drivers/gpu/drm/display/drm_dp_tunnel.c b/drivers/gpu/drm/display/drm_dp_tunnel.c index 48b2df120086..90fe07a89260 100644 --- a/drivers/gpu/drm/display/drm_dp_tunnel.c +++ b/drivers/gpu/drm/display/drm_dp_tunnel.c @@ -1896,8 +1896,8 @@ static void destroy_mgr(struct drm_dp_tunnel_mgr *mgr) * * Creates a DP tunnel manager for @dev. * - * Returns a pointer to the tunnel manager if created successfully or NULL in - * case of an error. + * Returns a pointer to the tunnel manager if created successfully or error + * pointer in case of failure. */ struct drm_dp_tunnel_mgr * drm_dp_tunnel_mgr_create(struct drm_device *dev, int max_group_count) @@ -1907,7 +1907,7 @@ drm_dp_tunnel_mgr_create(struct drm_device *dev, int max_group_count) mgr = kzalloc(sizeof(*mgr), GFP_KERNEL); if (!mgr) - return NULL; + return ERR_PTR(-ENOMEM); mgr->dev = dev; init_waitqueue_head(&mgr->bw_req_queue); @@ -1916,7 +1916,7 @@ drm_dp_tunnel_mgr_create(struct drm_device *dev, int max_group_count) if (!mgr->groups) { kfree(mgr); - return NULL; + return ERR_PTR(-ENOMEM); } #ifdef CONFIG_DRM_DISPLAY_DP_TUNNEL_STATE_DEBUG @@ -1927,7 +1927,7 @@ drm_dp_tunnel_mgr_create(struct drm_device *dev, int max_group_count) if (!init_group(mgr, &mgr->groups[i])) { destroy_mgr(mgr); - return NULL; + return ERR_PTR(-ENOMEM); } mgr->group_count++;
Instead of returning a generic NULL on error from drm_dp_tunnel_mgr_create(), use error pointers with informative codes. This will also trigger IS_ERR() in current caller (intel_dp_tunnerl_mgr_init()) instead of bypassing it via NULL pointer. v2: use error codes inside drm_dp_tunnel_mgr_create() instead of handling on caller's side (Michal, Imre) Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com> --- drivers/gpu/drm/display/drm_dp_tunnel.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)