On Sun, Jan 26, 2014 at 1:51 AM, Imre Deak <imre.deak@xxxxxxxxx> 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@xxxxxxxxx> >> >> 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
