Re: [PATCH v2 17/21] drm/fb-helper: Perform all fbdev I/O with the same implementation

[Javier Martinez Canillas <javierm@xxxxxxxxxx>]

On 10/24/22 13:19, Thomas Zimmermann wrote:
> Implement the fbdev's read/write helpers with the same functions. Use
> the generic fbdev's code as template. Convert all drivers.
> 
> DRM's fb helpers must implement regular I/O functionality in struct
> fb_ops and possibly perform a damage update. Handle all this in the
> same functions and convert drivers. The functionality has been used
> as part of the generic fbdev code for some time. The drivers don't
> set struct drm_fb_helper.fb_dirty, so they will not be affected by
> damage handling.
> 
> For I/O memory, fb helpers now provide drm_fb_helper_cfb_read() and
> drm_fb_helper_cfb_write(). Several drivers require these. Until now
> tegra used I/O read and write, although the memory buffer appears to
> be in system memory. So use _sys_ helpers now.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> ---

[...]

> +static ssize_t __drm_fb_helper_write(struct fb_info *info, const char __user 
> *buf, size_t count,
> +                                  loff_t *ppos, drm_fb_helper_write_screen 
> write_screen)
> +{

[...]

> +     /*
> +      * Copy to framebuffer even if we already logged an error. Emulates
> +      * the behavior of the original fbdev implementation.
> +      */
> +     ret = write_screen(info, buf, count, pos);
> +     if (ret < 0)
> +             return ret; /* return last error, if any */
> +     else if (!ret)
> +             return err; /* return previous error, if any */
> +
> +     *ppos += ret;
> +

Should *ppos be incremented even if the previous error is returned?

The write_screen() succeeded anyways, even when the count written was
smaller than what the caller asked for.

>  /**
> - * drm_fb_helper_sys_read - wrapper around fb_sys_read
> + * drm_fb_helper_sys_read - Implements struct &fb_ops.fb_read for system 
> memory
>   * @info: fb_info struct pointer
>   * @buf: userspace buffer to read from framebuffer memory
>   * @count: number of bytes to read from framebuffer memory
>   * @ppos: read offset within framebuffer memory
>   *
> - * A wrapper around fb_sys_read implemented by fbdev core
> + * Returns:
> + * The number of read bytes on success, or an error code otherwise.
>   */

This sentence sounds a little bit off to me. Shouldn't be "number of bytes read"
instead? I'm not a native English speaker though, so feel free to just ignore 
me.

[...]

>  
> +static ssize_t fb_read_screen_base(struct fb_info *info, char __user *buf, 
> size_t count,
> +                                loff_t pos)
> +{
> +     const char __iomem *src = info->screen_base + pos;
> +     size_t alloc_size = min_t(size_t, count, PAGE_SIZE);
> +     ssize_t ret = 0;
> +     int err = 0;

Do you really need these two? AFAIK ssize_t is a signed type
so you can just use the ret variable to store and return the
errno value.

[...]

> +static ssize_t fb_write_screen_base(struct fb_info *info, const char __user 
> *buf, size_t count,
> +                                 loff_t pos)
> +{
> +     char __iomem *dst = info->screen_base + pos;
> +     size_t alloc_size = min_t(size_t, count, PAGE_SIZE);
> +     ssize_t ret = 0;
> +     int err = 0;

Same here.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat