On Sun, Jan 26, 2014 at 1:51 AM, Imre Deak <[email protected]> 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 <[email protected]>
>>
>> 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
On Sun, 2014-01-26 at 10:21 +0100, Daniel Vetter wrote:
> On Sun, Jan 26, 2014 at 1:51 AM, Imre Deak <[email protected]> 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 <[email protected]>
> >>
> >> 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 <[email protected]> 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 <[email protected]>
> > >>
> > >> 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