Re: [PATCH RFD] alternative kobject release wait mechanism

[Dmitry Torokhov]

On 4/19/07, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, 18 Apr 2007, Dmitry Torokhov wrote:
>
> > I am still do not understand why this is needed. Would it not be
> > simplier just to use a reference to struct device instead of embedding
> > it in a larger structure if their lifetimes are different and one does
> > not have a subsystem that takes care of releasing logic.
> >
> >
> > Pretty much drivers have 2 options:
> >
> > struct my_device {
> >         void *private_data;
> >         struct device dev;
> > };
>
> Actually people use dev_[gs]et_drvdata() instead of a separate
> private_data pointer.  That way there's no need for the my_device
> container.

No, drvdata belongs to driver that is bound to a device. We are
talking about private data created and managed by driver that provides
device.

> > In this case ->release must live in a subsystem code; individual
> > drivers kfree(my_dev->private) and do any additional cleanup after
> > calling device_unregister(&my_dev->dev);
>
> That doesn't sound right.  Generally the call to device_unregister() and
> the release method live in the same module.  Maybe you meant to say
> individual drivers kfree(my_dev->private_data) and do any additional
> cleanup in their remove() routine.

Again, if we are talking about driver bound to a device then devices
->release() is irrelevant. If we are talking about driver that created
device then driver's ->remove() is irrelevant.

> This approach seems dangerous.  Suppose there's mutex embedded in
> my_dev->private_data, and suppose some other thread is blocked waiting on
> that mutex when remove() is called.  That other thread will then oops when
> my_dev->private_data is deallocated.

What other thread? I suppose it is module-local thread or
subsystem-local thread. Let's that particular subsystem take care of
it's own data and use its own ->release() when it is ready. What I
mean is there is no need to perform clean-up at once; every layer can
do its own cleanup.

> > Second option:
> >
> > struct my_device {
> >         type member1;
> >         type member2;
> >
> >        struct device *dev;
> > };
> >
> > dev is coming from _device_create(). Driver core takes care of
> > releasing dev structure; driver does cleanup of my_device.
>
> Lots of drivers create devices dynamically without using device_create().
>
> More to the point, how does the driver clean up my_device?  It probably
> has a reference count somewhere in my_device, especially if my_device is
> shared with other threads or other drivers.  We then face exactly the same
> problem: What happens if the driver's module is unloaded before all the
> references to my_device are gone?

This is up to subsystem to ensure that it does not access dead devices.

> > With current sysfs orphaning attributes upon removal request there is
> > no issue of accessing driver-private data through references obtained
> > via ether embedded or referenced dev structure so everything is fine.
>
> Not so.  There are other pathways besides sysfs which can cause a driver
> to access its data structures.

Which ones? These needs to be identified and treated with "immediate
disconnect" that you advocated earlier. Once active users of device's
services are gone you can zap it.

--
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/