class_device_add error in SCSI with 2.6.17-rc2-g52824b6b

Hello James,

While booting 2.6.17-rc2-g52824b6b on an alpha (a 164LX clone), I get the following error messages for every sd devices that get registered:

sd 0:0:0:0: Attached scsi disk sda
kobject_add failed for 0:0: with -EEXIST, don't try to register things with the same name in the same directory. fffffc0000e4bc88 0000000000100100 fffffc000045c398 fffffc0000ec9910

       fffffc0000ec9910 0000000000000000 fffffc0000688080 0000000000100100 
       fffffc0000488b84 fffffc0000ec9910 fffffc0000ec9500 fffffc0000ec9900 
       fffffc0000ec1800 0000000000000000 fffffc0000ec18e8 fffffc0000ec1a18 
       0000000000000000 ffffffffffffffea fffffc000045ac8c fffffc0000ec18e8 
       0000000000000000 fffffc0000687f50 fffffc0000683d48 0000000000000000 
Trace:
[<fffffc000045c398>] class_device_add+0xb4/0x388
[<fffffc0000488b84>] sd_probe+0x154/0x4cc
[<fffffc000045ac8c>] driver_probe_device+0x6c/0xf0
[<fffffc000045ae90>] __driver_attach+0x9c/0x11c
[<fffffc0000459bac>] bus_for_each_dev+0x5c/0xb0
[<fffffc000045adf4>] __driver_attach+0x0/0x11c
[<fffffc000045af3c>] driver_attach+0x2c/0x40
[<fffffc000045a390>] bus_add_driver+0xa8/0x1d4
[<fffffc000045b644>] driver_register+0xa8/0xc0
[<fffffc0000476018>] scsi_register_driver+0x24/0x38
[<fffffc0000310200>] init+0x11c/0x360
[<fffffc0000311598>] kernel_thread+0x28/0x90
[<fffffc0000311580>] kernel_thread+0x10/0x90

Looking at 'git whatchanged' for drivers/scsi/sd.c, I guess (just a wild guess, I'll bisect tomorrow if needed) this patch probably broke the whole thing:

diff-tree 6bdaa1f17dd32ec62345c7b57842f53e6278a2fa (from 5baba830e93732e802dc7e0a362eb730e1917f58)

Author: James Bottomley <[email protected]>
Date:   Sat Mar 18 14:14:21 2006 -0600

[SCSI] allow displaying and setting of cache type via sysfs

I think I promised to do this two years ago

This patch adds a scsi_disk class with the cache type and FUA parameters, so user land application can easily obtain them without having to parse dmesg. It also allows setting the cache type (use with care...)

This patch is a bit dangerous because I've replaced the disk kref with a class device reference ...

Looking at the backtrace, this portion of the patch appears to be failing:

@@ -1566,7 +1642,16 @@ static int sd_probe(struct device *dev)

        if (error)
                goto out_put;

+       class_device_initialize(&sdkp->cdev);
+       sdkp->cdev.dev = &sdp->sdev_gendev;
+       sdkp->cdev.class = &sd_disk_class;
+       strncpy(sdkp->cdev.class_id, sdp->sdev_gendev.bus_id, BUS_ID_SIZE);

+
+ if (class_device_add(&sdkp->cdev))
+ goto out_put;
+

        get_device(&sdp->sdev_gendev);
+
        sdkp->device = sdp;
        sdkp->driver = &sd_template;
        sdkp->disk = gd;

I'll look some more at the whole thing tomorrow as I'm not familiar at all with kobject/sysfs...

Cheers,

Mathieu Chouquet-Stringer                         [email protected]

Similar error previously reported by me for 2.6.17-rc1, except sda got added fine: error occurred when attempting to add/register sdb. Thankfully, you were able to append a trace...

I'll be trying the just-released 2.6.17-rc2 this evening. Report to follow, and if there's a problem, I'll attempt to provide the trace (will have to be transcribed by hand, as system won't come up far enough for syslog to capture anything).

-----------------------------------------------------------------------
Bob Tracy                   WTO + WIPO = DMCA? http://www.anti-dmca.org
[email protected]
-----------------------------------------------------------------------

Actually you're right. I'm rereading the logs and the first device on a scsi bus is always registered correctly. The sh*t hits the fan when you have more than one sd device per bus. (I've got 2 devices on one, and 3 on the other and only the first one on each got registered correctly).

I used the serial console.

If syslog doesn't come up and depending on the state of your system you could redirect dmesg output to a file.

Mathieu Chouquet-Stringer                           [email protected]

In my case, I was able to solve the problem by replacing this call at line 1648 in drivers/scsi/sd.c (patch attached):
strncpy(sdkp->cdev.class_id, sdp->sdev_gendev.bus_id, BUS_ID_SIZE);

by

snprintf(sdkp->cdev.class_id, BUS_ID_SIZE, "%s", sdp->sdev_gendev.bus_id);

Then it works.

While debugging the whole thing, I was printk'ing the value of sdkp->cdev.class_id right after the strncpy call and here's what I getting (basically only the first 4 chars + final \0):
"0:0:"

So I guess the strncpy routine on alpha is fscked up or gcc is doing something crazy. The function is under arch/alpha/lib/strncpy.S, time to learn assembly.

@@ -1645,7 +1645,8 @@
 	class_device_initialize(&sdkp->cdev);
 	sdkp->cdev.dev = &sdp->sdev_gendev;
 	sdkp->cdev.class = &sd_disk_class;
-	strncpy(sdkp->cdev.class_id, sdp->sdev_gendev.bus_id, BUS_ID_SIZE);
+	snprintf(sdkp->cdev.class_id, BUS_ID_SIZE, "%s",
+		sdp->sdev_gendev.bus_id);
 
 	if (class_device_add(&sdkp->cdev))
 		goto out_put;

Mathieu Chouquet-Stringer                           [email protected]
    "Le disparu, si l'on vénère sa mémoire, est plus présent et
                 plus puissant que le vivant".
           -- Antoine de Saint-Exupéry, Citadelle --

-
To unsubscribe from this list: send the line "unsubscribe linux-alpha" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Replying to myself here, i've created the following test program and redefined strncpy to mystrncpy (I used strncpy.S and stxncpy.S from arch/alpha/lib):

=============================================================================
#include <stdio.h>
#include <string.h>
#define FOO 50

extern char *mystrncpy(char *dest, const char *src, size_t count);

int main(int argc, char **argv) {

	char src[FOO] = "";
	char dest[FOO];
	char letter[] = "a";
	int i;

	for (i = 0; i < FOO - 1; i++) {
		size_t beflen, aftlen;

		letter[0] = 'a' + i;
		strncat(src, letter, FOO);
		beflen = strlen(src);

		mystrncpy(dest, src, FOO);
		aftlen = strlen(dest);
		if (beflen != aftlen)
			printf("fails for strlen = %ld (copied %ld)\n",
				beflen, aftlen);
	}


	return 0;
}

=============================================================================

And here's the output using gcc version 3.4.4 (Gentoo 3.4.4-r1, ssp-3.4.4-1.0, pie-8.7.8), note i didn't use flag except -Wall:

fails for strlen = 3 (copied 2) fails for strlen = 4 (copied 2) fails for strlen = 5 (copied 2) fails for strlen = 6 (copied 2) fails for strlen = 7 (copied 2) fails for strlen = 11 (copied 10) fails for strlen = 12 (copied 10) fails for strlen = 13 (copied 10) fails for strlen = 14 (copied 10) fails for strlen = 15 (copied 10) fails for strlen = 19 (copied 18) fails for strlen = 20 (copied 18) fails for strlen = 21 (copied 18) fails for strlen = 22 (copied 18) fails for strlen = 23 (copied 18) fails for strlen = 27 (copied 26) fails for strlen = 28 (copied 26) fails for strlen = 29 (copied 26) fails for strlen = 30 (copied 26) fails for strlen = 31 (copied 26) fails for strlen = 35 (copied 34) fails for strlen = 36 (copied 34) fails for strlen = 37 (copied 34) fails for strlen = 38 (copied 34) fails for strlen = 39 (copied 34) fails for strlen = 43 (copied 42) fails for strlen = 44 (copied 42) fails for strlen = 45 (copied 42) fails for strlen = 46 (copied 42) fails for strlen = 47 (copied 42)

So much for this function... I'll look at the assembly to see if I can understand what's going on: it always copy a multiple of 8 + 2 bytes (as in 8x + 2).

Mathieu Chouquet-Stringer                           [email protected]

[ some background here: this started as a kobject_add error from the scsi subsystem, now it appears the strncpy routine on Alpha is broken see http://lkml.org/lkml/fancy/2006/4/19/305 or
msgid 20060419213129.GA9148@localhost ]

Replying to myself here, i've created the following test program and redefined strncpy to mystrncpy (I used strncpy.S and stxncpy.S from arch/alpha/lib):

=============================================================================
#include <stdio.h>
#include <string.h>
#define FOO 50

extern char *mystrncpy(char *dest, const char *src, size_t count);

int main(int argc, char **argv)
{

	char src[FOO] = "";
	char dest[FOO];
	char letter[] = "a";
	int i;

	for (i = 0; i < FOO - 1; i++) {
		size_t beflen, aftlen;

		letter[0] = 'a' + i;
		strncat(src, letter, FOO);
		beflen = strlen(src);

		mystrncpy(dest, src, FOO);
		aftlen = strlen(dest);
		if (beflen != aftlen)
			printf("fails for strlen = %ld (copied %ld)\n",
				beflen, aftlen);
	}


	return 0;
}

=============================================================================

And here's the output using gcc version 3.4.4 (Gentoo 3.4.4-r1, ssp-3.4.4-1.0, pie-8.7.8), note i didn't use flag except -Wall:

fails for strlen = 3 (copied 2)
fails for strlen = 4 (copied 2)
fails for strlen = 5 (copied 2)
fails for strlen = 6 (copied 2)
fails for strlen = 7 (copied 2)
fails for strlen = 11 (copied 10)
fails for strlen = 12 (copied 10)
fails for strlen = 13 (copied 10)
fails for strlen = 14 (copied 10)
fails for strlen = 15 (copied 10)
fails for strlen = 19 (copied 18)
fails for strlen = 20 (copied 18)
fails for strlen = 21 (copied 18)
fails for strlen = 22 (copied 18)
fails for strlen = 23 (copied 18)
fails for strlen = 27 (copied 26)
fails for strlen = 28 (copied 26)
fails for strlen = 29 (copied 26)
fails for strlen = 30 (copied 26)
fails for strlen = 31 (copied 26)
fails for strlen = 35 (copied 34)
fails for strlen = 36 (copied 34)
fails for strlen = 37 (copied 34)
fails for strlen = 38 (copied 34)
fails for strlen = 39 (copied 34)
fails for strlen = 43 (copied 42)
fails for strlen = 44 (copied 42)
fails for strlen = 45 (copied 42)
fails for strlen = 46 (copied 42)
fails for strlen = 47 (copied 42)

So much for this function... I'll look at the assembly to see if I can understand what's going on: it always copy a multiple of 8 + 2 bytes (as in 8x + 2).

Mathieu Chouquet-Stringer                           [email protected]

Same code compiled with 2.95.3 fails too (I'll be trying 4.1.0 just for the kick of it, if it cross-compiles ok but I don't expect it to work either).

PS: Richard, we're having troubles with at least one of your function (strncpy) on alpha. It appears it doesn't copy the source string properly.
You can read a recap of the thread here:
http://groups.google.com/group/linux.kernel/browse_thread/thread/551729237671f997/11109932eae9fd93?tvc=2&q=group%3A*kernel*+strncpy+mathieu#11109932eae9fd93

Mathieu Chouquet-Stringer                           [email protected]

Broken binutils, maybe?

Ivan.

2.15.92.0.2 and 2.15 (cross compiled) fail. What's yours?

Mathieu Chouquet-Stringer                           [email protected]

Possible... On my Debian system, the installed binutils package *was* binutils_2.15-6. I just upgraded to binutils_2.16.1cvs20060117-1 with the related gcc and cpp packages for default 4.0 compilation, and we'll see if that makes any difference.

-----------------------------------------------------------------------
Bob Tracy                   WTO + WIPO = DMCA? http://www.anti-dmca.org
[email protected]
-----------------------------------------------------------------------

Stop the press, it's definitely a binutils issue. 2.16.1 doesn't trigger the error.

Mathieu Chouquet-Stringer                           [email protected]

Still not out of the woods here :-(. Built 2.6.17-rc2 with gcc-4.0 and binutils 2.16.91 (package name is binutils_2.16.1cvs20060117-1) and I'm still getting the kobject_add error.

Mathieu -- you mentioned testing with a cross-compile. Was that the case for your reported success? How about a native compile? I'm pretty sure this *is* a binutils issue, but we don't quite have it nailed down yet.

-----------------------------------------------------------------------
Bob Tracy                   WTO + WIPO = DMCA? http://www.anti-dmca.org
[email protected]
-----------------------------------------------------------------------

My reported success was with the cross-compiled test case.

I've recompiled 2.16.1 overnight on the alpha and i'm currently rebuilding the kernel. I'll keep you posted.

Mathieu Chouquet-Stringer                           [email protected]

The bad news is my test case, compiled with a native gcc version 3.4.4 and binutils version 2.16.1 doesn't work as expected. So maybe it's combination of gcc/binutils? I'm booting the new kernel just to confirm that 3.4.4 and 2.16.1 do not work.

FWIW, the cross-compiler environment was made of gcc 4.1.0 and binutils 2.16.1.

Mathieu Chouquet-Stringer                           [email protected]

A native gcc 3.4.4 and binutils 2.16.1 do not work... What should we try next?

Mathieu Chouquet-Stringer                           [email protected]

I'll try upgrading from gcc-4.0 to gcc-4.1, and if/when that has no effect, I'll go looking for a later binutils in Debian's "unstable" tree (I've already had to go to the "testing" tree to get beyond gcc-3 and binutils-2.15.X). Report to follow later today.

Item for consideration: what kind of optimization is enabled for your test case compile vs. what's being used for the kernel build? That's another variable we need to sort out. For what it's worth, I do *not* have CONFIG_CC_OPTIMIZE_FOR_SIZE enabled: the comment about "watch out for broken compilers" was enough to scare me off while we're trying to chase this down.

-----------------------------------------------------------------------
Bob Tracy                   WTO + WIPO = DMCA? http://www.anti-dmca.org
[email protected]
-----------------------------------------------------------------------

Ok.

Well I thought of this already and tried my test case without any flags but -Wall, with -O2 and -Os. Same result. I also compiled my test case with the options used by the kernel (which ATM isn't compiled with -Os), same thing.

I've attached to this email a tarball of what I use to test the compiler/binutils. It's faster than recompiling the whole kernel on these slow machines!

Mathieu Chouquet-Stringer                           [email protected]

Oops. I was using a wrong copy of strncpy.S (remained from previous __stxncpy() debugging). What's why I wasn't able to reproduce that...

It seems that the registers $24 and $27 are mixed up in strncpy(). This fixes your test case, please check if it fixes kernel problem as well.

Ivan.

@@ -43,8 +43,8 @@
 
 	.align	4
 $multiword:
-	subq	$24, 1, $2	# clear the final bits in the prev word
-	or	$2, $24, $2
+	subq	$27, 1, $2	# clear the final bits in the prev word
+	or	$2, $27, $2
 	zapnot	$1, $2, $1
 	subq	$18, 1, $18
 

@@ -70,8 +70,8 @@
 	bne	$18, 0b
 
 1:	ldq_u	$1, 0($16)	# clear the leading bits in the final word
-	subq	$27, 1, $2
-	or	$2, $27, $2
+	subq	$24, 1, $2
+	or	$2, $24, $2
 
 	zap	$1, $2, $1
 	stq_u	$1, 0($16)

With this patch applied, the test suite appears to work correctly with ALL versions of gcc on the Alpha to which I have access. The binutils package is currently the 2.16.1cvs20060413-1 version from Debian's "unstable" tree.

WITHOUT the patch, and everything else the same, gcc-4.0 and gcc-4.1 appear to work, but the gcc-3.3 build produces the bad behavior we've been seeing.

I'm currently building a 2.6.17-rc2 kernel with gcc-4.1, the binutils package mentioned above, and Ivan's patch. Report to follow later.

Pending a knowledgable peer review of Ivan's patch (no insult intended:
I'm not qualified), I'd say we're close to putting this to rest. If it turns out the patch is the correct fix, I'm genuinely concerned about how long this bug went undetected :-(. The modification date of arch/alpha/lib/strncpy.S is 28 Jul 2003 in my tree. The stxncpy.S file is not quite a year newer: 10 May 2004.

-----------------------------------------------------------------------
Bob Tracy                   WTO + WIPO = DMCA? http://www.anti-dmca.org
[email protected]
-----------------------------------------------------------------------

Actually the bug doesn't show up with gcc-4 *only* because it tends to pack the data more tightly, so that both src[] and dest[] have different alignment vs. gcc-3 compiled foo.c:
src 0x11ffff8f4, dest 0x11ffff926 - gcc-4
src 0x11ffff8c0, dest 0x11ffff900 - gcc-3

If you add __attribute__((aligned(8))) to both src and dest declarations in Mathieu's foo.c, you'll see the bug is still here.

Well, these things happen. I think it's not quite surprising. First, the kernel is not overloaded with strncpy calls. ;-) Second, strncpy was mostly used in drivers that are rarely (if at all) used on alpha.
Third, to discover this bug you need some special combination of source and destination alignment, source string length and byte count.

Ivan.

As I mentioned in an earlier private message, I hope I can be forgiven for assuming otherwise, particularly since someone went to the trouble of writing an architecture-specific optimized version of that function.

Which was evidently the case, since this problem didn't surface until the strncpy() call was added to sd.c.

I'm happy to report my Alpha is now up and running on 2.6.17-rc2, so the fix works for me. Thanks to discussion participants for their time and trouble!

-----------------------------------------------------------------------
Bob Tracy                   WTO + WIPO = DMCA? http://www.anti-dmca.org
[email protected]
-----------------------------------------------------------------------

Yeah most of the drivers I was looking at use snprintf and friends.

Thanks to you too:

Mathieu Chouquet-Stringer                           [email protected]

Well done, it indeed fixed the test case. I'll try the kernel when I get back home...

While we're at it, next week, I'll exercise the other functions to make sure they also work as they should.

Thanks Ivan and Bob for your time, valuable feedback and work!!!

Mathieu Chouquet-Stringer                           [email protected]

For what it's worth, I've found gcc 3.4.4 to be bad on gentoo. If I compile any binutils-2.16.[01] versions with 3.4.4 ld segfaults when trying to compile gmp. I don't know of any binutils related problems when using gcc 3.4.6 currently.

Regards,
Bryan Østergaard
-
To unsubscribe from this list: send the line "unsubscribe linux-alpha" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

...

I'm unable to reproduce this with the toolchains that I have.

Ivan.

Ha, that's good news: which versions?

Mathieu Chouquet-Stringer                           [email protected]