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

[Javier Martinez Canillas <javierm@xxxxxxxxxx>]

On 11/2/22 11:33, Thomas Zimmermann wrote:

[...]

>>
>>> +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?
> 
> Yes. It emulates the original fbdev code at [1]. Further down in that 
> function, the position is being updated even if an error occured. We 
> only return the initial error if no bytes got written.
> 
> It could happen that some userspace program hits to error, but still 
> relies on the output and position being updated. IIRC I even added 
> validation of this behavior to the IGT fbdev tests.  I agree that this 
> is somewhat bogus behavior, but changing it would change long-standing 
> userspace semantics.
>

Thanks for the explanation, feel free then to also add to this patch:

Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat