| Message ID | 20180912082713.23294-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Differentiate the lack of an interface from invalid parameter | expand |
On Wed, Sep 12, 2018 at 10:27 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > If the ioctl is not supported on a particular piece of HW/driver > combination, report ENODEV so that it can be easily distinguished from > both the lack of the ioctl and from a regular invalid parameter. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Hm, I thought the canonical errno for "ioctl doesn't apply to this device" is ENOTTY? And ENODEV means that it applies, but atm nothing plugged in, or device gone already. I found a few more modeset ioctl: - drm_mode_gamma_set_ioctl, drm_mode_gamma_get_ioctl - drm_mode_getconnector - drm_mode_getcrtc, drm_mode_setcrtc - drm_mode_getencoder - drm_mode_create_lease_ioctl, drm_mode_list_lessees_ioctl, ... Ok now I stop looking through the grep thing, looks like we've been using EINVAL consistently. I'm all for switching, it makes sense, but I think we should at least try to be consistent across all kms ioctl. I'm happy to r-b such a series. Cheers, Daniel > --- > drivers/gpu/drm/drm_framebuffer.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > index 6eaacd4eb8cc..eed6ad0fe84a 100644 > --- a/drivers/gpu/drm/drm_framebuffer.c > +++ b/drivers/gpu/drm/drm_framebuffer.c > @@ -113,6 +113,9 @@ int drm_mode_addfb(struct drm_device *dev, struct drm_mode_fb_cmd *or, > struct drm_mode_fb_cmd2 r = {}; > int ret; > > + if (!drm_core_check_feature(dev, DRIVER_MODESET)) > + return -ENODEV; > + > r.pixel_format = drm_mode_legacy_fb_format(or->bpp, or->depth); > if (r.pixel_format == DRM_FORMAT_INVALID) { > DRM_DEBUG("bad {bpp:%d, depth:%d}\n", or->bpp, or->depth); > @@ -352,7 +355,7 @@ int drm_mode_addfb2(struct drm_device *dev, > struct drm_framebuffer *fb; > > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > - return -EINVAL; > + return -ENODEV; > > fb = drm_internal_framebuffer_create(dev, r, file_priv); > if (IS_ERR(fb)) > @@ -387,7 +390,7 @@ int drm_mode_addfb2_ioctl(struct drm_device *dev, > * ADDFB. > */ > DRM_DEBUG_KMS("addfb2 broken on bigendian"); > - return -EINVAL; > + return -ENODEV; > } > #endif > return drm_mode_addfb2(dev, data, file_priv); > @@ -432,7 +435,7 @@ int drm_mode_rmfb(struct drm_device *dev, u32 fb_id, > int found = 0; > > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > - return -EINVAL; > + return -ENODEV; > > fb = drm_framebuffer_lookup(dev, file_priv, fb_id); > if (!fb) > @@ -509,7 +512,7 @@ int drm_mode_getfb(struct drm_device *dev, > int ret; > > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > - return -EINVAL; > + return -ENODEV; > > fb = drm_framebuffer_lookup(dev, file_priv, r->fb_id); > if (!fb) > @@ -582,7 +585,7 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev, > int ret; > > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > - return -EINVAL; > + return -ENODEV; > > fb = drm_framebuffer_lookup(dev, file_priv, r->fb_id); > if (!fb) > -- > 2.19.0 >
Quoting Daniel Vetter (2018-09-12 09:39:30) > On Wed, Sep 12, 2018 at 10:27 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > If the ioctl is not supported on a particular piece of HW/driver > > combination, report ENODEV so that it can be easily distinguished from > > both the lack of the ioctl and from a regular invalid parameter. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Hm, I thought the canonical errno for "ioctl doesn't apply to this > device" is ENOTTY? That's ioctl doesn't exist. Sometimes it's nice to know the kernel knows about the ioctl but it isn't applicable. Either is fine for my purpose, which is to know the ioctl exists, I just need to distinguish it from EINVAL. > And ENODEV means that it applies, but atm nothing > plugged in, or device gone already. > > I found a few more modeset ioctl: > - drm_mode_gamma_set_ioctl, drm_mode_gamma_get_ioctl > - drm_mode_getconnector > - drm_mode_getcrtc, drm_mode_setcrtc > - drm_mode_getencoder > - drm_mode_create_lease_ioctl, drm_mode_list_lessees_ioctl, ... > > Ok now I stop looking through the grep thing, looks like we've been > using EINVAL consistently. I'm all for switching, it makes sense, but > I think we should at least try to be consistent across all kms ioctl. Ah, but we've been consistent on the other side and have been using ENODEV for the better part of a decade for this meaning (that the ioctl doesn't apply to this HW) :) -Chris
On Wed, Sep 12, 2018 at 10:50 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Daniel Vetter (2018-09-12 09:39:30) >> On Wed, Sep 12, 2018 at 10:27 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> > If the ioctl is not supported on a particular piece of HW/driver >> > combination, report ENODEV so that it can be easily distinguished from >> > both the lack of the ioctl and from a regular invalid parameter. >> > >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> > Cc: Daniel Vetter <daniel@ffwll.ch> >> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >> Hm, I thought the canonical errno for "ioctl doesn't apply to this >> device" is ENOTTY? > > That's ioctl doesn't exist. Sometimes it's nice to know the kernel knows > about the ioctl but it isn't applicable. Either is fine for my purpose, > which is to know the ioctl exists, I just need to distinguish it from > EINVAL. > >> And ENODEV means that it applies, but atm nothing >> plugged in, or device gone already. >> >> I found a few more modeset ioctl: >> - drm_mode_gamma_set_ioctl, drm_mode_gamma_get_ioctl >> - drm_mode_getconnector >> - drm_mode_getcrtc, drm_mode_setcrtc >> - drm_mode_getencoder >> - drm_mode_create_lease_ioctl, drm_mode_list_lessees_ioctl, ... >> >> Ok now I stop looking through the grep thing, looks like we've been >> using EINVAL consistently. I'm all for switching, it makes sense, but >> I think we should at least try to be consistent across all kms ioctl. > > Ah, but we've been consistent on the other side and have been using > ENODEV for the better part of a decade for this meaning (that the ioctl > doesn't apply to this HW) :) Hm indeed, we've been using either ENODEV or EINVAL for "this ioctl doesn't exist/doesn't apply". ENODEV is clearly used in many places (also outside of drm) for "hw absent/gone/disappeared". And we have very few uses of ENOTTY. So I'm kinda leaning towards starting a new trend here, and using ENOTTY for "this ioctl doesn't apply". I don't think we need to differentiate this from the case of "this kernel has no idea about this ioctl at all", since from a userspace pov there's no difference really: Either way it can't use it. But I'm also fine if we're just sticking to ENODEV, just feels like wasting a perfectly useful errno (and there's not many) if we don't give ENOTTY some more use. -Daniel
Quoting Daniel Vetter (2018-09-12 10:02:47) > On Wed, Sep 12, 2018 at 10:50 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Daniel Vetter (2018-09-12 09:39:30) > >> On Wed, Sep 12, 2018 at 10:27 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > >> > If the ioctl is not supported on a particular piece of HW/driver > >> > combination, report ENODEV so that it can be easily distinguished from > >> > both the lack of the ioctl and from a regular invalid parameter. > >> > > >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >> > Cc: Daniel Vetter <daniel@ffwll.ch> > >> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> > >> Hm, I thought the canonical errno for "ioctl doesn't apply to this > >> device" is ENOTTY? > > > > That's ioctl doesn't exist. Sometimes it's nice to know the kernel knows > > about the ioctl but it isn't applicable. Either is fine for my purpose, > > which is to know the ioctl exists, I just need to distinguish it from > > EINVAL. > > > >> And ENODEV means that it applies, but atm nothing > >> plugged in, or device gone already. > >> > >> I found a few more modeset ioctl: > >> - drm_mode_gamma_set_ioctl, drm_mode_gamma_get_ioctl > >> - drm_mode_getconnector > >> - drm_mode_getcrtc, drm_mode_setcrtc > >> - drm_mode_getencoder > >> - drm_mode_create_lease_ioctl, drm_mode_list_lessees_ioctl, ... > >> > >> Ok now I stop looking through the grep thing, looks like we've been > >> using EINVAL consistently. I'm all for switching, it makes sense, but > >> I think we should at least try to be consistent across all kms ioctl. > > > > Ah, but we've been consistent on the other side and have been using > > ENODEV for the better part of a decade for this meaning (that the ioctl > > doesn't apply to this HW) :) > > Hm indeed, we've been using either ENODEV or EINVAL for "this ioctl > doesn't exist/doesn't apply". ENODEV is clearly used in many places > (also outside of drm) for "hw absent/gone/disappeared". And we have > very few uses of ENOTTY. So I'm kinda leaning towards starting a new > trend here, and using ENOTTY for "this ioctl doesn't apply". I don't > think we need to differentiate this from the case of "this kernel has > no idea about this ioctl at all", since from a userspace pov there's > no difference really: Either way it can't use it. > > But I'm also fine if we're just sticking to ENODEV, just feels like > wasting a perfectly useful errno (and there's not many) if we don't > give ENOTTY some more use. In going through the drm_core_checks, I noticed one brave soul used -ENOTSUPP. How about that? -Chris
Am 12.09.2018 um 11:12 schrieb Chris Wilson: > Quoting Daniel Vetter (2018-09-12 10:02:47) >> On Wed, Sep 12, 2018 at 10:50 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >>> Quoting Daniel Vetter (2018-09-12 09:39:30) >>>> On Wed, Sep 12, 2018 at 10:27 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >>>>> If the ioctl is not supported on a particular piece of HW/driver >>>>> combination, report ENODEV so that it can be easily distinguished from >>>>> both the lack of the ioctl and from a regular invalid parameter. >>>>> >>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>>>> Cc: Daniel Vetter <daniel@ffwll.ch> >>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>>> Hm, I thought the canonical errno for "ioctl doesn't apply to this >>>> device" is ENOTTY? >>> That's ioctl doesn't exist. Sometimes it's nice to know the kernel knows >>> about the ioctl but it isn't applicable. Either is fine for my purpose, >>> which is to know the ioctl exists, I just need to distinguish it from >>> EINVAL. >>> >>>> And ENODEV means that it applies, but atm nothing >>>> plugged in, or device gone already. >>>> >>>> I found a few more modeset ioctl: >>>> - drm_mode_gamma_set_ioctl, drm_mode_gamma_get_ioctl >>>> - drm_mode_getconnector >>>> - drm_mode_getcrtc, drm_mode_setcrtc >>>> - drm_mode_getencoder >>>> - drm_mode_create_lease_ioctl, drm_mode_list_lessees_ioctl, ... >>>> >>>> Ok now I stop looking through the grep thing, looks like we've been >>>> using EINVAL consistently. I'm all for switching, it makes sense, but >>>> I think we should at least try to be consistent across all kms ioctl. >>> Ah, but we've been consistent on the other side and have been using >>> ENODEV for the better part of a decade for this meaning (that the ioctl >>> doesn't apply to this HW) :) >> Hm indeed, we've been using either ENODEV or EINVAL for "this ioctl >> doesn't exist/doesn't apply". ENODEV is clearly used in many places >> (also outside of drm) for "hw absent/gone/disappeared". And we have >> very few uses of ENOTTY. So I'm kinda leaning towards starting a new >> trend here, and using ENOTTY for "this ioctl doesn't apply". I don't >> think we need to differentiate this from the case of "this kernel has >> no idea about this ioctl at all", since from a userspace pov there's >> no difference really: Either way it can't use it. >> >> But I'm also fine if we're just sticking to ENODEV, just feels like >> wasting a perfectly useful errno (and there's not many) if we don't >> give ENOTTY some more use. > In going through the drm_core_checks, I noticed one brave soul used > -ENOTSUPP. How about that? +1 for that as well and I'm pretty sure I have seen that in a couple of drivers if an IOCTL isn't supported for a specific device. Christian. > -Chris > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 6eaacd4eb8cc..eed6ad0fe84a 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -113,6 +113,9 @@ int drm_mode_addfb(struct drm_device *dev, struct drm_mode_fb_cmd *or, struct drm_mode_fb_cmd2 r = {}; int ret; + if (!drm_core_check_feature(dev, DRIVER_MODESET)) + return -ENODEV; + r.pixel_format = drm_mode_legacy_fb_format(or->bpp, or->depth); if (r.pixel_format == DRM_FORMAT_INVALID) { DRM_DEBUG("bad {bpp:%d, depth:%d}\n", or->bpp, or->depth); @@ -352,7 +355,7 @@ int drm_mode_addfb2(struct drm_device *dev, struct drm_framebuffer *fb; if (!drm_core_check_feature(dev, DRIVER_MODESET)) - return -EINVAL; + return -ENODEV; fb = drm_internal_framebuffer_create(dev, r, file_priv); if (IS_ERR(fb)) @@ -387,7 +390,7 @@ int drm_mode_addfb2_ioctl(struct drm_device *dev, * ADDFB. */ DRM_DEBUG_KMS("addfb2 broken on bigendian"); - return -EINVAL; + return -ENODEV; } #endif return drm_mode_addfb2(dev, data, file_priv); @@ -432,7 +435,7 @@ int drm_mode_rmfb(struct drm_device *dev, u32 fb_id, int found = 0; if (!drm_core_check_feature(dev, DRIVER_MODESET)) - return -EINVAL; + return -ENODEV; fb = drm_framebuffer_lookup(dev, file_priv, fb_id); if (!fb) @@ -509,7 +512,7 @@ int drm_mode_getfb(struct drm_device *dev, int ret; if (!drm_core_check_feature(dev, DRIVER_MODESET)) - return -EINVAL; + return -ENODEV; fb = drm_framebuffer_lookup(dev, file_priv, r->fb_id); if (!fb) @@ -582,7 +585,7 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev, int ret; if (!drm_core_check_feature(dev, DRIVER_MODESET)) - return -EINVAL; + return -ENODEV; fb = drm_framebuffer_lookup(dev, file_priv, r->fb_id); if (!fb)
If the ioctl is not supported on a particular piece of HW/driver combination, report ENODEV so that it can be easily distinguished from both the lack of the ioctl and from a regular invalid parameter. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/drm_framebuffer.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)