[1/2] drm/property: Enforce more lifetime rules
diff mbox series

Message ID 20191023144953.28190-1-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • [1/2] drm/property: Enforce more lifetime rules
Related show

Commit Message

Daniel Vetter Oct. 23, 2019, 2:49 p.m. UTC
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(+)

Comments

Thierry Reding Oct. 24, 2019, 10:40 a.m. UTC | #1
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
Thierry Reding Oct. 24, 2019, 10:43 a.m. UTC | #2
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>
Daniel Vetter Oct. 24, 2019, 11:47 a.m. UTC | #3
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
Jani Nikula Oct. 24, 2019, 12:09 p.m. UTC | #4
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 "

Patch
diff mbox series

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 "