/sys/devices/system/timer registered twice

PATCH New Kay Sievers 4 participants 9 messages Nov 9, 2004 – Nov 12, 2004

Subsystems (2)

Library code Driver core

lore

Share & tools

lore

Kay Sievers

author

2004-11-09 19:30 UTC #0 lore raw

Hi,
I got this on a Centrino box with the latest bk:

[kay@pim linux.kay]$ ls -l /sys/devices/system/
total 0

  drwxr-xr-x  7 root root 0 Nov  8 15:12 .
  drwxr-xr-x  5 root root 0 Nov  8 15:12 ..
  drwxr-xr-x  3 root root 0 Nov  8 15:12 cpu
  drwxr-xr-x  3 root root 0 Nov  8 15:12 i8259
  drwxr-xr-x  2 root root 0 Nov  8 15:12 ioapic
  drwxr-xr-x  3 root root 0 Nov  8 15:12 irqrouter
  ?---------  ? ?    ?    ?            ? timer

It is caused by registering two devices with the name "timer" from:

arch/i386/kernel/time.c
arch/i386/kernel/timers/timer_pit.c

If I change one of the names, I get two correct looking sysfs entries.

Greg, shouldn't the driver core prevent the corruption of the first device if another one tries to register with the same name?

Thanks,
Kay

Greg KH

2004-11-09 19:40 UTC #1 lore raw

Quoted 16 lines · Kay Sievers

Hi,
I got this on a Centrino box with the latest bk:
[kay@pim linux.kay]$ ls -l /sys/devices/system/
total 0
  drwxr-xr-x  7 root root 0 Nov  8 15:12 .
  drwxr-xr-x  5 root root 0 Nov  8 15:12 ..
  drwxr-xr-x  3 root root 0 Nov  8 15:12 cpu
  drwxr-xr-x  3 root root 0 Nov  8 15:12 i8259
  drwxr-xr-x  2 root root 0 Nov  8 15:12 ioapic
  drwxr-xr-x  3 root root 0 Nov  8 15:12 irqrouter
  ?---------  ? ?    ?    ?            ? timer
It is caused by registering two devices with the name "timer" from:

Quoted 7 lines

arch/i386/kernel/time.c
arch/i386/kernel/timers/timer_pit.c
If I change one of the names, I get two correct looking sysfs entries.
Greg, shouldn't the driver core prevent the corruption of the first device if another one tries to register with the same name?

Yes, we should handle this. Can you try the patch below? I just sent it to Linus, as it fixes a bug that was recently introduced.

The second registration should fail, and this patch will make it fail, and recover properly.

thanks,

greg k-h

lib/kobject.c +1 -1

@@ -181,10 +181,10 @@ int kobject_add(struct kobject * kobj)
 
 	error = create_dir(kobj);
 	if (error) {
+		/* Does the kobject_put() for us */
 		unlink(kobj);
 		if (parent)
 			kobject_put(parent);
-		kobject_put(kobj);
 	} else {
 		kobject_hotplug(kobj, KOBJ_ADD);
 	}

Kay Sievers

author

2004-11-10 02:25 UTC #2 lore raw

Quoted 22 lines · Greg KH

Quoted 21 lines · Kay Sievers
Hi,
I got this on a Centrino box with the latest bk:
[kay@pim linux.kay]$ ls -l /sys/devices/system/
total 0
  drwxr-xr-x  7 root root 0 Nov  8 15:12 .
  drwxr-xr-x  5 root root 0 Nov  8 15:12 ..
  drwxr-xr-x  3 root root 0 Nov  8 15:12 cpu
  drwxr-xr-x  3 root root 0 Nov  8 15:12 i8259
  drwxr-xr-x  2 root root 0 Nov  8 15:12 ioapic
  drwxr-xr-x  3 root root 0 Nov  8 15:12 irqrouter
  ?---------  ? ?    ?    ?            ? timer
It is caused by registering two devices with the name "timer" from:
arch/i386/kernel/time.c
arch/i386/kernel/timers/timer_pit.c
If I change one of the names, I get two correct looking sysfs entries.

Quoted 8 lines

Quoted 2 lines
Greg, shouldn't the driver core prevent the corruption of the first device if another one tries to register with the same name?
Yes, we should handle this. Can you try the patch below? I just sent it to Linus, as it fixes a bug that was recently introduced.
The second registration should fail, and this patch will make it fail, and recover properly.

Yes, the registration fails. But it seems that the second call to create_dir(kobj) with a kobject with the same name and parent corrupts the dentry from the first call.

To test it, I just called create_dir(kobj) a second time for my video driver and the sysfs entry of the successful registered kobject was corrupted:

[kay@pim ~]$ ls -la /sys/class/video4linux/
total 0
drwxr-xr-x 3 root root 0 Nov 10 02:53 .
drwxr-xr-x 18 root root 0 Nov 10 02:53 ..
?--------- ? ? ? ? ? video0

Thanks,
Kay

Maneesh Soni

2004-11-10 22:38 UTC #3 lore raw

Quoted 32 lines · Kay Sievers

Quoted 22 lines · Greg KH
Quoted 21 lines · Kay Sievers
Hi,
I got this on a Centrino box with the latest bk:
[kay@pim linux.kay]$ ls -l /sys/devices/system/
total 0
  drwxr-xr-x  7 root root 0 Nov  8 15:12 .
  drwxr-xr-x  5 root root 0 Nov  8 15:12 ..
  drwxr-xr-x  3 root root 0 Nov  8 15:12 cpu
  drwxr-xr-x  3 root root 0 Nov  8 15:12 i8259
  drwxr-xr-x  2 root root 0 Nov  8 15:12 ioapic
  drwxr-xr-x  3 root root 0 Nov  8 15:12 irqrouter
  ?---------  ? ?    ?    ?            ? timer
It is caused by registering two devices with the name "timer" from:
arch/i386/kernel/time.c
arch/i386/kernel/timers/timer_pit.c
If I change one of the names, I get two correct looking sysfs entries.
Quoted 8 lines
Quoted 2 lines
Greg, shouldn't the driver core prevent the corruption of the first device if another one tries to register with the same name?
Yes, we should handle this. Can you try the patch below? I just sent it to Linus, as it fixes a bug that was recently introduced.
The second registration should fail, and this patch will make it fail, and recover properly.

Quoted 7 lines

Yes, the registration fails. But it seems that the second call to create_dir(kobj) with a kobject with the same name and parent corrupts the dentry from the first call.
To test it, I just called create_dir(kobj) a second time for my video driver and the sysfs entry of the successful registered kobject was corrupted:

Quoted 6 lines

[kay@pim ~]$ ls -la /sys/class/video4linux/
total 0
drwxr-xr-x 3 root root 0 Nov 10 02:53 .
drwxr-xr-x 18 root root 0 Nov 10 02:53 ..
?--------- ? ? ? ? ? video0

Thanks for reporting this bug. I think the problem here is doing d_drop() for the existing directory dentry in create_dir() for error case. It should not be done for EEXIST. Could you please try the same test with the following patch applied.

Thanks
Maneesh

o Do not release existing directory if the new directory happens to be a duplicate directory. Thanks to Kay Sievers for the testcase.

Signed-off-by <[email protected]>

---

 linux-2.6.10-rc1-bk20-maneesh/fs/sysfs/dir.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

fs/sysfs/dir.c renamed from fs/sysfs/dir.c~fix-dropping-existing-dir +1 -1

diff -puN fs/sysfs/dir.c~fix-dropping-existing-dir fs/sysfs/dir.c
@@ -111,7 +111,7 @@ static int create_dir(struct kobject * k
 				d_rehash(*d);
 			}
 		}
-		if (error)
+		if (error && (error != -EEXIST))
 			d_drop(*d);
 		dput(*d);
 	} else

Signature · 6 lines

Maneesh Soni
Linux Technology Center, 
IBM Austin
email: [email protected]
Phone: 1-512-838-1896 Fax: 
T/L : 6781896

Kay Sievers

author

2004-11-11 00:15 UTC #4 lore raw

Quoted 48 lines · Maneesh Soni

Quoted 32 lines · Kay Sievers
Quoted 22 lines · Greg KH
Quoted 21 lines · Kay Sievers
Hi,
I got this on a Centrino box with the latest bk:
[kay@pim linux.kay]$ ls -l /sys/devices/system/
total 0
  drwxr-xr-x  7 root root 0 Nov  8 15:12 .
  drwxr-xr-x  5 root root 0 Nov  8 15:12 ..
  drwxr-xr-x  3 root root 0 Nov  8 15:12 cpu
  drwxr-xr-x  3 root root 0 Nov  8 15:12 i8259
  drwxr-xr-x  2 root root 0 Nov  8 15:12 ioapic
  drwxr-xr-x  3 root root 0 Nov  8 15:12 irqrouter
  ?---------  ? ?    ?    ?            ? timer
It is caused by registering two devices with the name "timer" from:
arch/i386/kernel/time.c
arch/i386/kernel/timers/timer_pit.c
If I change one of the names, I get two correct looking sysfs entries.
Quoted 8 lines
Quoted 2 lines
Greg, shouldn't the driver core prevent the corruption of the first device if another one tries to register with the same name?
Yes, we should handle this. Can you try the patch below? I just sent it to Linus, as it fixes a bug that was recently introduced.
The second registration should fail, and this patch will make it fail, and recover properly.
Quoted 7 lines
Yes, the registration fails. But it seems that the second call to create_dir(kobj) with a kobject with the same name and parent corrupts the dentry from the first call.
To test it, I just called create_dir(kobj) a second time for my video driver and the sysfs entry of the successful registered kobject was corrupted:
	> 
Quoted 6 lines
[kay@pim ~]$ ls -la /sys/class/video4linux/
total 0
drwxr-xr-x 3 root root 0 Nov 10 02:53 .
drwxr-xr-x 18 root root 0 Nov 10 02:53 ..
?--------- ? ? ? ? ? video0

Quoted 4 lines

Thanks for reporting this bug. I think the problem here is doing d_drop() for the existing directory dentry in create_dir() for error case. It should not be done for EEXIST. Could you please try the same test with the following patch applied.

My second create_dir(), also and the double "timer" registration do not corrupt the sysfs directory from the first call anymore. Nice fix!

Thanks,
Kay

Greg KH

Applied

2004-11-12 21:31 UTC #5 lore raw

In reply to Maneesh Soni

Quoted 53 lines · Maneesh Soni

Quoted 32 lines · Kay Sievers
Quoted 22 lines · Greg KH
Quoted 21 lines · Kay Sievers
Hi,
I got this on a Centrino box with the latest bk:
[kay@pim linux.kay]$ ls -l /sys/devices/system/
total 0
  drwxr-xr-x  7 root root 0 Nov  8 15:12 .
  drwxr-xr-x  5 root root 0 Nov  8 15:12 ..
  drwxr-xr-x  3 root root 0 Nov  8 15:12 cpu
  drwxr-xr-x  3 root root 0 Nov  8 15:12 i8259
  drwxr-xr-x  2 root root 0 Nov  8 15:12 ioapic
  drwxr-xr-x  3 root root 0 Nov  8 15:12 irqrouter
  ?---------  ? ?    ?    ?            ? timer
It is caused by registering two devices with the name "timer" from:
arch/i386/kernel/time.c
arch/i386/kernel/timers/timer_pit.c
If I change one of the names, I get two correct looking sysfs entries.
Quoted 8 lines
Quoted 2 lines
Greg, shouldn't the driver core prevent the corruption of the first device if another one tries to register with the same name?
Yes, we should handle this. Can you try the patch below? I just sent it to Linus, as it fixes a bug that was recently introduced.
The second registration should fail, and this patch will make it fail, and recover properly.
Quoted 7 lines
Yes, the registration fails. But it seems that the second call to create_dir(kobj) with a kobject with the same name and parent corrupts the dentry from the first call.
To test it, I just called create_dir(kobj) a second time for my video driver and the sysfs entry of the successful registered kobject was corrupted:
	> 
Quoted 6 lines
[kay@pim ~]$ ls -la /sys/class/video4linux/
total 0
drwxr-xr-x 3 root root 0 Nov 10 02:53 .
drwxr-xr-x 18 root root 0 Nov 10 02:53 ..
?--------- ? ? ? ? ? video0
Thanks for reporting this bug. I think the problem here is doing d_drop() for the existing directory dentry in create_dir() for error case. It should not be done for EEXIST. Could you please try the same test with the following patch applied.

Quoted 7 lines

Thanks
Maneesh
o Do not release existing directory if the new directory happens to be a duplicate directory. Thanks to Kay Sievers for the testcase.
Signed-off-by <[email protected]>

Applied, thanks.

greg k-h

Greg KH

2004-11-09 22:53 UTC #6 lore raw

In reply to Kay Sievers

Quoted 16 lines · Kay Sievers

Hi,
I got this on a Centrino box with the latest bk:
[kay@pim linux.kay]$ ls -l /sys/devices/system/
total 0
  drwxr-xr-x  7 root root 0 Nov  8 15:12 .
  drwxr-xr-x  5 root root 0 Nov  8 15:12 ..
  drwxr-xr-x  3 root root 0 Nov  8 15:12 cpu
  drwxr-xr-x  3 root root 0 Nov  8 15:12 i8259
  drwxr-xr-x  2 root root 0 Nov  8 15:12 ioapic
  drwxr-xr-x  3 root root 0 Nov  8 15:12 irqrouter
  ?---------  ? ?    ?    ?            ? timer
It is caused by registering two devices with the name "timer" from:

Quoted 7 lines

arch/i386/kernel/time.c
arch/i386/kernel/timers/timer_pit.c
If I change one of the names, I get two correct looking sysfs entries.
Greg, shouldn't the driver core prevent the corruption of the first device if another one tries to register with the same name?

Hm, this looks like an issue for Dmitry, as there shouldn't be too sysdev_class structures with the same name, right?

thanks,

greg k-h

Dmitry Torokhov

2004-11-09 23:19 UTC #7 lore raw

Quoted 22 lines · Greg KH

Quoted 19 lines · Kay Sievers
Hi,
I got this on a Centrino box with the latest bk:
[kay@pim linux.kay]$ ls -l /sys/devices/system/
total 0
  drwxr-xr-x  7 root root 0 Nov  8 15:12 .
  drwxr-xr-x  5 root root 0 Nov  8 15:12 ..
  drwxr-xr-x  3 root root 0 Nov  8 15:12 cpu
  drwxr-xr-x  3 root root 0 Nov  8 15:12 i8259
  drwxr-xr-x  2 root root 0 Nov  8 15:12 ioapic
  drwxr-xr-x  3 root root 0 Nov  8 15:12 irqrouter
  ?---------  ? ?    ?    ?            ? timer
It is caused by registering two devices with the name "timer" from:
arch/i386/kernel/time.c
arch/i386/kernel/timers/timer_pit.c

Quoted 8 lines

Quoted 4 lines
If I change one of the names, I get two correct looking sysfs entries.
Greg, shouldn't the driver core prevent the corruption of the first device if another one tries to register with the same name?
Hm, this looks like an issue for Dmitry, as there shouldn't be too sysdev_class structures with the same name, right?

I agree, but I think you got the wrong man here ;) You need to talk to Venkatesh.

Signature · 1 line

Dmitry

Greg KH

2004-11-09 23:31 UTC #8 lore raw

PATCH 1/1 timer: fix up problem where two sysdev_class devices had the same name.

Thanks to Kay Sievers for reporting this.

Was caused by a change from Venkatesh Pallipadi as seen at:
http://linux.bkbits.net:8080/linux-2.5/cset@41810e4aGZ0E5bn_hMb4JgIY5u90zA

Signed-off-by Greg Kroah-Hartman <[email protected]>

arch/i386/kernel/timers/timer_pit.c +1 -1

diff -Nru a/arch/i386/kernel/timers/timer_pit.c b/arch/i386/kernel/timers/timer_pit.c
@@ -181,7 +181,7 @@
 }
 
 static struct sysdev_class timer_sysclass = {
-	set_kset_name("timer"),
+	set_kset_name("timer_pit"),
 	.resume	= timer_resume,
 };