| Message ID | 1421693756-12917-1-git-send-email-wsa@the-dreams.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Wolfram, > On Jan 19, 2015, at 20:55 , Wolfram Sang <wsa@the-dreams.de> wrote: > > Back in the days, sysfs seemed to have refcounting issues and subsystems > needed a completion to be safe. This is not the case anymore, so I2C can > get rid of this code. There is noone else besides I2C doing something > like this currently (checked with the attached coccinelle script which > checks if a release function exists and if it contains a completion). > > I have been digging through the history of linux.git and > linux-history.git and found that e.g. w1 used to have such a mechanism > and also simply removed it later. > > Some more info from Greg Kroah-Hartman: > "Having that call "wait" for the other release call to happen is really > old, as Jean points out, from 2003. We have "fixed" sysfs since then to > detach the files from the devices easier, we used to have some nasy > reference count issues in that area." > > And some testing from Jean Delvare which matches my results: > "However I just tested unloading an i2c bus driver while its adapter's > new_device attribute was opened and rmmod returned immediately. So it > doesn't look like accessing sysfs attributes actually takes a reference > to the underlying i2c_adapter." > > Let's get rid of this code before really nobody knows/understands > anymore what this was for and if it has a subtle use. > Hehe, rather obliquely tested by me too :) Please save the reference counter hackers sanity and merge this :) > Reported-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com> > Signed-off-by: Wolfram Sang <wsa@the-dreams.de> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Jean Delvare <jdelvare@suse.de> > Cc: Julia Lawall <julia.lawall@lip6.fr> > — > Regards — Pantelis
On Mon, Jan 19, 2015 at 07:55:56PM +0100, Wolfram Sang wrote: > Back in the days, sysfs seemed to have refcounting issues and subsystems > needed a completion to be safe. This is not the case anymore, so I2C can > get rid of this code. There is noone else besides I2C doing something > like this currently (checked with the attached coccinelle script which > checks if a release function exists and if it contains a completion). > > I have been digging through the history of linux.git and > linux-history.git and found that e.g. w1 used to have such a mechanism > and also simply removed it later. > > Some more info from Greg Kroah-Hartman: > "Having that call "wait" for the other release call to happen is really > old, as Jean points out, from 2003. We have "fixed" sysfs since then to > detach the files from the devices easier, we used to have some nasy > reference count issues in that area." > > And some testing from Jean Delvare which matches my results: > "However I just tested unloading an i2c bus driver while its adapter's > new_device attribute was opened and rmmod returned immediately. So it > doesn't look like accessing sysfs attributes actually takes a reference > to the underlying i2c_adapter." > > Let's get rid of this code before really nobody knows/understands > anymore what this was for and if it has a subtle use. > > Reported-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com> > Signed-off-by: Wolfram Sang <wsa@the-dreams.de> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Jean Delvare <jdelvare@suse.de> > Cc: Julia Lawall <julia.lawall@lip6.fr> > --- > > Of course, more testing is appreciated. Here is the coccinelle script: > > === > > @has_type@ > identifier d_type, rel_f; > @@ > > struct device_type d_type = { > .release = rel_f, > }; > > @has_device@ > struct device *d; > identifier rel_f, p; > @@ > > ( > p->dev.release = &rel_f; > | > d->release = &rel_f; > ) > > @find_type depends on has_type@ > identifier has_type.rel_f, d; > @@ > > void rel_f(struct device *d) > { > ... > * complete(...); > ... > } > > @find_device depends on has_device@ > identifier has_device.rel_f, d; > @@ > > void rel_f(struct device *d) > { > ... > * complete(...); > ... > } > > === > > drivers/i2c/i2c-core.c | 10 +--------- > include/linux/i2c.h | 1 - > 2 files changed, 1 insertion(+), 10 deletions(-) > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > index 39d25a8cb1ad..15cc5902cf89 100644 > --- a/drivers/i2c/i2c-core.c > +++ b/drivers/i2c/i2c-core.c > @@ -41,7 +41,6 @@ > #include <linux/of_device.h> > #include <linux/of_irq.h> > #include <linux/clk/clk-conf.h> > -#include <linux/completion.h> > #include <linux/hardirq.h> > #include <linux/irqflags.h> > #include <linux/rwsem.h> > @@ -1184,8 +1183,7 @@ EXPORT_SYMBOL_GPL(i2c_new_dummy); > > static void i2c_adapter_dev_release(struct device *dev) > { > - struct i2c_adapter *adap = to_i2c_adapter(dev); > - complete(&adap->dev_released); > + /* empty, but the driver core insists we need a release function */ Yeah, it does, but I hate to see this in "real" code as something is probably wrong with it if it happens. Please move the rest of 'i2c_del_adapter' into the release function (what was after the wait_for_completion() call), and then all should be fine. thanks, greg k-h
On Mon, Jan 19, 2015 at 07:55:56PM +0100, Wolfram Sang wrote: > Back in the days, sysfs seemed to have refcounting issues and subsystems > needed a completion to be safe. This is not the case anymore, so I2C can > get rid of this code. There is noone else besides I2C doing something > like this currently (checked with the attached coccinelle script which > checks if a release function exists and if it contains a completion). Have you validated this with DEBUG_KOBJECT_RELEASE enabled?
On Mon, Jan 19, 2015 at 07:12:10PM +0000, Russell King - ARM Linux wrote: > On Mon, Jan 19, 2015 at 07:55:56PM +0100, Wolfram Sang wrote: > > Back in the days, sysfs seemed to have refcounting issues and subsystems > > needed a completion to be safe. This is not the case anymore, so I2C can > > get rid of this code. There is noone else besides I2C doing something > > like this currently (checked with the attached coccinelle script which > > checks if a release function exists and if it contains a completion). > > Have you validated this with DEBUG_KOBJECT_RELEASE enabled? You saved me, thank you a lot for this pointer! Patch discarded. I assume other subsystems also moved away from 'struct device_type' for the release function as well; but this is just a guess for now and I'll call it a day for today.
On 01/19/2015 07:55 PM, Wolfram Sang wrote: [...] > > Let's get rid of this code before really nobody knows/understands > anymore what this was for and if it has a subtle use. Getting rid of this is the right thing, cause it's just not how it should be done, but unfortunately it is not as simple as this. The problem is that the adapter is typically embedded in the parent device's state struct. This state struct is typically freed directly after calling i2c_del_adapter(). If there is still something holding a reference to the adapter this will result in a use after free. To do this properly i2c_add_adapter() needs to be changed to i2c_alloc_adapter() that returns a pointer to a newly allocated adapter. i2c_free_adapter() will then only drop a reference to the adapter, but not free any memory. Once the last reference has been removed the memory can then be freed in the release callback. The other issue is as long as something has a reference to the adapter they can run operations on the adapter. So freeing the adapter also has to make sure that any further operations that are performed on the adapter do no longer call into the device specific ops, but rather returns -ENODEV, or similar. - Lars
> > @@ -1184,8 +1183,7 @@ EXPORT_SYMBOL_GPL(i2c_new_dummy); > > > > static void i2c_adapter_dev_release(struct device *dev) > > { > > - struct i2c_adapter *adap = to_i2c_adapter(dev); > > - complete(&adap->dev_released); > > + /* empty, but the driver core insists we need a release function */ > > Yeah, it does, but I hate to see this in "real" code as something is > probably wrong with it if it happens. > > Please move the rest of 'i2c_del_adapter' into the release function > (what was after the wait_for_completion() call), and then all should be > fine. I was about to do this as a follow-up patch. But Russell's and Lars-Peter's responses made this obsolete already.
On Tue, Jan 20, 2015 at 03:01:42AM +0800, Greg Kroah-Hartman wrote: > On Mon, Jan 19, 2015 at 07:55:56PM +0100, Wolfram Sang wrote: > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > > index 39d25a8cb1ad..15cc5902cf89 100644 > > --- a/drivers/i2c/i2c-core.c > > +++ b/drivers/i2c/i2c-core.c > > @@ -41,7 +41,6 @@ > > #include <linux/of_device.h> > > #include <linux/of_irq.h> > > #include <linux/clk/clk-conf.h> > > -#include <linux/completion.h> > > #include <linux/hardirq.h> > > #include <linux/irqflags.h> > > #include <linux/rwsem.h> > > @@ -1184,8 +1183,7 @@ EXPORT_SYMBOL_GPL(i2c_new_dummy); > > > > static void i2c_adapter_dev_release(struct device *dev) > > { > > - struct i2c_adapter *adap = to_i2c_adapter(dev); > > - complete(&adap->dev_released); > > + /* empty, but the driver core insists we need a release function */ > > Yeah, it does, but I hate to see this in "real" code as something is > probably wrong with it if it happens. > > Please move the rest of 'i2c_del_adapter' into the release function > (what was after the wait_for_completion() call), and then all should be > fine. Are you sure about that? Some drivers do this, eg, i2c_del_adapter(&drv_data->adapter); free_irq(drv_data->irq, drv_data); where drv_data was allocated using devm_kzalloc(), and so will be released when the ->remove callback (which calls the above i2c_del_adapter()) returns... freeing the embedded device struct.
On Mon, Jan 19, 2015 at 11:04:27PM +0000, Russell King - ARM Linux wrote: > On Tue, Jan 20, 2015 at 03:01:42AM +0800, Greg Kroah-Hartman wrote: > > On Mon, Jan 19, 2015 at 07:55:56PM +0100, Wolfram Sang wrote: > > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > > > index 39d25a8cb1ad..15cc5902cf89 100644 > > > --- a/drivers/i2c/i2c-core.c > > > +++ b/drivers/i2c/i2c-core.c > > > @@ -41,7 +41,6 @@ > > > #include <linux/of_device.h> > > > #include <linux/of_irq.h> > > > #include <linux/clk/clk-conf.h> > > > -#include <linux/completion.h> > > > #include <linux/hardirq.h> > > > #include <linux/irqflags.h> > > > #include <linux/rwsem.h> > > > @@ -1184,8 +1183,7 @@ EXPORT_SYMBOL_GPL(i2c_new_dummy); > > > > > > static void i2c_adapter_dev_release(struct device *dev) > > > { > > > - struct i2c_adapter *adap = to_i2c_adapter(dev); > > > - complete(&adap->dev_released); > > > + /* empty, but the driver core insists we need a release function */ > > > > Yeah, it does, but I hate to see this in "real" code as something is > > probably wrong with it if it happens. > > > > Please move the rest of 'i2c_del_adapter' into the release function > > (what was after the wait_for_completion() call), and then all should be > > fine. > > Are you sure about that? Some drivers do this, eg, > > i2c_del_adapter(&drv_data->adapter); > free_irq(drv_data->irq, drv_data); > > where drv_data was allocated using devm_kzalloc(), and so will be > released when the ->remove callback (which calls the above > i2c_del_adapter()) returns... freeing the embedded device struct. But that will fail today if the memory is freed in i2c_del_adapter(), so there shouldn't be any change in logic here. Or am I missing something obvious? thanks, greg k-h
On 01/20/2015 02:41 AM, Greg Kroah-Hartman wrote: > On Mon, Jan 19, 2015 at 11:04:27PM +0000, Russell King - ARM Linux wrote: >> On Tue, Jan 20, 2015 at 03:01:42AM +0800, Greg Kroah-Hartman wrote: >>> On Mon, Jan 19, 2015 at 07:55:56PM +0100, Wolfram Sang wrote: >>>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c >>>> index 39d25a8cb1ad..15cc5902cf89 100644 >>>> --- a/drivers/i2c/i2c-core.c >>>> +++ b/drivers/i2c/i2c-core.c >>>> @@ -41,7 +41,6 @@ >>>> #include <linux/of_device.h> >>>> #include <linux/of_irq.h> >>>> #include <linux/clk/clk-conf.h> >>>> -#include <linux/completion.h> >>>> #include <linux/hardirq.h> >>>> #include <linux/irqflags.h> >>>> #include <linux/rwsem.h> >>>> @@ -1184,8 +1183,7 @@ EXPORT_SYMBOL_GPL(i2c_new_dummy); >>>> >>>> static void i2c_adapter_dev_release(struct device *dev) >>>> { >>>> - struct i2c_adapter *adap = to_i2c_adapter(dev); >>>> - complete(&adap->dev_released); >>>> + /* empty, but the driver core insists we need a release function */ >>> >>> Yeah, it does, but I hate to see this in "real" code as something is >>> probably wrong with it if it happens. >>> >>> Please move the rest of 'i2c_del_adapter' into the release function >>> (what was after the wait_for_completion() call), and then all should be >>> fine. >> >> Are you sure about that? Some drivers do this, eg, >> >> i2c_del_adapter(&drv_data->adapter); >> free_irq(drv_data->irq, drv_data); >> >> where drv_data was allocated using devm_kzalloc(), and so will be >> released when the ->remove callback (which calls the above >> i2c_del_adapter()) returns... freeing the embedded device struct. > > But that will fail today if the memory is freed in i2c_del_adapter(), so > there shouldn't be any change in logic here. > > Or am I missing something obvious? The memory is not freed in i2c_del_adapter().
On Tue, Jan 20, 2015 at 08:05:20AM +0100, Lars-Peter Clausen wrote: > On 01/20/2015 02:41 AM, Greg Kroah-Hartman wrote: > >On Mon, Jan 19, 2015 at 11:04:27PM +0000, Russell King - ARM Linux wrote: > >>On Tue, Jan 20, 2015 at 03:01:42AM +0800, Greg Kroah-Hartman wrote: > >>>On Mon, Jan 19, 2015 at 07:55:56PM +0100, Wolfram Sang wrote: > >>>>diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > >>>>index 39d25a8cb1ad..15cc5902cf89 100644 > >>>>--- a/drivers/i2c/i2c-core.c > >>>>+++ b/drivers/i2c/i2c-core.c > >>>>@@ -41,7 +41,6 @@ > >>>> #include <linux/of_device.h> > >>>> #include <linux/of_irq.h> > >>>> #include <linux/clk/clk-conf.h> > >>>>-#include <linux/completion.h> > >>>> #include <linux/hardirq.h> > >>>> #include <linux/irqflags.h> > >>>> #include <linux/rwsem.h> > >>>>@@ -1184,8 +1183,7 @@ EXPORT_SYMBOL_GPL(i2c_new_dummy); > >>>> > >>>> static void i2c_adapter_dev_release(struct device *dev) > >>>> { > >>>>- struct i2c_adapter *adap = to_i2c_adapter(dev); > >>>>- complete(&adap->dev_released); > >>>>+ /* empty, but the driver core insists we need a release function */ > >>> > >>>Yeah, it does, but I hate to see this in "real" code as something is > >>>probably wrong with it if it happens. > >>> > >>>Please move the rest of 'i2c_del_adapter' into the release function > >>>(what was after the wait_for_completion() call), and then all should be > >>>fine. > >> > >>Are you sure about that? Some drivers do this, eg, > >> > >> i2c_del_adapter(&drv_data->adapter); > >> free_irq(drv_data->irq, drv_data); > >> > >>where drv_data was allocated using devm_kzalloc(), and so will be > >>released when the ->remove callback (which calls the above > >>i2c_del_adapter()) returns... freeing the embedded device struct. > > > >But that will fail today if the memory is freed in i2c_del_adapter(), so > >there shouldn't be any change in logic here. > > > >Or am I missing something obvious? > > The memory is not freed in i2c_del_adapter(). Right, and I'm not saying it should be, just move the existing logic into the release callback, and the code flow should be the same and we don't end up with an "empty" release callback. thanks, greg k-h
On 01/20/2015 08:12 AM, Greg Kroah-Hartman wrote: > On Tue, Jan 20, 2015 at 08:05:20AM +0100, Lars-Peter Clausen wrote: >> On 01/20/2015 02:41 AM, Greg Kroah-Hartman wrote: >>> On Mon, Jan 19, 2015 at 11:04:27PM +0000, Russell King - ARM Linux wrote: >>>> On Tue, Jan 20, 2015 at 03:01:42AM +0800, Greg Kroah-Hartman wrote: >>>>> On Mon, Jan 19, 2015 at 07:55:56PM +0100, Wolfram Sang wrote: >>>>>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c >>>>>> index 39d25a8cb1ad..15cc5902cf89 100644 >>>>>> --- a/drivers/i2c/i2c-core.c >>>>>> +++ b/drivers/i2c/i2c-core.c >>>>>> @@ -41,7 +41,6 @@ >>>>>> #include <linux/of_device.h> >>>>>> #include <linux/of_irq.h> >>>>>> #include <linux/clk/clk-conf.h> >>>>>> -#include <linux/completion.h> >>>>>> #include <linux/hardirq.h> >>>>>> #include <linux/irqflags.h> >>>>>> #include <linux/rwsem.h> >>>>>> @@ -1184,8 +1183,7 @@ EXPORT_SYMBOL_GPL(i2c_new_dummy); >>>>>> >>>>>> static void i2c_adapter_dev_release(struct device *dev) >>>>>> { >>>>>> - struct i2c_adapter *adap = to_i2c_adapter(dev); >>>>>> - complete(&adap->dev_released); >>>>>> + /* empty, but the driver core insists we need a release function */ >>>>> >>>>> Yeah, it does, but I hate to see this in "real" code as something is >>>>> probably wrong with it if it happens. >>>>> >>>>> Please move the rest of 'i2c_del_adapter' into the release function >>>>> (what was after the wait_for_completion() call), and then all should be >>>>> fine. >>>> >>>> Are you sure about that? Some drivers do this, eg, >>>> >>>> i2c_del_adapter(&drv_data->adapter); >>>> free_irq(drv_data->irq, drv_data); >>>> >>>> where drv_data was allocated using devm_kzalloc(), and so will be >>>> released when the ->remove callback (which calls the above >>>> i2c_del_adapter()) returns... freeing the embedded device struct. >>> >>> But that will fail today if the memory is freed in i2c_del_adapter(), so >>> there shouldn't be any change in logic here. >>> >>> Or am I missing something obvious? >> >> The memory is not freed in i2c_del_adapter(). > > Right, and I'm not saying it should be, just move the existing logic > into the release callback, and the code flow should be the same and we > don't end up with an "empty" release callback. But the code flow often is. i2c_del_adapter(&drvdata->adap); kfree(drvdata); That wont work anymore if i2c_del_adapter() returns before the last reference has been dropped. This needs to be restructured so that the adapter memory can be freed by the release callback.
On Tue, Jan 20, 2015 at 03:12:56PM +0800, Greg Kroah-Hartman wrote: > On Tue, Jan 20, 2015 at 08:05:20AM +0100, Lars-Peter Clausen wrote: > > On 01/20/2015 02:41 AM, Greg Kroah-Hartman wrote: > > >On Mon, Jan 19, 2015 at 11:04:27PM +0000, Russell King - ARM Linux wrote: > > >>On Tue, Jan 20, 2015 at 03:01:42AM +0800, Greg Kroah-Hartman wrote: > > >>>On Mon, Jan 19, 2015 at 07:55:56PM +0100, Wolfram Sang wrote: > > >>>>diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > > >>>>index 39d25a8cb1ad..15cc5902cf89 100644 > > >>>>--- a/drivers/i2c/i2c-core.c > > >>>>+++ b/drivers/i2c/i2c-core.c > > >>>>@@ -41,7 +41,6 @@ > > >>>> #include <linux/of_device.h> > > >>>> #include <linux/of_irq.h> > > >>>> #include <linux/clk/clk-conf.h> > > >>>>-#include <linux/completion.h> > > >>>> #include <linux/hardirq.h> > > >>>> #include <linux/irqflags.h> > > >>>> #include <linux/rwsem.h> > > >>>>@@ -1184,8 +1183,7 @@ EXPORT_SYMBOL_GPL(i2c_new_dummy); > > >>>> > > >>>> static void i2c_adapter_dev_release(struct device *dev) > > >>>> { > > >>>>- struct i2c_adapter *adap = to_i2c_adapter(dev); > > >>>>- complete(&adap->dev_released); > > >>>>+ /* empty, but the driver core insists we need a release function */ > > >>> > > >>>Yeah, it does, but I hate to see this in "real" code as something is > > >>>probably wrong with it if it happens. > > >>> > > >>>Please move the rest of 'i2c_del_adapter' into the release function > > >>>(what was after the wait_for_completion() call), and then all should be > > >>>fine. > > >> > > >>Are you sure about that? Some drivers do this, eg, > > >> > > >> i2c_del_adapter(&drv_data->adapter); > > >> free_irq(drv_data->irq, drv_data); > > >> > > >>where drv_data was allocated using devm_kzalloc(), and so will be > > >>released when the ->remove callback (which calls the above > > >>i2c_del_adapter()) returns... freeing the embedded device struct. > > > > > >But that will fail today if the memory is freed in i2c_del_adapter(), so > > >there shouldn't be any change in logic here. > > > > > >Or am I missing something obvious? > > > > The memory is not freed in i2c_del_adapter(). > > Right, and I'm not saying it should be, just move the existing logic > into the release callback, and the code flow should be the same and we > don't end up with an "empty" release callback. IMHO there are two possibilities here: 1. leave it as-is, where we ensure that the remainder of i2c_del_adapter does not complete until the release callback has been called. 2. fix it properly by taking (eg) the netdev approach to i2c_adapter, or an alternative solution which results in decoupling the lifetime of the struct device from the i2c_adapter. Either of these would be much better than removing the completion and then moving a chunk of code to make it "look" safer than it actually is and thereby introducing potential use-after-free bugs.
===
@has_type@
identifier d_type, rel_f;
@@
struct device_type d_type = {
.release = rel_f,
};
@has_device@
struct device *d;
identifier rel_f, p;
@@
(
p->dev.release = &rel_f;
|
d->release = &rel_f;
)
@find_type depends on has_type@
identifier has_type.rel_f, d;
@@
void rel_f(struct device *d)
{
...
* complete(...);
...
}
@find_device depends on has_device@
identifier has_device.rel_f, d;
@@
void rel_f(struct device *d)
{
...
* complete(...);
...
}
===
drivers/i2c/i2c-core.c | 10 +---------
include/linux/i2c.h | 1 -
2 files changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 39d25a8cb1ad..15cc5902cf89 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -41,7 +41,6 @@
#include <linux/of_device.h>
#include <linux/of_irq.h>
#include <linux/clk/clk-conf.h>
-#include <linux/completion.h>
#include <linux/hardirq.h>
#include <linux/irqflags.h>
#include <linux/rwsem.h>
@@ -1184,8 +1183,7 @@ EXPORT_SYMBOL_GPL(i2c_new_dummy);
static void i2c_adapter_dev_release(struct device *dev)
{
- struct i2c_adapter *adap = to_i2c_adapter(dev);
- complete(&adap->dev_released);
+ /* empty, but the driver core insists we need a release function */
}
/*
@@ -1795,14 +1793,8 @@ void i2c_del_adapter(struct i2c_adapter *adap)
/* device name is gone after device_unregister */
dev_dbg(&adap->dev, "adapter [%s] unregistered\n", adap->name);
-
- /* clean up the sysfs representation */
- init_completion(&adap->dev_released);
device_unregister(&adap->dev);
- /* wait for sysfs to drop all references */
- wait_for_completion(&adap->dev_released);
-
/* free bus id */
mutex_lock(&core_lock);
idr_remove(&i2c_adapter_idr, adap->nr);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 524b45ea85a2..1df0c20ce648 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -466,7 +466,6 @@ struct i2c_adapter {
int nr;
char name[48];
- struct completion dev_released;
struct mutex userspace_clients_lock;
struct list_head userspace_clients;
Back in the days, sysfs seemed to have refcounting issues and subsystems needed a completion to be safe. This is not the case anymore, so I2C can get rid of this code. There is noone else besides I2C doing something like this currently (checked with the attached coccinelle script which checks if a release function exists and if it contains a completion). I have been digging through the history of linux.git and linux-history.git and found that e.g. w1 used to have such a mechanism and also simply removed it later. Some more info from Greg Kroah-Hartman: "Having that call "wait" for the other release call to happen is really old, as Jean points out, from 2003. We have "fixed" sysfs since then to detach the files from the devices easier, we used to have some nasy reference count issues in that area." And some testing from Jean Delvare which matches my results: "However I just tested unloading an i2c bus driver while its adapter's new_device attribute was opened and rmmod returned immediately. So it doesn't look like accessing sysfs attributes actually takes a reference to the underlying i2c_adapter." Let's get rid of this code before really nobody knows/understands anymore what this was for and if it has a subtle use. Reported-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com> Signed-off-by: Wolfram Sang <wsa@the-dreams.de> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Jean Delvare <jdelvare@suse.de> Cc: Julia Lawall <julia.lawall@lip6.fr> --- Of course, more testing is appreciated. Here is the coccinelle script: