Re: [PATCH V4] drm/drm_vblank: Change EINVAL by the correct errno

[Daniel Vetter]

On Wed, Jun 19, 2019 at 09:48:56AM +0200, Daniel Vetter wrote:
> On Tue, Jun 18, 2019 at 11:07:50PM -0300, Rodrigo Siqueira wrote:
> > For historical reason, the function drm_wait_vblank_ioctl always return
> > -EINVAL if something gets wrong. This scenario limits the flexibility
> > for the userspace make detailed verification of the problem and take
> > some action. In particular, the validation of âif (!dev->irq_enabled)â
> > in the drm_wait_vblank_ioctl is responsible for checking if the driver
> > support vblank or not. If the driver does not support VBlank, the
> > function drm_wait_vblank_ioctl returns EINVAL which does not represent
> > the real issue; this patch changes this behavior by return EOPNOTSUPP.
> > Additionally, some operations are unsupported by this function, and
> > returns EINVAL; this patch also changes the return value to EOPNOTSUPP
> > in this case. Lastly, the function drm_wait_vblank_ioctl is invoked by
> > libdrm, which is used by many compositors; because of this, it is
> > important to check if this change breaks any compositor. In this sense,
> > the following projects were examined:
> > 
> > * Drm-hwcomposer
> > * Kwin
> > * Sway
> > * Wlroots
> > * Wayland-core
> > * Weston
> > * Xorg (67 different drivers)
> > 
> > For each repository the verification happened in three steps:
> > 
> > * Update the main branch
> > * Look for any occurrence "drmWaitVBlank" with the command:
> >   git grep -n "drmWaitVBlank"
> > * Look in the git history of the project with the command:
> >   git log -SdrmWaitVBlank
> > 
> > Finally, none of the above projects validate the use of EINVAL which
> > make safe, at least for these projects, to change the return values.
> > 
> > Change since V3:
> >  - Return EINVAL for _DRM_VBLANK_SIGNAL (Daniel)
> > 
> > Change since V2:
> >  Daniel Vetter and Chris Wilson
> >  - Replace ENOTTY by EOPNOTSUPP
> >  - Return EINVAL if the parameters are wrong
> > 
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> 
> Apologies for the confusion on the last time around. btw if someone tells
> you "r-b (or a-b) with these changes", then just apply the r-b/a-b tag
> next time around. Otherwise people will re-review the same thing over and
> over again.

btw when resending patches it's good practice to add anyone who commented
on it (or who commented on the igt test for the same patch and other way
round) onto the explicit Cc: list of the patch. That way it's easier for
them to follow the patch evolution and do a quick r-b once they're happy.

If you don't do that then much bigger chances your patch gets ignored.
-Daniel
> 
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/drm_vblank.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > index 603ab105125d..bed233361614 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -1582,7 +1582,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
> >  	unsigned int flags, pipe, high_pipe;
> >  
> >  	if (!dev->irq_enabled)
> > -		return -EINVAL;
> > +		return -EOPNOTSUPP;
> >  
> >  	if (vblwait->request.type & _DRM_VBLANK_SIGNAL)
> >  		return -EINVAL;
> > -- 
> > 2.21.0
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch