On Sun, Jan 26, 2014 at 1:51 AM, Imre Deak imre.deak@intel.com wrote:
On Sat, 2014-01-25 at 21:37 +0100, Daniel Vetter wrote:
On Fri, Jan 24, 2014 at 02:47:33PM +0200, Imre Deak wrote:
Atm we try to remove the connector's i2c sysfs entry too late in the encoder's destroy callback. By that time the kobject used as the parent for all connector sysfs entries is already removed when we do an early removal of all connector sysfs entries in intel_modeset_cleanup(). Fix this by adding an early_destroy encoder callback, where we remove the encoder's i2c adapter.
v2:
- add missing static to function, use existing sdvo cast helper, s/intel_sdvo/sdvo/ (Chris)
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70523
Signed-off-by: Imre Deak imre.deak@intel.com
This looks fishy ... sysfs should all be reference-counted, so I'm confused what's going on here. Also I think this smells a bit like it's going overall in the wrong direction - essentially with sysfs we can't really force-remove stuff but have to wait until the refcount drops to 0. Or at least that's how I think it works, I'd need to blow through a pile of time to figure this all out.
Hm, I haven't thought about refcounting :) Now, I agree that should normally allow for removing a parent and child device in both order.
What happens and why we can't remove first the parent then the child:
In
intel_dp_init_connector()-> intel_dp_i2c_init()
we set the i2c adapter.dev.parent = intel_connector->base.kdev;
device_register will then add the i2c sysfs entry to the connector sysfs dir. Refcounting here looks ok, both the parent connector kobject and its sysfs dir entry gets a reference from the child.
During module cleanup, we call
intel_modeset_cleanup()-> drm_sysfs_connector_remove() device_unregister(connector->kdev)
which is the i2c adapter's parent kdev. Then:
device_del()-> kobject_del()-> sysfs_remove_dir()
will remove all entries recursively in the connector's sysfs dir, along with all the i2c sysfs entries. Afterwards the intel encoder->destroy() callback calls i2c_del_adapter()->device_unregister()->device_del() and that will try to remove its own sysfs attributes, namely the power sysfs group, but won't find it since it was removed above and give a WARN.
Note that the parent's recursive removal happens regardless of its kobject's or sysfs entry's refcount. The kobject itself will be put after device_del() in put_device() and only destroyed after the i2c adapter releases the refcount it holds on it. I admit it feels strange that the sysfs entries are removed before the last reference on the kobject is dropped, not sure if it's by design or an overlook..
I have no idea either how exactly this is supposed to work, and I quick scan through Documentation/ didn't point me into a useful direction either.
Adding Greg (and more mailing lists) for insight. -Daniel
On Sun, 2014-01-26 at 10:21 +0100, Daniel Vetter wrote:
On Sun, Jan 26, 2014 at 1:51 AM, Imre Deak imre.deak@intel.com wrote:
On Sat, 2014-01-25 at 21:37 +0100, Daniel Vetter wrote:
On Fri, Jan 24, 2014 at 02:47:33PM +0200, Imre Deak wrote:
Atm we try to remove the connector's i2c sysfs entry too late in the encoder's destroy callback. By that time the kobject used as the parent for all connector sysfs entries is already removed when we do an early removal of all connector sysfs entries in intel_modeset_cleanup(). Fix this by adding an early_destroy encoder callback, where we remove the encoder's i2c adapter.
v2:
- add missing static to function, use existing sdvo cast helper, s/intel_sdvo/sdvo/ (Chris)
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70523
Signed-off-by: Imre Deak imre.deak@intel.com
This looks fishy ... sysfs should all be reference-counted, so I'm confused what's going on here. Also I think this smells a bit like it's going overall in the wrong direction - essentially with sysfs we can't really force-remove stuff but have to wait until the refcount drops to 0. Or at least that's how I think it works, I'd need to blow through a pile of time to figure this all out.
Hm, I haven't thought about refcounting :) Now, I agree that should normally allow for removing a parent and child device in both order.
What happens and why we can't remove first the parent then the child:
In
intel_dp_init_connector()-> intel_dp_i2c_init()
we set the i2c adapter.dev.parent = intel_connector->base.kdev;
device_register will then add the i2c sysfs entry to the connector sysfs dir. Refcounting here looks ok, both the parent connector kobject and its sysfs dir entry gets a reference from the child.
During module cleanup, we call
intel_modeset_cleanup()-> drm_sysfs_connector_remove() device_unregister(connector->kdev)
which is the i2c adapter's parent kdev. Then:
device_del()-> kobject_del()-> sysfs_remove_dir()
will remove all entries recursively in the connector's sysfs dir, along with all the i2c sysfs entries. Afterwards the intel encoder->destroy() callback calls i2c_del_adapter()->device_unregister()->device_del() and that will try to remove its own sysfs attributes, namely the power sysfs group, but won't find it since it was removed above and give a WARN.
Note that the parent's recursive removal happens regardless of its kobject's or sysfs entry's refcount. The kobject itself will be put after device_del() in put_device() and only destroyed after the i2c adapter releases the refcount it holds on it. I admit it feels strange that the sysfs entries are removed before the last reference on the kobject is dropped, not sure if it's by design or an overlook..
I have no idea either how exactly this is supposed to work, and I quick scan through Documentation/ didn't point me into a useful direction either.
Adding Greg (and more mailing lists) for insight.
Attached the corresponding dmesg.
Also one more thought. Imo whether or not it's a valid thing to delete first a parent device and only then its child device, in this case we don't have a reason to do so. We created first the connector device (parent) and then the i2c adapter device (child) and the cleanup should happen in reverse order. This is so regardless of what order the corresponding kobjects get destroyed based on their refcounts.
--Imre
On Sun, Jan 26, 2014 at 01:11:15PM +0200, Imre Deak wrote:
On Sun, 2014-01-26 at 10:21 +0100, Daniel Vetter wrote:
On Sun, Jan 26, 2014 at 1:51 AM, Imre Deak imre.deak@intel.com wrote:
On Sat, 2014-01-25 at 21:37 +0100, Daniel Vetter wrote:
On Fri, Jan 24, 2014 at 02:47:33PM +0200, Imre Deak wrote:
Atm we try to remove the connector's i2c sysfs entry too late in the encoder's destroy callback. By that time the kobject used as the parent for all connector sysfs entries is already removed when we do an early removal of all connector sysfs entries in intel_modeset_cleanup(). Fix this by adding an early_destroy encoder callback, where we remove the encoder's i2c adapter.
v2:
- add missing static to function, use existing sdvo cast helper, s/intel_sdvo/sdvo/ (Chris)
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70523
Signed-off-by: Imre Deak imre.deak@intel.com
This looks fishy ... sysfs should all be reference-counted, so I'm confused what's going on here. Also I think this smells a bit like it's going overall in the wrong direction - essentially with sysfs we can't really force-remove stuff but have to wait until the refcount drops to 0. Or at least that's how I think it works, I'd need to blow through a pile of time to figure this all out.
Hm, I haven't thought about refcounting :) Now, I agree that should normally allow for removing a parent and child device in both order.
What happens and why we can't remove first the parent then the child:
In
intel_dp_init_connector()-> intel_dp_i2c_init()
we set the i2c adapter.dev.parent = intel_connector->base.kdev;
device_register will then add the i2c sysfs entry to the connector sysfs dir. Refcounting here looks ok, both the parent connector kobject and its sysfs dir entry gets a reference from the child.
During module cleanup, we call
intel_modeset_cleanup()-> drm_sysfs_connector_remove() device_unregister(connector->kdev)
which is the i2c adapter's parent kdev. Then:
device_del()-> kobject_del()-> sysfs_remove_dir()
will remove all entries recursively in the connector's sysfs dir, along with all the i2c sysfs entries. Afterwards the intel encoder->destroy() callback calls i2c_del_adapter()->device_unregister()->device_del() and that will try to remove its own sysfs attributes, namely the power sysfs group, but won't find it since it was removed above and give a WARN.
Note that the parent's recursive removal happens regardless of its kobject's or sysfs entry's refcount. The kobject itself will be put after device_del() in put_device() and only destroyed after the i2c adapter releases the refcount it holds on it. I admit it feels strange that the sysfs entries are removed before the last reference on the kobject is dropped, not sure if it's by design or an overlook..
I have no idea either how exactly this is supposed to work, and I quick scan through Documentation/ didn't point me into a useful direction either.
Adding Greg (and more mailing lists) for insight.
Attached the corresponding dmesg.
Also one more thought. Imo whether or not it's a valid thing to delete first a parent device and only then its child device, in this case we don't have a reason to do so. We created first the connector device (parent) and then the i2c adapter device (child) and the cleanup should happen in reverse order. This is so regardless of what order the corresponding kobjects get destroyed based on their refcounts.
The kernel used to not complain if you removed a parent before the child, but now it does. As you already have all of the needed information, just switch the removal and then all should be fine.
thanks,
greg k-h
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Greg KH -
Imre Deak