[v2,01/14] video: fb_defio: preserve user fb_ops
diff mbox series

Message ID 022c82429da15d6450ff9ac1a897322ec3124db4.1575022735.git.jani.nikula@intel.com
State New
Headers show
Series
  • video, drm, etc: constify fbops in struct fb_info
Related show

Commit Message

Jani Nikula Nov. 29, 2019, 10:29 a.m. UTC
Modifying fb_ops directly to override fb_mmap with fb_deferred_io_mmap
and then resetting it to NULL afterwards causes problems all over the
place. First, it prevents making the fbops member of struct fb_info a
const pointer, which means we can't make struct fb_ops const
anywhere. Second, a few places have to go out of their way to restore
the original fb_mmap pointer that gets reset to NULL.

Since the only user of the fbops->fb_mmap hook is fb_mmap() in fbmem.c,
call fb_deferred_io_mmap() directly when deferred IO is enabled, and
avoid modifying fb_ops altogether.

Simply use info->fbdefio to determine whether deferred IO should be used
or not. This should be accurate enough for all use cases, although
perhaps not pedantically correct.

v2: Simplify considerably by calling fb_deferred_io_mmap() directly
    (Daniel, Ville)

Cc: Jaya Kumar <jayalk@intworks.biz>
Cc: linux-fbdev@vger.kernel.org
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/video/fbdev/core/fb_defio.c |  3 ---
 drivers/video/fbdev/core/fbmem.c    | 15 +++++++++++----
 2 files changed, 11 insertions(+), 7 deletions(-)

Comments

Noralf Trønnes Nov. 29, 2019, 2:12 p.m. UTC | #1
Den 29.11.2019 11.29, skrev Jani Nikula:
> Modifying fb_ops directly to override fb_mmap with fb_deferred_io_mmap
> and then resetting it to NULL afterwards causes problems all over the
> place. First, it prevents making the fbops member of struct fb_info a
> const pointer, which means we can't make struct fb_ops const
> anywhere. Second, a few places have to go out of their way to restore
> the original fb_mmap pointer that gets reset to NULL.
> 
> Since the only user of the fbops->fb_mmap hook is fb_mmap() in fbmem.c,
> call fb_deferred_io_mmap() directly when deferred IO is enabled, and
> avoid modifying fb_ops altogether.
> 
> Simply use info->fbdefio to determine whether deferred IO should be used
> or not. This should be accurate enough for all use cases, although
> perhaps not pedantically correct.
> 
> v2: Simplify considerably by calling fb_deferred_io_mmap() directly
>     (Daniel, Ville)
> 
> Cc: Jaya Kumar <jayalk@intworks.biz>
> Cc: linux-fbdev@vger.kernel.org
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---

Nice simple solution:

Acked-by: Noralf Trønnes <noralf@tronnes.org>
Daniel Vetter Nov. 29, 2019, 3:22 p.m. UTC | #2
On Fri, Nov 29, 2019 at 12:29:31PM +0200, Jani Nikula wrote:
> Modifying fb_ops directly to override fb_mmap with fb_deferred_io_mmap
> and then resetting it to NULL afterwards causes problems all over the
> place. First, it prevents making the fbops member of struct fb_info a
> const pointer, which means we can't make struct fb_ops const
> anywhere. Second, a few places have to go out of their way to restore
> the original fb_mmap pointer that gets reset to NULL.
> 
> Since the only user of the fbops->fb_mmap hook is fb_mmap() in fbmem.c,
> call fb_deferred_io_mmap() directly when deferred IO is enabled, and
> avoid modifying fb_ops altogether.
> 
> Simply use info->fbdefio to determine whether deferred IO should be used
> or not. This should be accurate enough for all use cases, although
> perhaps not pedantically correct.
> 
> v2: Simplify considerably by calling fb_deferred_io_mmap() directly
>     (Daniel, Ville)
> 
> Cc: Jaya Kumar <jayalk@intworks.biz>
> Cc: linux-fbdev@vger.kernel.org
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/video/fbdev/core/fb_defio.c |  3 ---
>  drivers/video/fbdev/core/fbmem.c    | 15 +++++++++++----
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> index 82c20c6047b0..a591d291b231 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -171,7 +171,6 @@ int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
>  	vma->vm_private_data = info;
>  	return 0;
>  }
> -EXPORT_SYMBOL(fb_deferred_io_mmap);
>  
>  /* workqueue callback */
>  static void fb_deferred_io_work(struct work_struct *work)
> @@ -206,7 +205,6 @@ void fb_deferred_io_init(struct fb_info *info)
>  
>  	BUG_ON(!fbdefio);
>  	mutex_init(&fbdefio->lock);
> -	info->fbops->fb_mmap = fb_deferred_io_mmap;
>  	INIT_DELAYED_WORK(&info->deferred_work, fb_deferred_io_work);
>  	INIT_LIST_HEAD(&fbdefio->pagelist);
>  	if (fbdefio->delay == 0) /* set a default of 1 s */
> @@ -237,7 +235,6 @@ void fb_deferred_io_cleanup(struct fb_info *info)
>  		page->mapping = NULL;
>  	}
>  
> -	info->fbops->fb_mmap = NULL;
>  	mutex_destroy(&fbdefio->lock);
>  }
>  EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 86b06a599f96..990550930a8e 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1332,16 +1332,23 @@ static int
>  fb_mmap(struct file *file, struct vm_area_struct * vma)
>  {
>  	struct fb_info *info = file_fb_info(file);
> -	struct fb_ops *fb;
> +	int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma);

Oh I function pointer for tidier control flow, neato.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  	unsigned long mmio_pgoff;
>  	unsigned long start;
>  	u32 len;
>  
>  	if (!info)
>  		return -ENODEV;
> -	fb = info->fbops;
>  	mutex_lock(&info->mm_lock);
> -	if (fb->fb_mmap) {
> +
> +	fb_mmap_fn = info->fbops->fb_mmap;
> +
> +#if IS_ENABLED(CONFIG_FB_DEFERRED_IO)
> +	if (info->fbdefio)
> +		fb_mmap_fn = fb_deferred_io_mmap;
> +#endif
> +
> +	if (fb_mmap_fn) {
>  		int res;
>  
>  		/*
> @@ -1349,7 +1356,7 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
>  		 * SME protection is removed ahead of the call
>  		 */
>  		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> -		res = fb->fb_mmap(info, vma);
> +		res = fb_mmap_fn(info, vma);
>  		mutex_unlock(&info->mm_lock);
>  		return res;
>  	}
> -- 
> 2.20.1
>

Patch
diff mbox series

diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index 82c20c6047b0..a591d291b231 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -171,7 +171,6 @@  int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
 	vma->vm_private_data = info;
 	return 0;
 }
-EXPORT_SYMBOL(fb_deferred_io_mmap);
 
 /* workqueue callback */
 static void fb_deferred_io_work(struct work_struct *work)
@@ -206,7 +205,6 @@  void fb_deferred_io_init(struct fb_info *info)
 
 	BUG_ON(!fbdefio);
 	mutex_init(&fbdefio->lock);
-	info->fbops->fb_mmap = fb_deferred_io_mmap;
 	INIT_DELAYED_WORK(&info->deferred_work, fb_deferred_io_work);
 	INIT_LIST_HEAD(&fbdefio->pagelist);
 	if (fbdefio->delay == 0) /* set a default of 1 s */
@@ -237,7 +235,6 @@  void fb_deferred_io_cleanup(struct fb_info *info)
 		page->mapping = NULL;
 	}
 
-	info->fbops->fb_mmap = NULL;
 	mutex_destroy(&fbdefio->lock);
 }
 EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 86b06a599f96..990550930a8e 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1332,16 +1332,23 @@  static int
 fb_mmap(struct file *file, struct vm_area_struct * vma)
 {
 	struct fb_info *info = file_fb_info(file);
-	struct fb_ops *fb;
+	int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma);
 	unsigned long mmio_pgoff;
 	unsigned long start;
 	u32 len;
 
 	if (!info)
 		return -ENODEV;
-	fb = info->fbops;
 	mutex_lock(&info->mm_lock);
-	if (fb->fb_mmap) {
+
+	fb_mmap_fn = info->fbops->fb_mmap;
+
+#if IS_ENABLED(CONFIG_FB_DEFERRED_IO)
+	if (info->fbdefio)
+		fb_mmap_fn = fb_deferred_io_mmap;
+#endif
+
+	if (fb_mmap_fn) {
 		int res;
 
 		/*
@@ -1349,7 +1356,7 @@  fb_mmap(struct file *file, struct vm_area_struct * vma)
 		 * SME protection is removed ahead of the call
 		 */
 		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
-		res = fb->fb_mmap(info, vma);
+		res = fb_mmap_fn(info, vma);
 		mutex_unlock(&info->mm_lock);
 		return res;
 	}