| Message ID | 1423505008-15515-6-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 09, 2015 at 07:03:28PM +0100, Daniel Vetter wrote: > Just the usual paranoia ... > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_crtc.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index b15d720eda4c..a12d7e8a0ca0 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -3322,6 +3322,23 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r) > } > } > > + for (; i < 4; i++) { > + if (r->handles[i]) { > + DRM_DEBUG_KMS("buffer object handle for unused plane %d\n", i); Printing the invalid value is also useful. We tended to put user debugging messages as DRM_DEBUG(); would probably be useful to add DRM_DEBUG_USER() and clean up all the EINVAL reporting. -Chris
On Tue, Feb 10, 2015 at 11:01:56AM +0000, Chris Wilson wrote: > On Mon, Feb 09, 2015 at 07:03:28PM +0100, Daniel Vetter wrote: > > Just the usual paranoia ... > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/drm_crtc.c | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index b15d720eda4c..a12d7e8a0ca0 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -3322,6 +3322,23 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r) > > } > > } > > > > + for (; i < 4; i++) { > > + if (r->handles[i]) { > > + DRM_DEBUG_KMS("buffer object handle for unused plane %d\n", i); > > Printing the invalid value is also useful. We tended to put user > debugging messages as DRM_DEBUG(); would probably be useful to add > DRM_DEBUG_USER() and clean up all the EINVAL reporting. Generally I agree, but here I couldn't come up with a case where it would be useful: - Missing memset is just that. - Memset is there, but userspace filled in too many buffers - the i it prints should be enough. -Daniel
On Tue, Feb 10, 2015 at 12:36:51PM +0100, Daniel Vetter wrote: > On Tue, Feb 10, 2015 at 11:01:56AM +0000, Chris Wilson wrote: > > On Mon, Feb 09, 2015 at 07:03:28PM +0100, Daniel Vetter wrote: > > > Just the usual paranoia ... > > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > --- > > > drivers/gpu/drm/drm_crtc.c | 17 +++++++++++++++++ > > > 1 file changed, 17 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > > index b15d720eda4c..a12d7e8a0ca0 100644 > > > --- a/drivers/gpu/drm/drm_crtc.c > > > +++ b/drivers/gpu/drm/drm_crtc.c > > > @@ -3322,6 +3322,23 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r) > > > } > > > } > > > > > > + for (; i < 4; i++) { > > > + if (r->handles[i]) { > > > + DRM_DEBUG_KMS("buffer object handle for unused plane %d\n", i); > > > > Printing the invalid value is also useful. We tended to put user > > debugging messages as DRM_DEBUG(); would probably be useful to add > > DRM_DEBUG_USER() and clean up all the EINVAL reporting. > > Generally I agree, but here I couldn't come up with a case where it would > be useful: > - Missing memset is just that. > - Memset is there, but userspace filled in too many buffers - the i it > prints should be enough. The comment was more towards the future, when I expect the validity checking to be more stringent. For consistency we should use the same debug level as for other EINVAL logging, and when we come to cut and paste the error message, having the value in there should remind us to be verbose. Even here I expect the value to be useful diagnostic, e.g. to help narrow down the circumstances under which the invalid ioctl was called. Maybe it contains poison, maybe it is valid but stale etc. -Chris
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index b15d720eda4c..a12d7e8a0ca0 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3322,6 +3322,23 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r) } } + for (; i < 4; i++) { + if (r->handles[i]) { + DRM_DEBUG_KMS("buffer object handle for unused plane %d\n", i); + return -EINVAL; + } + + if (r->pitches[i] || r->offsets[i]) { + DRM_DEBUG_KMS("buffer pitch/offset for unused plane", i); + return -EINVAL; + } + + if (r->modifier[i]) { + DRM_DEBUG_KMS("fb modifer for unused plane", i); + return -EINVAL; + } + } + return 0; }
Just the usual paranoia ... Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_crtc.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)