Re: [PATCH v2 1/2] drm: extract closefb logic in separate function

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Oct 20, 2023 at 10:19:38AM +0000, Simon Ser wrote:
> drm_mode_rmfb performs two operations: drop the FB from the
> file_priv->fbs list, and make sure the FB is no longer used on a
> plane.
> 
> In the next commit an IOCTL which only does so former will be
> introduced, so let's split it into a separate function.
> 
> No functional change, only refactoring.
> 
> v2: no change
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Dennis Filder <d.filder@web.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Daniel Stone <daniels@collabora.com>
> ---
>  drivers/gpu/drm/drm_framebuffer.c | 51 +++++++++++++++++++------------
>  1 file changed, 31 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index d3ba0698b84b..62306196808c 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -394,6 +394,31 @@ static void drm_mode_rmfb_work_fn(struct work_struct *w)
>  	}
>  }
>  
> +static int drm_mode_closefb(struct drm_framebuffer *fb,
> +			    struct drm_file *file_priv)
> +{
> +	struct drm_framebuffer *fbl = NULL;

The initialization confused me a bit. Made me expect this to
get used somehow after the loop has finished. But now I see
it's entirely redundant, and was already there in the original.

> +	bool found = false;
> +
> +	mutex_lock(&file_priv->fbs_lock);
> +	list_for_each_entry(fbl, &file_priv->fbs, filp_head)
> +		if (fb == fbl)
> +			found = true;
> +
> +	if (!found) {
> +		mutex_unlock(&file_priv->fbs_lock);
> +		return -ENOENT;
> +	}
> +
> +	list_del_init(&fb->filp_head);
> +	mutex_unlock(&file_priv->fbs_lock);
> +
> +	/* Drop the reference that was stored in the fbs list */
> +	drm_framebuffer_put(fb);
> +
> +	return 0;
> +}
> +
>  /**
>   * drm_mode_rmfb - remove an FB from the configuration
>   * @dev: drm device
> @@ -411,8 +436,7 @@ int drm_mode_rmfb(struct drm_device *dev, u32 fb_id,
>  		  struct drm_file *file_priv)
>  {
>  	struct drm_framebuffer *fb = NULL;

This initialization also looks redundant.

Could drop them already in this patch I think, or as a followup.

> -	struct drm_framebuffer *fbl = NULL;
> -	int found = 0;
> +	int ret;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EOPNOTSUPP;
> @@ -421,23 +445,14 @@ int drm_mode_rmfb(struct drm_device *dev, u32 fb_id,
>  	if (!fb)
>  		return -ENOENT;
>  
> -	mutex_lock(&file_priv->fbs_lock);
> -	list_for_each_entry(fbl, &file_priv->fbs, filp_head)
> -		if (fb == fbl)
> -			found = 1;
> -	if (!found) {
> -		mutex_unlock(&file_priv->fbs_lock);
> -		goto fail_unref;
> +	ret = drm_mode_closefb(fb, file_priv);
> +	if (ret != 0) {
> +		drm_framebuffer_put(fb);
> +		return ret;
>  	}
>  
> -	list_del_init(&fb->filp_head);
> -	mutex_unlock(&file_priv->fbs_lock);
> -
> -	/* drop the reference we picked up in framebuffer lookup */
> -	drm_framebuffer_put(fb);
> -
>  	/*
> -	 * we now own the reference that was stored in the fbs list
> +	 * We now own the reference we picked up in drm_framebuffer_lookup.

I think it should be fairly obvious that you own the reference
from any lookup, so this part of the comment feels redundant now.

Anyways, the patch looks correct to me
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  	 *
>  	 * drm_framebuffer_remove may fail with -EINTR on pending signals,
>  	 * so run this in a separate stack as there's no way to correctly
> @@ -457,10 +472,6 @@ int drm_mode_rmfb(struct drm_device *dev, u32 fb_id,
>  		drm_framebuffer_put(fb);
>  
>  	return 0;
> -
> -fail_unref:
> -	drm_framebuffer_put(fb);
> -	return -ENOENT;
>  }
>  
>  int drm_mode_rmfb_ioctl(struct drm_device *dev,
> -- 
> 2.42.0
> 

-- 
Ville Syrjälä
Intel