Re: [Intel-gfx] [PATCH 1/2] drm/property: Enforce more lifetime rules
On Wed, 23 Oct 2019, 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
> Cc: Rajat Jain
> Signed-off-by: Daniel Vetter
> ---
> 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
> + 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 "
--
Jani Nikula, Intel Open Source Graphics Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/property: Enforce more lifetime rules
On Thu, Oct 24, 2019 at 12:43 PM Thierry Reding
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
> > > Cc: Rajat Jain
> > > Signed-off-by: Daniel Vetter
> > > ---
> > > 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
Date: Tue Sep 17 14:09:35 2019 +0200
drm/kms: Duct-tape for mode object lifetime checks
commit 4f5368b5541a902f6596558b05f5c21a9770dd32
Author: Daniel Vetter
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ä
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
Cc: Maarten Lankhorst
Reported-by: Michel Dänzer
Reviewed-by: Michel Dänzer
Tested-by: Michel Dänzer
Cc: Michel Dänzer
Signed-off-by: Daniel Vetter
Link:
https://patchwork.freedesktop.org/patch/msgid/20190917120936.7501-1-daniel.vet...@ffwll.ch>
I think I'll add a reference to the above to the commit message when applying.
> Reviewed-by: Thierry Reding
Thanks for taking a look.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.o
Re: [Intel-gfx] [PATCH 1/2] drm/property: Enforce more lifetime rules
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
> > Cc: Rajat Jain
> > Signed-off-by: Daniel Vetter
> > ---
> > 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
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/property: Enforce more lifetime rules
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
> Cc: Rajat Jain
> Signed-off-by: Daniel Vetter
> ---
> 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
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] drm/property: Enforce more lifetime rules
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
Cc: Rajat Jain
Signed-off-by: Daniel Vetter
---
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);
+ }
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
