Bug creating USB endpoints in 2.6.20.x (kernel bug 8198)

linux-usb-devel New Tejun Heo 6 participants 20 messages May 9, 2007 – May 10, 2007 1 message not in the archive Driver core

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Tejun Heo

author

2007-05-09 09:40 UTC #1 lore raw

[adding back linux-usb-devel. please don't drop cc] [also adding lkml and Greg K-H]

Quoted 6 lines · Chris Rankin

Quoted 2 lines · --- Tejun Heo
Okay, here's patch against 2.6.20.11. Let's hope we're not chasing something which is already fixed.
Hooray! The trick was to trace the "dev" file instead of the endpoint directory itself. The compressed oops is attached.

Alright, it took a lot of work but the winner is the "dev" node. Gee... We could have caught this earlier unless I forgot to print the name of the last component in the initial debug patch. Well, better late than never. :-(

Anyways, the problem is that the attribute "dev" is dynamically allocated using kmalloc() in device_add() and freed immediately after the file is removed in device_del(). It's basically assuming immediate-disconnect but that apparently isn't true yet.

The dynamic allocation is to set the owner of the attribute to the owner of the device being registered. This is twisted in that the field is added to work around sysfs/module lifetime problem but the workaround itself is broken with regard to lifetime rules. Aieeeeeeee... This sort of things are why I implemented immediate disconnect on sysfs in the first place.

So, we can fix the problem Chris is seeing by breaking module unload (by allowing it to unload too early). It doesn't sound too hot but module unloading race is much less likely than sysfs node deletion/open race.

Thanks.

Signature · 1 line

tejun

Chris Rankin

2007-05-09 12:24 UTC #2 lore raw

Quoted 3 lines · --- Tejun Heo

So, we can fix the problem Chris is seeing by breaking module unload (by allowing it to unload too early). It doesn't sound too hot but module unloading race is much less likely than sysfs node deletion/open race.

Yikes! Just temporary breakage, I hope :-)!! The only modules I unload on a regular basis these days are things like "microcode", which the init scripts take care of as part of the boot-up process.

Thanks for all that hard work,
Cheers,
Chris

      ___________________________________________________________

Yahoo! Answers - Got a question? Someone out there knows the answer. Try it now.
http://uk.answers.yahoo.com/

Tejun Heo

author

2007-05-09 13:31 UTC #3 lore raw

Quoted 8 lines · Chris Rankin

Quoted 3 lines · --- Tejun Heo
So, we can fix the problem Chris is seeing by breaking module unload (by allowing it to unload too early). It doesn't sound too hot but module unloading race is much less likely than sysfs node deletion/open race.
Yikes! Just temporary breakage, I hope :-)!! The only modules I unload on a regular basis these days are things like "microcode", which the init scripts take care of as part of the boot-up process.

Okay, here's a half-assed fix. With this patch applied, if you try to unload a module while you're opening it's dev attribute, kernel will oops later when the file is accessed or closed later but it should fix the bug winecfg triggers. I really dunno how to fix this the right way in the stable kernel. Better ideas? Anyone?

Signature · 1 line

tejun

---
 drivers/base/core.c    |   33 ++++++++-------------------------
 include/linux/device.h |    1 -
 2 files changed, 8 insertions(+), 26 deletions(-)

drivers/base/core.c +8 ~~-25~~

Index: tree0/drivers/base/core.c
===================================================================
@@ -287,6 +287,8 @@ static ssize_t show_dev(struct device *d
 	return print_dev_t(buf, dev->devt);
 }
 
+static struct device_attribute devt_attr = __ATTR(dev, S_IRUGO, show_dev, NULL );
+
 /*
  *	devices_subsys - structure to be registered with kobject core.
  */

@@ -490,24 +492,9 @@ int device_add(struct device *dev)
 		goto attrError;
 
 	if (MAJOR(dev->devt)) {
-		struct device_attribute *attr;
-		attr = kzalloc(sizeof(*attr), GFP_KERNEL);
-		if (!attr) {
-			error = -ENOMEM;
-			goto ueventattrError;
-		}
-		attr->attr.name = "dev";
-		attr->attr.mode = S_IRUGO;
-		if (dev->driver)
-			attr->attr.owner = dev->driver->owner;
-		attr->show = show_dev;
-		error = device_create_file(dev, attr);
-		if (error) {
-			kfree(attr);
+		error = device_create_file(dev, &devt_attr);
+		if (error)
 			goto ueventattrError;
-		}
-
-		dev->devt_attr = attr;
 	}
 
 	if (dev->class) {

@@ -568,10 +555,8 @@ int device_add(struct device *dev)
  GroupError:
  	device_remove_attrs(dev);
  AttrsError:
-	if (dev->devt_attr) {
-		device_remove_file(dev, dev->devt_attr);
-		kfree(dev->devt_attr);
-	}
+	if (MAJOR(dev->devt))
+		device_remove_file(dev, &devt_attr);
  ueventattrError:
 	device_remove_file(dev, &dev->uevent_attr);
  attrError:

@@ -650,10 +635,8 @@ void device_del(struct device * dev)
 
 	if (parent)
 		klist_del(&dev->knode_parent);
-	if (dev->devt_attr) {
-		device_remove_file(dev, dev->devt_attr);
-		kfree(dev->devt_attr);
-	}
+	if (MAJOR(dev->devt))
+		device_remove_file(dev, &devt_attr);
 	if (dev->class) {
 		sysfs_remove_link(&dev->kobj, "subsystem");
 		/* If this is not a "fake" compatible device, remove the

include/linux/device.h +0 -1

Index: tree0/include/linux/device.h
===================================================================
@@ -357,7 +357,6 @@ struct device {
 	char	bus_id[BUS_ID_SIZE];	/* position on parent bus */
 	unsigned		is_registered:1;
 	struct device_attribute uevent_attr;
-	struct device_attribute *devt_attr;
 
 	struct semaphore	sem;	/* semaphore to synchronize calls to
 					 * its driver.

Chris Rankin

2007-05-09 13:56 UTC #4 lore raw

Quoted 5 lines · --- Tejun Heo

Okay, here's a half-assed fix. With this patch applied, if you try to unload a module while you're opening it's dev attribute, kernel will oops later when the file is accessed or closed later but it should fix the bug winecfg triggers. I really dunno how to fix this the right way in the stable kernel. Better ideas? Anyone?

How about a WARN() and a small(?) memory leak? Better than an oops, surely?

Cheers,
Chris

      ___________________________________________________________

Yahoo! Mail is the world's favourite email. Don't settle for less, sign up for
your free account today http://uk.rd.yahoo.com/evt=44106/*http://uk.docs.yahoo.com/mail/winter07.html

Tejun Heo

author

2007-05-09 14:12 UTC #5 lore raw

Quoted 8 lines · Chris Rankin

Quoted 5 lines · --- Tejun Heo
Okay, here's a half-assed fix. With this patch applied, if you try to unload a module while you're opening it's dev attribute, kernel will oops later when the file is accessed or closed later but it should fix the bug winecfg triggers. I really dunno how to fix this the right way in the stable kernel. Better ideas? Anyone?
How about a WARN() and a small(?) memory leak? Better than an oops, surely?

Device node creation/deletion can be quite often depending on configuration, so I don't think we can afford memory leak here. It can develop into a big problem for long running hosts. IMHO, just introducing module unload/deletion race is much better. It's the lesser evil, difficult to trigger and already broken in other places anyway.

I think we need to hear what other people are thinking about it. Cc'ing Maneesh, Dmitry and Cornelia. The whole thread can be read at...

http://thread.gmane.org/gmane.linux.usb.devel/53559
http://thread.gmane.org/gmane.linux.usb.devel/53846

The thread is rather long but just reading the message from the second URL should be enough. The problem is that dev->devt_attr (class dev has the same problem) is deallocated when the device is deleted. If the dev sysfs attribute has users at that point, the dev sysfs node is left with garbled struct attribute causing oops later.

IMHO, the proper fix for this is immediate-disconnect which is no in -mm as the problem is caused by expecting immediate-disconnect behavior when it isn't implemented.

As written above, I think it's better to risk module unload / sysfs race than keeping the current sysfs deletion / open race. What do you guys think?

Thanks.

Signature · 1 line

tejun

Dmitry Torokhov

2007-05-09 14:35 UTC #6 lore raw

Hi Tejun,

Quoted 28 lines · Tejun Heo

Quoted 8 lines · Chris Rankin
Quoted 5 lines · --- Tejun Heo
Okay, here's a half-assed fix. With this patch applied, if you try to unload a module while you're opening it's dev attribute, kernel will oops later when the file is accessed or closed later but it should fix the bug winecfg triggers. I really dunno how to fix this the right way in the stable kernel. Better ideas? Anyone?
How about a WARN() and a small(?) memory leak? Better than an oops, surely?
Device node creation/deletion can be quite often depending on configuration, so I don't think we can afford memory leak here. It can develop into a big problem for long running hosts. IMHO, just introducing module unload/deletion race is much better. It's the lesser evil, difficult to trigger and already broken in other places anyway.
I think we need to hear what other people are thinking about it. Cc'ing Maneesh, Dmitry and Cornelia. The whole thread can be read at...
http://thread.gmane.org/gmane.linux.usb.devel/53559
http://thread.gmane.org/gmane.linux.usb.devel/53846
The thread is rather long but just reading the message from the second URL should be enough. The problem is that dev->devt_attr (class dev has the same problem) is deallocated when the device is deleted. If the dev sysfs attribute has users at that point, the dev sysfs node is left with garbled struct attribute causing oops later.

Quoted 8 lines

IMHO, the proper fix for this is immediate-disconnect which is no in -mm as the problem is caused by expecting immediate-disconnect behavior when it isn't implemented.
As written above, I think it's better to risk module unload / sysfs race than keeping the current sysfs deletion / open race. What do you guys think?

How about embedding struct attribute fro devt into struct [class_]device for now? It is not too big and device is still going to be pinned into memory while there are sysfs users... I don't like fattening of device structures but leaks and/or oopses are worse in my book.

Signature · 1 line

Dmitry

Tejun Heo

author

2007-05-09 14:59 UTC #7 lore raw

Quoted 10 lines · Dmitry Torokhov

Quoted 4 lines
As written above, I think it's better to risk module unload / sysfs race than keeping the current sysfs deletion / open race. What do you guys think?
How about embedding struct attribute fro devt into struct [class_]device for now? It is not too big and device is still going to be pinned into memory while there are sysfs users... I don't like fattening of device structures but leaks and/or oopses are worse in my book.

Right, your book is apparently much better than mine. Actually, we can just free devt_attr in device_release(). Looking at the code, class device is already doing it that way, so here's the full-assed fix. Chris, can you please test the attached patch?

Thanks.

Signature · 1 line

tejun

---
 drivers/base/core.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/base/core.c +4 -3

Index: tree0/drivers/base/core.c
===================================================================
@@ -93,6 +93,9 @@ static void device_release(struct kobjec
 {
 	struct device * dev = to_dev(kobj);
 
+	kfree(dev->devt_attr);
+	dev->devt_attr = NULL;
+
 	if (dev->release)
 		dev->release(dev);
 	else if (dev->class && dev->class->dev_release)

@@ -650,10 +653,8 @@ void device_del(struct device * dev)
 
 	if (parent)
 		klist_del(&dev->knode_parent);
-	if (dev->devt_attr) {
+	if (dev->devt_attr)
 		device_remove_file(dev, dev->devt_attr);
-		kfree(dev->devt_attr);
-	}
 	if (dev->class) {
 		sysfs_remove_link(&dev->kobj, "subsystem");
 		/* If this is not a "fake" compatible device, remove the

Chris Rankin

2007-05-09 21:10 UTC #8 lore raw

Quoted 3 lines · --- Tejun Heo

Looking at the code, class
device is already doing it that way, so here's the full-assed fix. Chris, can you please test the attached patch?

Tejun,

So far so good; my 2.6.20.11+patch kernel hasn't oopsed yet. I'm going to start using this kernel as my default boot and see how it goes.

Cheers,
Chris

      ___________________________________________________________

Yahoo! Answers - Got a question? Someone out there knows the answer. Try it now.
http://uk.answers.yahoo.com/

Greg KH

2007-05-09 14:58 UTC #9 lore raw

In reply to Tejun Heo

Quoted 10 lines · Tejun Heo

Quoted 8 lines · Chris Rankin
Quoted 3 lines · --- Tejun Heo
So, we can fix the problem Chris is seeing by breaking module unload (by allowing it to unload too early). It doesn't sound too hot but module unloading race is much less likely than sysfs node deletion/open race.
Yikes! Just temporary breakage, I hope :-)!! The only modules I unload on a regular basis these days are things like "microcode", which the init scripts take care of as part of the boot-up process.

Quoted 5 lines

Okay, here's a half-assed fix. With this patch applied, if you try to unload a module while you're opening it's dev attribute, kernel will oops later when the file is accessed or closed later but it should fix the bug winecfg triggers. I really dunno how to fix this the right way in the stable kernel. Better ideas? Anyone?

This looks like the correct fix, the only reason I made this dynamic was to properly set up the module owner. And now that you have removed that link in your sysfs rework, it can go and become static again.

Oh, and this isn't a "device node", it's just a text file that contains the major:minor number for a device node to be created from.

So, care to fix up the class code too and send a patch with a signed-off-by:?

thanks,

greg k-h

Tejun Heo

author

2007-05-09 15:01 UTC #10 lore raw

Quoted 19 lines · Greg KH

Quoted 5 lines · Tejun Heo
Quoted 4 lines · Chris Rankin
Quoted 3 lines · --- Tejun Heo
So, we can fix the problem Chris is seeing by breaking module unload (by allowing it to unload too early). It doesn't sound too hot but module unloading race is much less likely than sysfs node deletion/open race.
Quoted 8 lines
Quoted 3 lines
Yikes! Just temporary breakage, I hope :-)!! The only modules I unload on a regular basis these days are things like "microcode", which the init scripts take care of as part of the boot-up process.
Okay, here's a half-assed fix. With this patch applied, if you try to unload a module while you're opening it's dev attribute, kernel will oops later when the file is accessed or closed later but it should fix the bug winecfg triggers. I really dunno how to fix this the right way in the stable kernel. Better ideas? Anyone?
This looks like the correct fix, the only reason I made this dynamic was to properly set up the module owner. And now that you have removed that link in your sysfs rework, it can go and become static again.

Quoted 5 lines

Oh, and this isn't a "device node", it's just a text file that contains the major:minor number for a device node to be created from.
So, care to fix up the class code too and send a patch with a signed-off-by:?

I think the proper fix for -stable is to free the structure in device_release(). I asked Chris to test it. After he confirms, I'll send a proper patch with S-O-B.

For -mm, yeap, we can simply make the whole thing static. I'll send patch for that soon.

Thanks.

Signature · 1 line

tejun

Greg KH

2007-05-09 16:13 UTC #11 lore raw

Quoted 23 lines · Tejun Heo

Quoted 22 lines · Greg KH
Quoted 5 lines · Tejun Heo
Quoted 4 lines · Chris Rankin
Quoted 3 lines · --- Tejun Heo
So, we can fix the problem Chris is seeing by breaking module unload (by allowing it to unload too early). It doesn't sound too hot but module unloading race is much less likely than sysfs node deletion/open race.
Quoted 8 lines
Quoted 3 lines
Yikes! Just temporary breakage, I hope :-)!! The only modules I unload on a regular basis these days are things like "microcode", which the init scripts take care of as part of the boot-up process.
Okay, here's a half-assed fix. With this patch applied, if you try to unload a module while you're opening it's dev attribute, kernel will oops later when the file is accessed or closed later but it should fix the bug winecfg triggers. I really dunno how to fix this the right way in the stable kernel. Better ideas? Anyone?
This looks like the correct fix, the only reason I made this dynamic was to properly set up the module owner. And now that you have removed that link in your sysfs rework, it can go and become static again.
Oh, and this isn't a "device node", it's just a text file that contains the major:minor number for a device node to be created from.

Quoted 6 lines

Quoted 2 lines
So, care to fix up the class code too and send a patch with a signed-off-by:?
I think the proper fix for -stable is to free the structure in device_release(). I asked Chris to test it. After he confirms, I'll send a proper patch with S-O-B.

Ah, yes, that's the correct fix for this.

thanks,

greg k-h

Tejun Heo

author

2007-05-10 14:45 UTC #12 lore raw

PATCH 1/1 driver-core: don't free devt_attr till the device is released

Currently, devt_attr for the "dev" file is freed immediately on device removal, but if the "dev" sysfs file is open when a device is removed, sysfs will access its attribute structure for further access including close resulting in jumping to garbled address. Fix it by postponing freeing devt_attr to device release time.

Note that devt_attr for class_device is already freed on release.

This bug is reported by Chris Rankin as bugzilla bug#8198.

Signed-off-by Tejun Heo <[email protected]>

Cc Chris Rankin <[email protected]>

---
Applies well to 2.6.20 and 21. As sysfs-immediate-disconnect doesn't seem to be included in 2.6.22, this should be included in linus#master too (applies well there as well).

* This is the second post. Something went wrong with the recipients list on the first posting. Both are same.

Thanks.

 drivers/base/core.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/base/core.c +4 -3

Index: tree0/drivers/base/core.c
===================================================================
@@ -93,6 +93,9 @@ static void device_release(struct kobjec
 {
 	struct device * dev = to_dev(kobj);
 
+	kfree(dev->devt_attr);
+	dev->devt_attr = NULL;
+
 	if (dev->release)
 		dev->release(dev);
 	else if (dev->class && dev->class->dev_release)

@@ -650,10 +653,8 @@ void device_del(struct device * dev)
 
 	if (parent)
 		klist_del(&dev->knode_parent);
-	if (dev->devt_attr) {
+	if (dev->devt_attr)
 		device_remove_file(dev, dev->devt_attr);
-		kfree(dev->devt_attr);
-	}
 	if (dev->class) {
 		sysfs_remove_link(&dev->kobj, "subsystem");
 		/* If this is not a "fake" compatible device, remove the

Greg KH

2007-05-10 15:04 UTC #13 lore raw

Re: [PATCH] driver-core: don't free devt_attr till the device is released

Quoted 8 lines · Tejun Heo

Currently, devt_attr for the "dev" file is freed immediately on device removal, but if the "dev" sysfs file is open when a device is removed, sysfs will access its attribute structure for further access including close resulting in jumping to garbled address. Fix it by postponing freeing devt_attr to device release time.
Note that devt_attr for class_device is already freed on release.

Quoted 8 lines

This bug is reported by Chris Rankin as bugzilla bug#8198.
Signed-off-by Tejun Heo <[email protected]>
Cc Chris Rankin <[email protected]>
---
Applies well to 2.6.20 and 21. As sysfs-immediate-disconnect doesn't seem to be included in 2.6.22, this should be included in linus#master too (applies well there as well).

As I don't think we should be adding your sysfs rework to 2.6.22 just yet, any objections to me just sending this to Linus for 2.6.22 and waiting on your previous one for when the whole sysfs rework patchset is sent?

thanks,

greg k-h

Tejun Heo

author

2007-05-10 15:13 UTC #14 lore raw

Re: [PATCH] driver-core: don't free devt_attr till the device is released

Quoted 8 lines · Greg KH

Quoted 3 lines
Applies well to 2.6.20 and 21. As sysfs-immediate-disconnect doesn't seem to be included in 2.6.22, this should be included in linus#master too (applies well there as well).
As I don't think we should be adding your sysfs rework to 2.6.22 just yet, any objections to me just sending this to Linus for 2.6.22 and waiting on your previous one for when the whole sysfs rework patchset is sent?

No objection at all. That actually is exactly my intention. The make-attrs-static patch will conflict with this fix tho. Just let me know when it breaks, I'll post updated one.

Thanks.

Signature · 1 line

tejun

Greg KH

2007-05-10 15:36 UTC #15 lore raw

Re: [PATCH] driver-core: don't free devt_attr till the device is released

Quoted 5 lines · Tejun Heo

Quoted 4 lines · Greg KH
Quoted 3 lines
Applies well to 2.6.20 and 21. As sysfs-immediate-disconnect doesn't seem to be included in 2.6.22, this should be included in linus#master too (applies well there as well).

Quoted 8 lines

Quoted 4 lines
As I don't think we should be adding your sysfs rework to 2.6.22 just yet, any objections to me just sending this to Linus for 2.6.22 and waiting on your previous one for when the whole sysfs rework patchset is sent?
No objection at all. That actually is exactly my intention. The make-attrs-static patch will conflict with this fix tho. Just let me know when it breaks, I'll post updated one.

If it breaks, I'll fix up the merge issues, don't worry about it :)

Thanks again for taking the time and tracking it down and fixing this.

greg k-h

Kay Sievers

2007-05-10 15:33 UTC #16 lore raw

Re: [PATCH] driver-core: don't free devt_attr till the device is released

In reply to Tejun Heo

Quoted 7 lines · Tejun Heo

Currently, devt_attr for the "dev" file is freed immediately on device removal, but if the "dev" sysfs file is open when a device is removed, sysfs will access its attribute structure for further access including close resulting in jumping to garbled address. Fix it by postponing freeing devt_attr to device release time.
Note that devt_attr for class_device is already freed on release.

Hi Tejun,
your rework removes the "owner" field from the attributes. I think we kept the "dev" and "uevent" attribute as part of "struct device" only to be able to assign it the actual owner of the module that has created the device. The attribute can probably just live as one instance statically in the driver core now?

Thanks,
Kay

Tejun Heo

author

2007-05-10 15:42 UTC #17 lore raw

Re: [PATCH] driver-core: don't free devt_attr till the device is released

Quoted 7 lines · Kay Sievers

Quoted 6 lines · Tejun Heo
Currently, devt_attr for the "dev" file is freed immediately on device removal, but if the "dev" sysfs file is open when a device is removed, sysfs will access its attribute structure for further access including close resulting in jumping to garbled address. Fix it by postponing freeing devt_attr to device release time.

Quoted 8 lines

Quoted 1 line
Note that devt_attr for class_device is already freed on release.
Hi Tejun,
your rework removes the "owner" field from the attributes. I think we kept the "dev" and "uevent" attribute as part of "struct device" only to be able to assign it the actual owner of the module that has created the device. The attribute can probably just live as one instance statically in the driver core now?

Yeah, that's -mm and this is for -stable and -22. For -mm, we can just make all those attributes static which is the other patch is this thread. :-)

Signature · 1 line

tejun

Alan Stern

2007-05-10 15:52 UTC #18 lore raw

Re: [PATCH] driver-core: don't free devt_attr till the device is released

In reply to Tejun Heo

Quoted 8 lines · Tejun Heo

Currently, devt_attr for the "dev" file is freed immediately on device removal, but if the "dev" sysfs file is open when a device is removed, sysfs will access its attribute structure for further access including close resulting in jumping to garbled address. Fix it by postponing freeing devt_attr to device release time.
Note that devt_attr for class_device is already freed on release.

Quoted 8 lines

This bug is reported by Chris Rankin as bugzilla bug#8198.
Signed-off-by Tejun Heo <[email protected]>
Cc Chris Rankin <[email protected]>
---
Applies well to 2.6.20 and 21. As sysfs-immediate-disconnect doesn't seem to be included in 2.6.22, this should be included in linus#master too (applies well there as well).

Although sysfs-immediate-disconnect may not be included in 2.6.22, the old attribute-orphan code by Oliver Neukum is present there and also in 2.6.21. Shouldn't that suffice?

Alan Stern

Tejun Heo

author

2007-05-10 16:18 UTC #19 lore raw

Re: [PATCH] driver-core: don't free devt_attr till the device is released

Quoted 19 lines · Alan Stern

Quoted 8 lines · Tejun Heo
Currently, devt_attr for the "dev" file is freed immediately on device removal, but if the "dev" sysfs file is open when a device is removed, sysfs will access its attribute structure for further access including close resulting in jumping to garbled address. Fix it by postponing freeing devt_attr to device release time.
Note that devt_attr for class_device is already freed on release.
Quoted 8 lines
This bug is reported by Chris Rankin as bugzilla bug#8198.
Signed-off-by Tejun Heo <[email protected]>
Cc Chris Rankin <[email protected]>
---
Applies well to 2.6.20 and 21. As sysfs-immediate-disconnect doesn't seem to be included in 2.6.22, this should be included in linus#master too (applies well there as well).

Quoted 3 lines

Although sysfs-immediate-disconnect may not be included in 2.6.22, the old attribute-orphan code by Oliver Neukum is present there and also in 2.6.21. Shouldn't that suffice?

sysfs_release() still needs to deference attr->owner to put it, so I think there's the same problem even with attribute-orphan. We end up calling module_put() on garbage.

Signature · 1 line

tejun

Tejun Heo

author

2007-05-10 14:26 UTC #20 lore raw

PATCH 1/1 driver-core: make devt_attr and uevent_attr static

In reply to Greg KH

devt_attr and uevent_attr are either allocated dynamically with or embedded in device and class_device as they needed their owner field set to the module implementing the driver. Now that sysfs implements immediate disconnect and owner field removed from struct attribute, there is no reason to do this. Remove these attributes from [class_]device and use static attribute structures instead.

Signed-off-by Tejun Heo <[email protected]>

---
 drivers/base/class.c   |   44 ++++++++++++++++----------------------------
 drivers/base/core.c    |   45 +++++++++++++++------------------------------
 include/linux/device.h |    5 -----
 3 files changed, 31 insertions(+), 63 deletions(-)

drivers/base/class.c +16 ~~-28~~

Index: tree0/drivers/base/class.c
===================================================================
@@ -312,9 +312,6 @@ static void class_dev_release(struct kob
 
 	pr_debug("device class '%s': release.\n", cd->class_id);
 
-	kfree(cd->devt_attr);
-	cd->devt_attr = NULL;
-
 	if (cd->release)
 		cd->release(cd);
 	else if (cls->release)

@@ -564,6 +561,9 @@ static ssize_t show_dev(struct class_dev
 	return print_dev_t(buf, class_dev->devt);
 }
 
+static struct class_device_attribute class_devt_attr =
+	__ATTR(dev, S_IRUGO, show_dev, NULL);
+
 static ssize_t store_uevent(struct class_device *class_dev,
 			    const char *buf, size_t count)
 {

@@ -571,6 +571,9 @@ static ssize_t store_uevent(struct class
 	return count;
 }
 
+static struct class_device_attribute class_uevent_attr =
+	__ATTR(uevent, S_IWUSR, NULL, store_uevent);
+
 void class_device_initialize(struct class_device *class_dev)
 {
 	kobj_set_kset_s(class_dev, class_obj_subsys);

@@ -620,30 +623,15 @@ int class_device_add(struct class_device
 				  &parent_class->subsys.kobj, "subsystem");
 	if (error)
 		goto out3;
-	class_dev->uevent_attr.attr.name = "uevent";
-	class_dev->uevent_attr.attr.mode = S_IWUSR;
-	class_dev->uevent_attr.store = store_uevent;
-	error = class_device_create_file(class_dev, &class_dev->uevent_attr);
+
+	error = class_device_create_file(class_dev, &class_uevent_attr);
 	if (error)
 		goto out3;
 
 	if (MAJOR(class_dev->devt)) {
-		struct class_device_attribute *attr;
-		attr = kzalloc(sizeof(*attr), GFP_KERNEL);
-		if (!attr) {
-			error = -ENOMEM;
-			goto out4;
-		}
-		attr->attr.name = "dev";
-		attr->attr.mode = S_IRUGO;
-		attr->show = show_dev;
-		error = class_device_create_file(class_dev, attr);
-		if (error) {
-			kfree(attr);
+		error = class_device_create_file(class_dev, &class_devt_attr);
+		if (error)
 			goto out4;
-		}
-
-		class_dev->devt_attr = attr;
 	}
 
 	error = class_device_add_attrs(class_dev);

@@ -686,10 +674,10 @@ int class_device_add(struct class_device
  out6:
 	class_device_remove_attrs(class_dev);
  out5:
-	if (class_dev->devt_attr)
-		class_device_remove_file(class_dev, class_dev->devt_attr);
+	if (MAJOR(class_dev->devt))
+		class_device_remove_file(class_dev, &class_devt_attr);
  out4:
-	class_device_remove_file(class_dev, &class_dev->uevent_attr);
+	class_device_remove_file(class_dev, &class_uevent_attr);
  out3:
 	kobject_del(&class_dev->kobj);
  out2:

@@ -789,9 +777,9 @@ void class_device_del(struct class_devic
 		sysfs_remove_link(&class_dev->kobj, "device");
 	}
 	sysfs_remove_link(&class_dev->kobj, "subsystem");
-	class_device_remove_file(class_dev, &class_dev->uevent_attr);
-	if (class_dev->devt_attr)
-		class_device_remove_file(class_dev, class_dev->devt_attr);
+	class_device_remove_file(class_dev, &class_uevent_attr);
+	if (MAJOR(class_dev->devt))
+		class_device_remove_file(class_dev, &class_devt_attr);
 	class_device_remove_attrs(class_dev);
 	class_device_remove_groups(class_dev);

drivers/base/core.c +15 ~~-30~~

92 lines not on this page · raw message

include/linux/device.h +0 -5

Index: tree0/include/linux/device.h
===================================================================
@@ -240,7 +240,6 @@ extern int __must_check class_device_cre
  * @devt: for internal use by the driver core only.
  * @node: for internal use by the driver core only.
  * @kobj: for internal use by the driver core only.
- * @devt_attr: for internal use by the driver core only.
  * @groups: optional additional groups to be created
  * @dev: if set, a symlink to the struct device is created in the sysfs
  * directory for this struct class device.

@@ -265,8 +264,6 @@ struct class_device {
 	struct kobject		kobj;
 	struct class		* class;	/* required */
 	dev_t			devt;		/* dev_t, creates the sysfs "dev" */
-	struct class_device_attribute *devt_attr;
-	struct class_device_attribute uevent_attr;
 	struct device		* dev;		/* not necessary, but nice to have */
 	void			* class_data;	/* class-specific data */
 	struct class_device	*parent;	/* parent of this child device, if there is one */

@@ -421,8 +418,6 @@ struct device {
 	struct device_type	*type;
 	unsigned		is_registered:1;
 	unsigned		uevent_suppress:1;
-	struct device_attribute uevent_attr;
-	struct device_attribute *devt_attr;
 
 	struct semaphore	sem;	/* semaphore to synchronize calls to
 					 * its driver.