| Message ID | 20191023144953.28190-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/property: Enforce more lifetime rules | expand |
On Wed, Oct 23, 2019 at 04:49:52PM +0200, Daniel Vetter wrote: > Properties can't be attached after registering, userspace would get > confused (no one bothers to reprobe really). > > - Add kerneldoc > - Enforce this with some checks. This needs a somewhat ugly check > since connectors can be added later on, but we still need to attach > all properties before they go public. > > Note that we already enforce that properties themselves are created > before the entire device is registered. > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Rajat Jain <rajatja@google.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_mode_object.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c > index 6a23e36ed4fe..35c2719407a8 100644 > --- a/drivers/gpu/drm/drm_mode_object.c > +++ b/drivers/gpu/drm/drm_mode_object.c > @@ -224,12 +224,26 @@ EXPORT_SYMBOL(drm_mode_object_get); > * This attaches the given property to the modeset object with the given initial > * value. Currently this function cannot fail since the properties are stored in > * a statically sized array. > + * > + * Note that all properties must be attached before the object itself is > + * registered and accessible from userspace. > */ > void drm_object_attach_property(struct drm_mode_object *obj, > struct drm_property *property, > uint64_t init_val) > { > int count = obj->properties->count; > + struct drm_device *dev = property->dev; > + > + > + if (obj->type == DRM_MODE_OBJECT_CONNECTOR) { > + struct drm_connector *connector = obj_to_connector(obj); > + > + WARN_ON(!dev->driver->load && > + connector->registration_state == DRM_CONNECTOR_REGISTERED); > + } else { > + WARN_ON(!dev->driver->load && dev->registered); > + } I'm not sure I understand why dev->driver->load needs to be a special case. Don't the same rules apply for those drivers as well? Thierry > > if (count == DRM_OBJECT_MAX_PROPERTY) { > WARN(1, "Failed to attach object property (type: 0x%x). Please " > -- > 2.23.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Oct 24, 2019 at 12:40:55PM +0200, Thierry Reding wrote: > On Wed, Oct 23, 2019 at 04:49:52PM +0200, Daniel Vetter wrote: > > Properties can't be attached after registering, userspace would get > > confused (no one bothers to reprobe really). > > > > - Add kerneldoc > > - Enforce this with some checks. This needs a somewhat ugly check > > since connectors can be added later on, but we still need to attach > > all properties before they go public. > > > > Note that we already enforce that properties themselves are created > > before the entire device is registered. > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: Rajat Jain <rajatja@google.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/drm_mode_object.c | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c > > index 6a23e36ed4fe..35c2719407a8 100644 > > --- a/drivers/gpu/drm/drm_mode_object.c > > +++ b/drivers/gpu/drm/drm_mode_object.c > > @@ -224,12 +224,26 @@ EXPORT_SYMBOL(drm_mode_object_get); > > * This attaches the given property to the modeset object with the given initial > > * value. Currently this function cannot fail since the properties are stored in > > * a statically sized array. > > + * > > + * Note that all properties must be attached before the object itself is > > + * registered and accessible from userspace. > > */ > > void drm_object_attach_property(struct drm_mode_object *obj, > > struct drm_property *property, > > uint64_t init_val) > > { > > int count = obj->properties->count; > > + struct drm_device *dev = property->dev; > > + > > + > > + if (obj->type == DRM_MODE_OBJECT_CONNECTOR) { > > + struct drm_connector *connector = obj_to_connector(obj); > > + > > + WARN_ON(!dev->driver->load && > > + connector->registration_state == DRM_CONNECTOR_REGISTERED); > > + } else { > > + WARN_ON(!dev->driver->load && dev->registered); > > + } > > I'm not sure I understand why dev->driver->load needs to be a special > case. Don't the same rules apply for those drivers as well? Nevermind, I just noticed that drm_dev_register() sets dev->registered to true before calling the driver's ->load() implementation, so makes sense: Reviewed-by: Thierry Reding <treding@nvidia.com>
On Thu, Oct 24, 2019 at 12:43 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Thu, Oct 24, 2019 at 12:40:55PM +0200, Thierry Reding wrote: > > On Wed, Oct 23, 2019 at 04:49:52PM +0200, Daniel Vetter wrote: > > > Properties can't be attached after registering, userspace would get > > > confused (no one bothers to reprobe really). > > > > > > - Add kerneldoc > > > - Enforce this with some checks. This needs a somewhat ugly check > > > since connectors can be added later on, but we still need to attach > > > all properties before they go public. > > > > > > Note that we already enforce that properties themselves are created > > > before the entire device is registered. > > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > Cc: Rajat Jain <rajatja@google.com> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > --- > > > drivers/gpu/drm/drm_mode_object.c | 14 ++++++++++++++ > > > 1 file changed, 14 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c > > > index 6a23e36ed4fe..35c2719407a8 100644 > > > --- a/drivers/gpu/drm/drm_mode_object.c > > > +++ b/drivers/gpu/drm/drm_mode_object.c > > > @@ -224,12 +224,26 @@ EXPORT_SYMBOL(drm_mode_object_get); > > > * This attaches the given property to the modeset object with the given initial > > > * value. Currently this function cannot fail since the properties are stored in > > > * a statically sized array. > > > + * > > > + * Note that all properties must be attached before the object itself is > > > + * registered and accessible from userspace. > > > */ > > > void drm_object_attach_property(struct drm_mode_object *obj, > > > struct drm_property *property, > > > uint64_t init_val) > > > { > > > int count = obj->properties->count; > > > + struct drm_device *dev = property->dev; > > > + > > > + > > > + if (obj->type == DRM_MODE_OBJECT_CONNECTOR) { > > > + struct drm_connector *connector = obj_to_connector(obj); > > > + > > > + WARN_ON(!dev->driver->load && > > > + connector->registration_state == DRM_CONNECTOR_REGISTERED); > > > + } else { > > > + WARN_ON(!dev->driver->load && dev->registered); > > > + } > > > > I'm not sure I understand why dev->driver->load needs to be a special > > case. Don't the same rules apply for those drivers as well? > > Nevermind, I just noticed that drm_dev_register() sets dev->registered > to true before calling the driver's ->load() implementation, so makes > sense: We tried to be clever with this already before, see: commit e0f32f78e51b9989ee89f608fd0dd10e9c230652 (tag: drm-misc-next-fixes-2019-09-18) Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Tue Sep 17 14:09:35 2019 +0200 drm/kms: Duct-tape for mode object lifetime checks commit 4f5368b5541a902f6596558b05f5c21a9770dd32 Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Fri Jun 14 08:17:23 2019 +0200 drm/kms: Catch mode_object lifetime errors uncovered a bit a mess in dp drivers. Most drivers (from a quick look, all except i915) register all the dp stuff in their init code, which is too early. With CONFIG_DRM_DP_AUX_CHARDEV this will blow up, because drm_dp_aux_register tries to add a child to a device in sysfs (the connector) which doesn't even exist yet. No one seems to have cared thus far. But with the above change I also moved the setting of dev->registered after the ->load callback, in an attempt to keep old drivers from hitting any WARN_ON backtraces. But that moved radeon.ko from the "working, by accident" to "now also broken" category. Since this is a huge mess I figured a revert would be simplest. But this check has already caught issues in i915: commit 1b9bd09630d4db4827cc04d358a41a16a6bc2cb0 Author: Ville Syrjälä <ville.syrjala@linux.intel.com> Date: Tue Aug 20 19:16:57 2019 +0300 drm/i915: Do not create a new max_bpc prop for MST connectors Hence I'd like to retain it. Fix the radeon regression by moving the setting of dev->registered back to were it was, and stop the backtraces with an explicit check for dev->driver->load. Everyone else will stay as broken with CONFIG_DRM_DP_AUX_CHARDEV. The next patch will improve the kerneldoc and add a todo entry for this. Fixes: 4f5368b5541a ("drm/kms: Catch mode_object lifetime errors") Cc: Sean Paul <sean@poorly.run> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Reported-by: Michel Dänzer <michel@daenzer.net> Reviewed-by: Michel Dänzer <mdaenzer@redhat.com> Tested-by: Michel Dänzer <mdaenzer@redhat.com> Cc: Michel Dänzer <michel@daenzer.net> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190917120936.7501-1-daniel.vetter@ffwll.ch> I think I'll add a reference to the above to the commit message when applying. > Reviewed-by: Thierry Reding <treding@nvidia.com> Thanks for taking a look. -Daniel
On Wed, 23 Oct 2019, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > Properties can't be attached after registering, userspace would get > confused (no one bothers to reprobe really). > > - Add kerneldoc > - Enforce this with some checks. This needs a somewhat ugly check > since connectors can be added later on, but we still need to attach > all properties before they go public. > > Note that we already enforce that properties themselves are created > before the entire device is registered. > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Rajat Jain <rajatja@google.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_mode_object.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c > index 6a23e36ed4fe..35c2719407a8 100644 > --- a/drivers/gpu/drm/drm_mode_object.c > +++ b/drivers/gpu/drm/drm_mode_object.c > @@ -224,12 +224,26 @@ EXPORT_SYMBOL(drm_mode_object_get); > * This attaches the given property to the modeset object with the given initial > * value. Currently this function cannot fail since the properties are stored in > * a statically sized array. > + * > + * Note that all properties must be attached before the object itself is > + * registered and accessible from userspace. > */ > void drm_object_attach_property(struct drm_mode_object *obj, > struct drm_property *property, > uint64_t init_val) > { > int count = obj->properties->count; > + struct drm_device *dev = property->dev; > + > + Superfluous newline, with that fixed, Reviewed-by: Jani Nikula <jani.nikula@intel.com> > + if (obj->type == DRM_MODE_OBJECT_CONNECTOR) { > + struct drm_connector *connector = obj_to_connector(obj); > + > + WARN_ON(!dev->driver->load && > + connector->registration_state == DRM_CONNECTOR_REGISTERED); > + } else { > + WARN_ON(!dev->driver->load && dev->registered); > + } > > if (count == DRM_OBJECT_MAX_PROPERTY) { > WARN(1, "Failed to attach object property (type: 0x%x). Please "
diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c index 6a23e36ed4fe..35c2719407a8 100644 --- a/drivers/gpu/drm/drm_mode_object.c +++ b/drivers/gpu/drm/drm_mode_object.c @@ -224,12 +224,26 @@ EXPORT_SYMBOL(drm_mode_object_get); * This attaches the given property to the modeset object with the given initial * value. Currently this function cannot fail since the properties are stored in * a statically sized array. + * + * Note that all properties must be attached before the object itself is + * registered and accessible from userspace. */ void drm_object_attach_property(struct drm_mode_object *obj, struct drm_property *property, uint64_t init_val) { int count = obj->properties->count; + struct drm_device *dev = property->dev; + + + if (obj->type == DRM_MODE_OBJECT_CONNECTOR) { + struct drm_connector *connector = obj_to_connector(obj); + + WARN_ON(!dev->driver->load && + connector->registration_state == DRM_CONNECTOR_REGISTERED); + } else { + WARN_ON(!dev->driver->load && dev->registered); + } if (count == DRM_OBJECT_MAX_PROPERTY) { WARN(1, "Failed to attach object property (type: 0x%x). Please "
Properties can't be attached after registering, userspace would get confused (no one bothers to reprobe really). - Add kerneldoc - Enforce this with some checks. This needs a somewhat ugly check since connectors can be added later on, but we still need to attach all properties before they go public. Note that we already enforce that properties themselves are created before the entire device is registered. Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Rajat Jain <rajatja@google.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_mode_object.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)