/sys/devices/system/timer registered twice

/sys/devices/system/timer registered twice

[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.

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

Re: /sys/devices/system/timer registered twice

[Greg KH]

On Tue, Nov 09, 2004 at 08:30:43PM +0100, Kay Sievers wrote:
> 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?

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

--- a/lib/kobject.c	2004-11-05 10:06:33 -08:00
+++ b/lib/kobject.c	2004-11-08 23:58:02 -08:00
@@ -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);
 	}

Re: /sys/devices/system/timer registered twice

[Greg KH]

On Tue, Nov 09, 2004 at 08:30:43PM +0100, Kay Sievers wrote:
> 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?

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

Re: /sys/devices/system/timer registered twice

[Dmitry Torokhov]

On Tue, 9 Nov 2004 14:52:45 -0800, Greg KH <greg@kroah.com> wrote:
> 
> 
> On Tue, Nov 09, 2004 at 08:30:43PM +0100, Kay Sievers wrote:
> > 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?
> 
> 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.

http://linux.bkbits.net:8080/linux-2.5/cset@41810e4aGZ0E5bn_hMb4JgIY5u90zA?nav=index.html|src/.|src/arch|src/arch/i386|src/arch/i386/kernel|related/arch/i386/kernel/time.c

-- 
Dmitry

[PATCH] timer: fix up problem where two sysdev_class devices had the same name.

[Greg KH]

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 <greg@kroah.com>

diff -Nru a/arch/i386/kernel/timers/timer_pit.c b/arch/i386/kernel/timers/timer_pit.c
--- a/arch/i386/kernel/timers/timer_pit.c	2004-11-09 15:25:54 -08:00
+++ b/arch/i386/kernel/timers/timer_pit.c	2004-11-09 15:25:54 -08:00
@@ -181,7 +181,7 @@
 }
 
 static struct sysdev_class timer_sysclass = {
-	set_kset_name("timer"),
+	set_kset_name("timer_pit"),
 	.resume	= timer_resume,
 };
 

Re: /sys/devices/system/timer registered twice

[Kay Sievers]

On Tue, Nov 09, 2004 at 11:39:47AM -0800, Greg KH wrote:
> On Tue, Nov 09, 2004 at 08:30:43PM +0100, Kay Sievers wrote:
> > 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?
> 
> 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

Re: /sys/devices/system/timer registered twice

[Maneesh Soni]

On Wed, Nov 10, 2004 at 03:25:35AM +0100, Kay Sievers wrote:
> On Tue, Nov 09, 2004 at 11:39:47AM -0800, Greg KH wrote:
> > On Tue, Nov 09, 2004 at 08:30:43PM +0100, Kay Sievers wrote:
> > > 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?
> > 
> > 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 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: <maneesh@in.ibm.com>
---

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

diff -puN fs/sysfs/dir.c~fix-dropping-existing-dir fs/sysfs/dir.c
--- linux-2.6.10-rc1-bk20/fs/sysfs/dir.c~fix-dropping-existing-dir	2004-11-10 16:26:14.000000000 -0600
+++ linux-2.6.10-rc1-bk20-maneesh/fs/sysfs/dir.c	2004-11-10 16:27:42.000000000 -0600
@@ -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
_



-- 
Maneesh Soni
Linux Technology Center, 
IBM Austin
email: maneesh@in.ibm.com
Phone: 1-512-838-1896 Fax: 
T/L : 6781896

Re: /sys/devices/system/timer registered twice

[Kay Sievers]

On Wed, Nov 10, 2004 at 04:36:29PM -0600, Maneesh Soni wrote:
> On Wed, Nov 10, 2004 at 03:25:35AM +0100, Kay Sievers wrote:
> > On Tue, Nov 09, 2004 at 11:39:47AM -0800, Greg KH wrote:
> > > On Tue, Nov 09, 2004 at 08:30:43PM +0100, Kay Sievers wrote:
> > > > 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?
> > > 
> > > 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 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

Re: /sys/devices/system/timer registered twice

[Greg KH]

On Wed, Nov 10, 2004 at 04:36:29PM -0600, Maneesh Soni wrote:
> On Wed, Nov 10, 2004 at 03:25:35AM +0100, Kay Sievers wrote:
> > On Tue, Nov 09, 2004 at 11:39:47AM -0800, Greg KH wrote:
> > > On Tue, Nov 09, 2004 at 08:30:43PM +0100, Kay Sievers wrote:
> > > > 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?
> > > 
> > > 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 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: <maneesh@in.ibm.com>

Applied, thanks.

greg k-h

RE: /sys/devices/system/timer registered twice

[Pallipadi, Venkatesh]

>-----Original Message-----
>From: Greg KH [mailto:greg@kroah.com] 
>Sent: Tuesday, November 09, 2004 3:44 PM
>To: Pallipadi, Venkatesh
>Cc: dtor_core@ameritech.net; Kay Sievers; linux-kernel@vger.kernel.org
>Subject: Re: /sys/devices/system/timer registered twice
>
>On Tue, Nov 09, 2004 at 03:41:51PM -0800, Pallipadi, Venkatesh wrote:
>> 
>> But, do we really need two system devices for timers?. I feel 
>> we can call setup_pit_timer from time.c, whenever pit is being used.
>> Otherwise, we may have more issues like the order in which these 
>> two resumes are called and the like.
>
>I have no idea.  Why not work this out with the other system device
>authors, as it's obvious people will have both of them loaded at the
>same time.
>

Agreed. We can change the name of the one in timer_pit.c to timer_pit 
for now. That will fix the immediate breakage.
I will try to come up with a patch to combine the two, soon.

Thanks,
Venki

Re: /sys/devices/system/timer registered twice

[Greg KH]

On Tue, Nov 09, 2004 at 03:41:51PM -0800, Pallipadi, Venkatesh wrote:
> 
> But, do we really need two system devices for timers?. I feel 
> we can call setup_pit_timer from time.c, whenever pit is being used.
> Otherwise, we may have more issues like the order in which these 
> two resumes are called and the like.

I have no idea.  Why not work this out with the other system device
authors, as it's obvious people will have both of them loaded at the
same time.

thanks,

greg k-h

RE: /sys/devices/system/timer registered twice

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: linux-kernel-owner@vger.kernel.org 
>[mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of 
>Dmitry Torokhov
>Sent: Tuesday, November 09, 2004 3:19 PM
>To: Greg KH
>Cc: Kay Sievers; linux-kernel@vger.kernel.org
>Subject: Re: /sys/devices/system/timer registered twice
>
>On Tue, 9 Nov 2004 14:52:45 -0800, Greg KH <greg@kroah.com> wrote:
>> 
>> 
>> On Tue, Nov 09, 2004 at 08:30:43PM +0100, Kay Sievers wrote:
>> > 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?
>> 
>> 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.
>
>http://linux.bkbits.net:8080/linux-2.5/cset@41810e4aGZ0E5bn_hMb
>4JgIY5u90zA?nav=index.html|src/.|src/arch|src/arch/i386|src/arc
>h/i386/kernel|related/arch/i386/kernel/time.c
>

Yes. It was me :(.

But, do we really need two system devices for timers?. I feel 
we can call setup_pit_timer from time.c, whenever pit is being used.
Otherwise, we may have more issues like the order in which these 
two resumes are called and the like.

Thanks,
Venki