[RFC] printks in print_inquiry

[RFC] printks in print_inquiry

[Matthew Wilcox]

One of the side-effects of my previous patch to parallelise scsi
scanning is that it causes print_inquiry to run when there's already a
lot of other printk traffic.  Due to the way it works, it's very easy
for print_inquiry to be in the middle of printing something when
something else chooses to emit a printk.  This could of course happen
without the async scsi scanning patch, it's just a lot more frequent
with it.

This is one way of solving the problem -- accumulate each line into a
buffer, then display each line in one shot.  However, the comment above
the function says we should be packaging all this up into a hotplug
event.  Is that still true?  If so, I can do that, and we can forget
about this patch.

Index: ./drivers/scsi/scsi_scan.c
===================================================================
RCS file: /var/cvs/linux-2.6/drivers/scsi/scsi_scan.c,v
retrieving revision 1.38
diff -u -p -r1.38 scsi_scan.c
--- ./drivers/scsi/scsi_scan.c	19 Apr 2006 04:55:59 -0000	1.38
+++ ./drivers/scsi/scsi_scan.c	11 May 2006 13:18:42 -0000
@@ -148,43 +166,43 @@ static void scsi_unlock_floptical(struct
  **/
 static void print_inquiry(unsigned char *inq_result)
 {
-	int i;
+	int i, len;
+	static char buf[80];
 
-	printk(KERN_NOTICE "  Vendor: ");
+	len = sprintf(buf, "  Vendor: ");
 	for (i = 8; i < 16; i++)
 		if (inq_result[i] >= 0x20 && i < inq_result[4] + 5)
-			printk("%c", inq_result[i]);
+			buf[len++] = inq_result[i];
 		else
-			printk(" ");
+			buf[len++] = ' ';
 
-	printk("  Model: ");
+	len += sprintf(buf + len, "  Model: ");
 	for (i = 16; i < 32; i++)
 		if (inq_result[i] >= 0x20 && i < inq_result[4] + 5)
-			printk("%c", inq_result[i]);
+			buf[len++] = inq_result[i];
 		else
-			printk(" ");
+			buf[len++] = ' ';
 
-	printk("  Rev: ");
+	len += sprintf(buf + len, "  Rev: ");
 	for (i = 32; i < 36; i++)
 		if (inq_result[i] >= 0x20 && i < inq_result[4] + 5)
-			printk("%c", inq_result[i]);
+			buf[len++] = inq_result[i];
 		else
-			printk(" ");
+			buf[len++] = ' ';
 
-	printk("\n");
+	buf[len] = '\0';
+	printk(KERN_NOTICE "%s\n", buf);
 
 	i = inq_result[0] & 0x1f;
 
-	printk(KERN_NOTICE "  Type:   %s ",
-	       i <
-	       MAX_SCSI_DEVICE_CODE ? scsi_device_types[i] :
-	       "Unknown          ");
-	printk("                 ANSI SCSI revision: %02x",
-	       inq_result[2] & 0x07);
+	len = sprintf(buf, "  Type:   %s ", i < MAX_SCSI_DEVICE_CODE ?
+			scsi_device_types[i] : "Unknown          ");
+	len += sprintf(buf + len, "                 ANSI SCSI revision: %02x",
+			inq_result[2] & 0x07);
 	if ((inq_result[2] & 0x07) == 1 && (inq_result[3] & 0x0f) == 1)
-		printk(" CCS\n");
-	else
-		printk("\n");
+		len += sprintf(buf + len, " CCS\n");
+
+	printk(KERN_NOTICE "%s\n", buf);
 }
 
 /**

Re: [RFC] printks in print_inquiry

[Patrick Mansfield]

On Thu, May 11, 2006 at 09:00:15AM -0600, Matthew Wilcox wrote:
> 
> One of the side-effects of my previous patch to parallelise scsi
> scanning is that it causes print_inquiry to run when there's already a
> lot of other printk traffic.  Due to the way it works, it's very easy
> for print_inquiry to be in the middle of printing something when
> something else chooses to emit a printk.  This could of course happen
> without the async scsi scanning patch, it's just a lot more frequent
> with it.
> 
> This is one way of solving the problem -- accumulate each line into a
> buffer, then display each line in one shot.  However, the comment above

Terser one line output would be nice (even for a few devices), with "scsi"
and bus_id. We don't need "Type", like:

SCSI 2:0:0:0: Vendor: FOO      Model: BAR              Rev: 0.2 ANSI rev: 04

And maybe use printk("%-16s") formatting? But garbage might get printed
for non-ASCII (though the SCSI specs say it is not allowed ...).

> the function says we should be packaging all this up into a hotplug
> event.  Is that still true?  If so, I can do that, and we can forget
> about this patch.

There is already a hotplug event. 

But the timing and output of it won't match well with the other SCSI (sd)
messages, they'll likely be output before hotplug handling for the event.

And then if relying on udev for the output: it replays hotplug events when
it starts up, so (AFAICT) you'd get a flurry of output after all the other
scsi kernel messages for no udev in initrd and for monolithic kernels
(well any time udev is started after scsi discovery).

But try a udev rule like:

ACTION=="add", SUBSYSTEM=="scsi_device" RUN+="/sbin/scsi_notify"

And then /sbin/scsi_notify bash script like:

#! /bin/bash
sys=/sys
sdev=${sys}${PHYSDEVPATH}
bus_id=${sdev##*/}
logger -t scsi "$(printf "%s: Vendor: %-8s Model: %-16s\n" ${bus_id} \
        $(cat ${sdev}/vendor) $(cat ${sdev}/model))"

-- Patrick Mansfield

Re: [RFC] printks in print_inquiry

[Matthew Wilcox]

On Fri, May 12, 2006 at 10:08:54AM -0700, Patrick Mansfield wrote:
> Terser one line output would be nice (even for a few devices), with "scsi"
> and bus_id. We don't need "Type", like:
> 
> SCSI 2:0:0:0: Vendor: FOO      Model: BAR              Rev: 0.2 ANSI rev: 04
> 
> And maybe use printk("%-16s") formatting? But garbage might get printed
> for non-ASCII (though the SCSI specs say it is not allowed ...).

I'm certainly in favour of changing the formatting; possibly even
eliminating it.  It gets very tiresome on a large system.  But then it's
sometimes useful, particularly in bug reports.  So eliminating it may
be a step too far.

> > the function says we should be packaging all this up into a hotplug
> > event.  Is that still true?  If so, I can do that, and we can forget
> > about this patch.
> 
> There is already a hotplug event. 
> 
> But the timing and output of it won't match well with the other SCSI (sd)
> messages, they'll likely be output before hotplug handling for the event.
> 
> And then if relying on udev for the output: it replays hotplug events when
> it starts up, so (AFAICT) you'd get a flurry of output after all the other
> scsi kernel messages for no udev in initrd and for monolithic kernels
> (well any time udev is started after scsi discovery).

I wasn't thinking about relying on it for the output; more that if the
data's being parsed out of the inquiry string already, doing this second
conversion in print_inquiry is a bit daft.  And ugly.  It'd be better to
take the constructed hotplug event and print the relevant fields out of
that.  I'll fiddle with it some more.

Re: [RFC] printks in print_inquiry

[Matthew Wilcox]

On Fri, May 12, 2006 at 11:00:59PM -0600, Matthew Wilcox wrote:
> On Fri, May 12, 2006 at 10:08:54AM -0700, Patrick Mansfield wrote:
> > Terser one line output would be nice (even for a few devices), with "scsi"
> > and bus_id. We don't need "Type", like:
> > 
> > SCSI 2:0:0:0: Vendor: FOO      Model: BAR              Rev: 0.2 ANSI rev: 04

I went for:

scsi: 2:0:1:0: Vendor: HP 18.2G Model: ATLAS10K3_18_SCA Rev: HP05 ANSI rev: 02  

Lower case seems to have the edge over upper case by a factor of 10:1

willy@rowlf:~/kernel/pscan-2.6$ cat drivers/scsi/*.c | grep -c '".*scsi'        
1406
willy@rowlf:~/kernel/pscan-2.6$ cat drivers/scsi/*.c | grep -c '".*SCSI'
525
willy@rowlf:~/kernel/pscan-2.6$ cat drivers/scsi/*.c | grep -c '"scsi'
940
willy@rowlf:~/kernel/pscan-2.6$ cat drivers/scsi/*.c | grep -c '"SCSI'
90

Although it would match with sd.c's
SCSI device sda: 35566480 512-byte hdwr sectors (18210 MB)                      
and
SCSI device sda: drive cache: write through w/ FUA                              


Arguably we should just do away with that text though.  Look at it:

SCSI device sda: 35566480 512-byte hdwr sectors (18210 MB)                      
sda: Write Protect is off                                                       
SCSI device sda: drive cache: write through w/ FUA                              
 sda: sda1 sda2 sda3                                                            
sd 2:0:1:0: Attached scsi disk sda                                              
sd 2:0:1:0: Attached scsi generic sg0 type 0                                    


wouldn't this look better?

sda: 35566480 512-byte hdwr sectors (18210 MB)                      
sda: Write Protect is off                                                       
sda: drive cache: write through w/ FUA                              
 sda: sda1 sda2 sda3                                                            
sd 2:0:1:0: Attached scsi disk sda                                              
sd 2:0:1:0: Attached scsi generic sg0 type 0                                    

> > And maybe use printk("%-16s") formatting? But garbage might get printed
> > for non-ASCII (though the SCSI specs say it is not allowed ...).

I think you meant %.16s, not %-16s; it's already padded, we just need a
maximum byte count.

Anyway, I noticed that sdev has everything I need in it, and it's
definitely clearer than using the inquiry data directly.  The one thing
I don't do this for is scsi_level; we want what the device returned,
not what we've mangled it to.

The comment about BLIST_ISROM is a little too terse for me to know what
to do; can anyone hazard a guess?

Index: ./drivers/scsi/scsi_scan.c
===================================================================
RCS file: /var/cvs/linux-2.6/drivers/scsi/scsi_scan.c,v
retrieving revision 1.38
diff -u -p -r1.38 scsi_scan.c
--- ./drivers/scsi/scsi_scan.c	19 Apr 2006 04:55:59 -0000	1.38
+++ ./drivers/scsi/scsi_scan.c	18 May 2006 18:11:50 -0000
@@ -135,59 +176,6 @@ static void scsi_unlock_floptical(struct
 }
 
 /**
- * print_inquiry - printk the inquiry information
- * @inq_result:	printk this SCSI INQUIRY
- *
- * Description:
- *     printk the vendor, model, and other information found in the
- *     INQUIRY data in @inq_result.
- *
- * Notes:
- *     Remove this, and replace with a hotplug event that logs any
- *     relevant information.
- **/
-static void print_inquiry(unsigned char *inq_result)
-{
-	int i;
-
-	printk(KERN_NOTICE "  Vendor: ");
-	for (i = 8; i < 16; i++)
-		if (inq_result[i] >= 0x20 && i < inq_result[4] + 5)
-			printk("%c", inq_result[i]);
-		else
-			printk(" ");
-
-	printk("  Model: ");
-	for (i = 16; i < 32; i++)
-		if (inq_result[i] >= 0x20 && i < inq_result[4] + 5)
-			printk("%c", inq_result[i]);
-		else
-			printk(" ");
-
-	printk("  Rev: ");
-	for (i = 32; i < 36; i++)
-		if (inq_result[i] >= 0x20 && i < inq_result[4] + 5)
-			printk("%c", inq_result[i]);
-		else
-			printk(" ");
-
-	printk("\n");
-
-	i = inq_result[0] & 0x1f;
-
-	printk(KERN_NOTICE "  Type:   %s ",
-	       i <
-	       MAX_SCSI_DEVICE_CODE ? scsi_device_types[i] :
-	       "Unknown          ");
-	printk("                 ANSI SCSI revision: %02x",
-	       inq_result[2] & 0x07);
-	if ((inq_result[2] & 0x07) == 1 && (inq_result[3] & 0x0f) == 1)
-		printk(" CCS\n");
-	else
-		printk("\n");
-}
-
-/**
  * scsi_alloc_sdev - allocate and setup a scsi_Device
  *
  * Description:
@@ -654,9 +643,8 @@ static int scsi_add_lun(struct scsi_devi
 	if (*bflags & BLIST_ISROM) {
 		/*
 		 * It would be better to modify sdev->type, and set
-		 * sdev->removable, but then the print_inquiry() output
-		 * would not show TYPE_ROM; if print_inquiry() is removed
-		 * the issue goes away.
+		 * sdev->removable; this can now be done since
+		 * print_inquiry has gone away.
 		 */
 		inq_result[0] = TYPE_ROM;
 		inq_result[1] |= 0x80;	/* removable */
@@ -685,7 +673,9 @@ static int scsi_add_lun(struct scsi_devi
 		printk(KERN_INFO "scsi: unknown device type %d\n", sdev->type);
 	}
 
-	print_inquiry(inq_result);
+	sdev_printk(KERN_NOTICE "scsi:", sdev, "Vendor: %.8s Model: %.16s "
+			"Rev: %.4s ANSI rev: %02x\n", sdev->vendor,
+			sdev->model, sdev->rev, sdev->inquiry[2] & 0x07);
 
 	/*
 	 * For a peripheral qualifier (PQ) value of 1 (001b), the SCSI

Re: [RFC] printks in print_inquiry

[Patrick Mansfield]

On Thu, May 18, 2006 at 12:36:52PM -0600, Matthew Wilcox wrote:

> I went for:
> 
> scsi: 2:0:1:0: Vendor: HP 18.2G Model: ATLAS10K3_18_SCA Rev: HP05 ANSI rev: 02  
That is very nice ... as is replacing print_inquiry with one line of code.

> SCSI device sda: 35566480 512-byte hdwr sectors (18210 MB)                  
> sda: Write Protect is off                                                  
> SCSI device sda: drive cache: write through w/ FUA                        
>  sda: sda1 sda2 sda3                                                     
> sd 2:0:1:0: Attached scsi disk sda                                      
> sd 2:0:1:0: Attached scsi generic sg0 type 0                           
> 
> 
> wouldn't this look better?
> 
> sda: 35566480 512-byte hdwr sectors (18210 MB)                      
> sda: Write Protect is off                                                       
> sda: drive cache: write through w/ FUA                              
>  sda: sda1 sda2 sda3                                                  
> sd 2:0:1:0: Attached scsi disk sda                                   
> sd 2:0:1:0: Attached scsi generic sg0 type 0                        

Yes, better. I guess those should all be sdev_printk in sd.c.

Funky how loading sd after sg changes the output ... and using the driver
name as a prefix sometimes messes this up for scsi.

i.e. scan without sd_mod or sg loaded (and distro I'm using loads sg
before sd_mod via udev rules):

 0:0:0:0: Attached scsi generic sg0 type 0
 0:0:0:1: Attached scsi generic sg1 type 0

Then remove/add those devices, and sg lines become:

sd 1:0:0:0: Attached scsi generic sg0 type 0
sd 1:0:0:1: Attached scsi generic sg1 type 0

> > > And maybe use printk("%-16s") formatting? But garbage might get printed
> > > for non-ASCII (though the SCSI specs say it is not allowed ...).
> 
> I think you meant %.16s, not %-16s; it's already padded, we just need a
> maximum byte count.

Yes, I was thinking it was not padded.

> Anyway, I noticed that sdev has everything I need in it, and it's
> definitely clearer than using the inquiry data directly.  The one thing
> I don't do this for is scsi_level; we want what the device returned,
> not what we've mangled it to.
> 
> The comment about BLIST_ISROM is a little too terse for me to know what
> to do; can anyone hazard a guess?

It is the only place that we modify the inquiry result, and I thought it
was gross and (a bit) confusing.

That is, after your patch, it could change to (and the no_uld_attach check
should not really be an "else"):

--- inq-print-linux-2.6.17-rc4-git6/drivers/scsi/o-scsi_scan.c	2006-05-18 11:57:28.000000000 -0700
+++ inq-print-linux-2.6.17-rc4-git6/drivers/scsi/scsi_scan.c	2006-05-18 12:06:05.000000000 -0700
@@ -598,18 +598,18 @@ static int scsi_add_lun(struct scsi_devi
 	sdev->model = (char *) (sdev->inquiry + 16);
 	sdev->rev = (char *) (sdev->inquiry + 32);
 
-	if (*bflags & BLIST_ISROM) {
-		/*
-		 * It would be better to modify sdev->type, and set
-		 * sdev->removable; this can now be done since
-		 * print_inquiry has gone away.
-		 */
-		inq_result[0] = TYPE_ROM;
-		inq_result[1] |= 0x80;	/* removable */
-	} else if (*bflags & BLIST_NO_ULD_ATTACH)
+	if (*bflags & BLIST_NO_ULD_ATTACH)
 		sdev->no_uld_attach = 1;
 
-	switch (sdev->type = (inq_result[0] & 0x1f)) {
+	if (*bflags & BLIST_ISROM) {
+		sdev->type = TYPE_ROM;
+		sdev->removable = 1;
+	} else {
+		sdev->type = (inq_result[0] & 0x1f);
+		sdev->removable = (0x80 & inq_result[1]) >> 7;
+	}
+
+	switch (sdev->type) {
 	case TYPE_TAPE:
 	case TYPE_DISK:
 	case TYPE_PRINTER:
@@ -652,7 +652,6 @@ static int scsi_add_lun(struct scsi_devi
 	 */ 
 
 	sdev->inq_periph_qual = (inq_result[0] >> 5) & 7;
-	sdev->removable = (0x80 & inq_result[1]) >> 7;
 	sdev->lockable = sdev->removable;
 	sdev->soft_reset = (inq_result[7] & 1) && ((inq_result[3] & 7) == 2);
 
-- Patrick Mansfield

Re: [RFC] printks in print_inquiry

[Matthew Wilcox]

On Thu, May 18, 2006 at 01:09:57PM -0700, Patrick Mansfield wrote:
> > scsi: 2:0:1:0: Vendor: HP 18.2G Model: ATLAS10K3_18_SCA Rev: HP05 ANSI rev: 02  
> That is very nice ... as is replacing print_inquiry with one line of code.

Thanks.  However, I'm now wondering about the length of the line.
Aren't people agitating to replace the channel with a string?  which
could be longer than the, oh, three bytes left after the end of the
current string?  Losing the scsi: isn't really a good idea.  So how
about:

scsi: 2:0:1:0 Device: HP 18.2G ATLAS10K3_18_SCA HP05 ANSI 02
scsi: 4:0:2:0 Device: HP       DVD-ROM 305      1.01 ANSI 02

If we really wanted to be smart, we could even do:

scsi: 2:0:1:0 Direct-Access      HP 18.2G ATLAS10K3_18_SCA HP05 ANSI 02
scsi: 4:0:2:0 CD-ROM             HP       DVD-ROM 305      1.01 ANSI 02

I'm still in two minds about even reporting the ANSI version.  Is there
ever a time when having that information would be useful to debug a
problem *and* we don't have access to that (eg through sysfs)?

> > sda: 35566480 512-byte hdwr sectors (18210 MB)
> > sda: Write Protect is off
> > sda: drive cache: write through w/ FUA
> >  sda: sda1 sda2 sda3
> > sd 2:0:1:0: Attached scsi disk sda
> > sd 2:0:1:0: Attached scsi generic sg0 type 0
> 
> Yes, better. I guess those should all be sdev_printk in sd.c.

Not sure I agree, actually.  A lot of these things pertain much more to
the sda-ness of the device than they do to the 2:0:1:0'ness of the device.

> Funky how loading sd after sg changes the output ... and using the driver
> name as a prefix sometimes messes this up for scsi.
> 
> i.e. scan without sd_mod or sg loaded (and distro I'm using loads sg
> before sd_mod via udev rules):
> 
>  0:0:0:0: Attached scsi generic sg0 type 0
>  0:0:0:1: Attached scsi generic sg1 type 0
> 
> Then remove/add those devices, and sg lines become:
> 
> sd 1:0:0:0: Attached scsi generic sg0 type 0
> sd 1:0:0:1: Attached scsi generic sg1 type 0

I'm going to reply to this in a separate email, as I think we need
buy-in from Greg on this one.

> It is the only place that we modify the inquiry result, and I thought it
> was gross and (a bit) confusing.

I agree, nice patch, makes much more sense.

Re: [RFC] printks in print_inquiry

[James Smart]

Matthew Wilcox wrote:
> On Thu, May 18, 2006 at 01:09:57PM -0700, Patrick Mansfield wrote:
>>> scsi: 2:0:1:0: Vendor: HP 18.2G Model: ATLAS10K3_18_SCA Rev: HP05 ANSI rev: 02  
>> That is very nice ... as is replacing print_inquiry with one line of code.
> 
> Thanks.  However, I'm now wondering about the length of the line.
> Aren't people agitating to replace the channel with a string?  which
> could be longer than the, oh, three bytes left after the end of the
> current string?  Losing the scsi: isn't really a good idea.  So how
> about:
> 
> scsi: 2:0:1:0 Device: HP 18.2G ATLAS10K3_18_SCA HP05 ANSI 02
> scsi: 4:0:2:0 Device: HP       DVD-ROM 305      1.01 ANSI 02
> 
> If we really wanted to be smart, we could even do:
> 
> scsi: 2:0:1:0 Direct-Access      HP 18.2G ATLAS10K3_18_SCA HP05 ANSI 02
> scsi: 4:0:2:0 CD-ROM             HP       DVD-ROM 305      1.01 ANSI 02

I wouldn't bother with the device type, unless you are reporting it as the
byte0 contents (eg. PQ bits and dev type). I would like to see the PQ bits.
I also expect the class driver to typically bind to the device right after
this, so I'll get the sd or st lines which will further reflect device type.

> I'm still in two minds about even reporting the ANSI version.  Is there
> ever a time when having that information would be useful to debug a
> problem *and* we don't have access to that (eg through sysfs)?

It makes a difference if debugging a scan issue. I would think it's useful
to see what Lun 0 has as it drives other scanning.

-- james s

Re: [RFC] printks in print_inquiry

[Matthew Wilcox]

On Fri, May 19, 2006 at 03:43:26PM -0400, James Smart wrote:
> Matthew Wilcox wrote:
> >scsi: 2:0:1:0 Device: HP 18.2G ATLAS10K3_18_SCA HP05 ANSI 02
> >scsi: 4:0:2:0 Device: HP       DVD-ROM 305      1.01 ANSI 02
> >
> >If we really wanted to be smart, we could even do:
> >
> >scsi: 2:0:1:0 Direct-Access      HP 18.2G ATLAS10K3_18_SCA HP05 ANSI 02
> >scsi: 4:0:2:0 CD-ROM             HP       DVD-ROM 305      1.01 ANSI 02
> 
> I wouldn't bother with the device type, unless you are reporting it as the
> byte0 contents (eg. PQ bits and dev type). I would like to see the PQ bits.
> I also expect the class driver to typically bind to the device right after
> this, so I'll get the sd or st lines which will further reflect device type.

Ah, well, that's no longer true with the async scanning patches.  They
work by scanning all the luns first, then going back and adding the luns
to sysfs.  I did this to preserve drive naming.  There were some other
options I considered, but this was the simplest way of doing things.

Looking at the device type stuff, I feel the urge to change the
interface to it -- no longer export the array directly, but change
it to a function which returns a string.  That way we can pass in
inquiry byte 0 and have it return an appropriate string based on PQ and
PQT.

> >I'm still in two minds about even reporting the ANSI version.  Is there
> >ever a time when having that information would be useful to debug a
> >problem *and* we don't have access to that (eg through sysfs)?
> 
> It makes a difference if debugging a scan issue. I would think it's useful
> to see what Lun 0 has as it drives other scanning.

Certainly -- but don't we get sysfs access to debug this kind of problem?
Or do people sometimes have this problem and can't find their root
device as a consequence?

Re: [RFC] printks in print_inquiry

[James Bottomley]

On Sat, 2006-05-20 at 08:19 -0600, Matthew Wilcox wrote:
> Certainly -- but don't we get sysfs access to debug this kind of
> problem?
> Or do people sometimes have this problem and can't find their root
> device as a consequence?

send me you dmesg output is a far easier thing to ask non-linux experts
than send me the contents of the scsi_level sysfs parameter for the
relevant scsi_device (primarily because it's usually hard to work out
where in the tree this is).

So, until we have some kind of sysfs dump command that comes with every
distribution and we can tell someone to run and attach the output, I'd
really prefer to keep useful diagnostic information in dmesg.

James

dev_printk output

[Matthew Wilcox]

On Thu, May 18, 2006 at 01:09:57PM -0700, Patrick Mansfield wrote:
> Funky how loading sd after sg changes the output ... and using the driver
> name as a prefix sometimes messes this up for scsi.
> 
> i.e. scan without sd_mod or sg loaded (and distro I'm using loads sg
> before sd_mod via udev rules):
> 
>  0:0:0:0: Attached scsi generic sg0 type 0
>  0:0:0:1: Attached scsi generic sg1 type 0
> 
> Then remove/add those devices, and sg lines become:
> 
> sd 1:0:0:0: Attached scsi generic sg0 type 0
> sd 1:0:0:1: Attached scsi generic sg1 type 0

I find that a bit confusing too.  Obviously, we should distinguish
different kinds of bus_id from each other somehow -- but isn't the
obvious thing to use the bus name?  That must already be unique as sysfs
relies on it.  ie this patch:

(seems that dev->bus isn't always set; I got a null ptr dereference when
booting without that check).

Signed-off-by: Matthew Wilcox <matthew@wil.cx>

Index: include/linux/device.h
===================================================================
RCS file: /var/cvs/linux-2.6/include/linux/device.h,v
retrieving revision 1.25
diff -u -p -r1.25 device.h
--- include/linux/device.h	13 May 2006 04:12:30 -0000	1.25
+++ include/linux/device.h	19 May 2006 19:54:04 -0000
@@ -412,7 +412,7 @@ extern void firmware_unregister(struct s
 
 /* debugging and troubleshooting/diagnostic helpers. */
 #define dev_printk(level, dev, format, arg...)	\
-	printk(level "%s %s: " format , (dev)->driver ? (dev)->driver->name : "" , (dev)->bus_id , ## arg)
+	printk(level "%s %s: " format , (dev)->bus ? (dev)->bus->name : "", (dev)->bus_id , ## arg)
 
 #ifdef DEBUG
 #define dev_dbg(dev, format, arg...)		\

Re: dev_printk output

[Greg KH]

On Fri, May 19, 2006 at 02:11:42PM -0600, Matthew Wilcox wrote:
> On Thu, May 18, 2006 at 01:09:57PM -0700, Patrick Mansfield wrote:
> > Funky how loading sd after sg changes the output ... and using the driver
> > name as a prefix sometimes messes this up for scsi.
> > 
> > i.e. scan without sd_mod or sg loaded (and distro I'm using loads sg
> > before sd_mod via udev rules):
> > 
> >  0:0:0:0: Attached scsi generic sg0 type 0
> >  0:0:0:1: Attached scsi generic sg1 type 0
> > 
> > Then remove/add those devices, and sg lines become:
> > 
> > sd 1:0:0:0: Attached scsi generic sg0 type 0
> > sd 1:0:0:1: Attached scsi generic sg1 type 0
> 
> I find that a bit confusing too.  Obviously, we should distinguish
> different kinds of bus_id from each other somehow -- but isn't the
> obvious thing to use the bus name?  That must already be unique as sysfs
> relies on it.  ie this patch:
> 
> (seems that dev->bus isn't always set; I got a null ptr dereference when
> booting without that check).

Yes, not all devices are on a bus, so this will not work.  And we want
to know the driver that controls the device too.  So how about adding
the bus if it's not null?

Something like (untested):
	printk(level "%s %s %s: " format , (dev)->bus ? (dev)->bus->name : "", (dev)->driver ? (dev)->driver->name : "", (dev)->bus_id , ## arg)

thanks,

greg k-h

Re: dev_printk output

[Matthew Wilcox]

On Fri, May 19, 2006 at 01:28:47PM -0700, Greg KH wrote:
> On Fri, May 19, 2006 at 02:11:42PM -0600, Matthew Wilcox wrote:
> > On Thu, May 18, 2006 at 01:09:57PM -0700, Patrick Mansfield wrote:
> > > Funky how loading sd after sg changes the output ... and using the driver
> > > name as a prefix sometimes messes this up for scsi.
> > > 
> > >  0:0:0:0: Attached scsi generic sg0 type 0
> > > sd 1:0:0:0: Attached scsi generic sg0 type 0
> > 
> > I find that a bit confusing too.  Obviously, we should distinguish
> > different kinds of bus_id from each other somehow -- but isn't the
> > obvious thing to use the bus name?  That must already be unique as sysfs
> > relies on it.  ie this patch:
> 
> Yes, not all devices are on a bus, so this will not work.  And we want
> to know the driver that controls the device too.  So how about adding
> the bus if it's not null?
> 
> Something like (untested):
> 	printk(level "%s %s %s: " format , (dev)->bus ? (dev)->bus->name : "", (dev)->driver ? (dev)->driver->name : "", (dev)->bus_id , ## arg)

Then we still get the inconsistency of device names changing as drivers
are loaded.  I think we should declare it a bug for devices to not be
on a bus.  The only example I have of devices not-on-a-bus are scsi
targets.  I would propose introducing a new scsi_target bus for them,
then removing the 'target' from the start of the bus_id.  Adding them to
the scsi bus looks like it'd be a lot of work.

Re: dev_printk output

[James Bottomley]

On Fri, 2006-05-19 at 22:55 -0600, Matthew Wilcox wrote:
> Then we still get the inconsistency of device names changing as
> drivers
> are loaded.  I think we should declare it a bug for devices to not be
> on a bus.  The only example I have of devices not-on-a-bus are scsi
> targets.  I would propose introducing a new scsi_target bus for them,
> then removing the 'target' from the start of the bus_id.  Adding them
> to
> the scsi bus looks like it'd be a lot of work.

It's not just the target.  All the intermediate devices in transport
classes don't have busses either.   The only reason scsi_device has a
bus is so we can use the driver model to bind the ULDs.

James

Re: dev_printk output

[Greg KH]

On Fri, May 19, 2006 at 10:55:44PM -0600, Matthew Wilcox wrote:
> On Fri, May 19, 2006 at 01:28:47PM -0700, Greg KH wrote:
> > On Fri, May 19, 2006 at 02:11:42PM -0600, Matthew Wilcox wrote:
> > > On Thu, May 18, 2006 at 01:09:57PM -0700, Patrick Mansfield wrote:
> > > > Funky how loading sd after sg changes the output ... and using the driver
> > > > name as a prefix sometimes messes this up for scsi.
> > > > 
> > > >  0:0:0:0: Attached scsi generic sg0 type 0
> > > > sd 1:0:0:0: Attached scsi generic sg0 type 0
> > > 
> > > I find that a bit confusing too.  Obviously, we should distinguish
> > > different kinds of bus_id from each other somehow -- but isn't the
> > > obvious thing to use the bus name?  That must already be unique as sysfs
> > > relies on it.  ie this patch:
> > 
> > Yes, not all devices are on a bus, so this will not work.  And we want
> > to know the driver that controls the device too.  So how about adding
> > the bus if it's not null?
> > 
> > Something like (untested):
> > 	printk(level "%s %s %s: " format , (dev)->bus ? (dev)->bus->name : "", (dev)->driver ? (dev)->driver->name : "", (dev)->bus_id , ## arg)
> 
> Then we still get the inconsistency of device names changing as drivers
> are loaded.

The bus id doesn't change, just the "hint" as to who is controling it at
that point in time.  Which is something that is needed/wanted by a lot
of people.

> I think we should declare it a bug for devices to not be on a bus.

No!  We have a few places already that are devices with no related bus,
and are only getting more and more with the shift away from class_device
to device (see a patch in my quilt tree that allows this.)

thanks,

greg k-h

Re: dev_printk output

[Matthew Wilcox]

On Sat, May 20, 2006 at 02:21:35PM -0700, Greg KH wrote:
> On Fri, May 19, 2006 at 10:55:44PM -0600, Matthew Wilcox wrote:
> > On Fri, May 19, 2006 at 01:28:47PM -0700, Greg KH wrote:
> > > On Fri, May 19, 2006 at 02:11:42PM -0600, Matthew Wilcox wrote:
> > > > On Thu, May 18, 2006 at 01:09:57PM -0700, Patrick Mansfield wrote:
> > > > > Funky how loading sd after sg changes the output ... and using the driver
> > > > > name as a prefix sometimes messes this up for scsi.
> > > > > 
> > > > >  0:0:0:0: Attached scsi generic sg0 type 0
> > > > > sd 1:0:0:0: Attached scsi generic sg0 type 0
> > > 
> > > Something like (untested):
> > > 	printk(level "%s %s %s: " format , (dev)->bus ? (dev)->bus->name : "", (dev)->driver ? (dev)->driver->name : "", (dev)->bus_id , ## arg)
> > 
> > Then we still get the inconsistency of device names changing as drivers
> > are loaded.
> 
> The bus id doesn't change, just the "hint" as to who is controling it at
> that point in time.  Which is something that is needed/wanted by a lot
> of people.

Sure, but it's also unwanted by others ;-)  There's no reason we have to
have a 'one size fits all' solution.  Let's add a dev_bus_printk() (or
maybe just bus_printk()?) that does
	printk(level "%s %s: " format , (dev)->bus ? (dev)->bus->name : "", \
		(dev)->bus_id , ## arg)

or we could rename the current dev_printk() implementation to drv_printk()
and have dev_printk() do the above.  or we could do something like:

#define DEV_DRV(dev)	(dev)->driver ? (dev)->driver->name : ""
#define DEV_BUS(dev)	(dev)->bus ? (dev)->bus->name : ""
#define dev_printk(level, dev, format, arg...)  \
	printk(level "%s %s: " format, DEV_BUS(dev), (dev)->bus_id , ## arg)
#define drv_printk(level, dev, format, arg...)  \
	printk(level "%s %s %s: " format, DEV_DRV(dev), DEV_BUS(dev), \
		(dev)->bus_id , ## arg)

Obviously, we could argue about the exact naming until the cows come home, 
so let me say that I don't really care.  I just want something that
looks like "scsi 0:0:0:0" rather than "sd 0:0:0:0" or " 0:0:0:0",
depending on whether sd has been loaded or not.

If you're still reluctant, think about it this way.  There are bits of
the kernel which need to tell the user about a device that may, or may
not have a driver attached.  The scsi midlayer is one, and I'm sure
there are others.

Re: dev_printk output

[James Bottomley]

On Sun, 2006-05-28 at 21:57 -0600, Matthew Wilcox wrote:
> Obviously, we could argue about the exact naming until the cows come
> home, 
> so let me say that I don't really care.  I just want something that
> looks like "scsi 0:0:0:0" rather than "sd 0:0:0:0" or " 0:0:0:0",
> depending on whether sd has been loaded or not.

If we follow the recommendations of the storage summit, we'll have to
trash the scsi bus type to get ata an upper level driver, so even this
will eventually no longer print what you want ...

James