Improve shost_printk

PATCH In review Matthew Wilcox 2 participants 2 messages Oct 2, 2007 SCSI

lore

Share & tools

lore

Matthew Wilcox

author

2007-10-02 15:45 UTC #0 lore raw

PATCH 1/1 Improve shost_printk

Using dev_printk to implement shost_printk leads to somewhat lacklustre results:

host2: SCSI BUS has been reset
scsi2 : sym-2.2.3

By reimplementing shost_printk directly on top of printk, we can get the more pleasing:

scsi 0: SCSI BUS has been reset
scsi 0: sym-2.2.3

It fits in well with

scsi 0:0:1:0: Optical Device MATSHITA PD-1 LF-1000 A105 PQ: 0 ANSI: 2

I'm less sure about the part of this patch which applies to starget_printk. It would be quite awkward to find the scsi_host, print its number, then print the channel and id of the starget. The compromise in this patch is to print:

scsi target0:0:1: Beginning Domain Validation

which at least doesn't penalise the caller of starget_printk. Maybe we could stash a pointer to the scsi_host directly in scsi_target, or just the scsi_host's number.

include/scsi/scsi_host.h +1 -1

index 5b79697..2268aa8 100644
@@ -675,7 +675,7 @@ struct Scsi_Host {
 	container_of(d, struct Scsi_Host, shost_classdev)
 
 #define shost_printk(prefix, shost, fmt, a...)	\
-	dev_printk(prefix, &(shost)->shost_gendev, fmt, ##a)
+	printk(prefix "scsi %d: " fmt, (shost)->host_no , ##a)
 
 static inline void *shost_priv(struct Scsi_Host *shost)
 {

include/scsi/scsi_device.h +1 -1

index d5057bc..4437b60 100644
@@ -202,7 +202,7 @@ static inline struct scsi_target *scsi_target(struct scsi_device *sdev)
 	to_scsi_target(class_dev->dev)
 
 #define starget_printk(prefix, starget, fmt, a...)	\
-	dev_printk(prefix, &(starget)->dev, fmt, ##a)
+	printk(prefix "scsi %s: " fmt, (starget)->dev.bus_id , ##a)
 
 extern struct scsi_device *__scsi_add_device(struct Scsi_Host *,
 		uint, uint, uint, void *hostdata);

drivers/scsi/hosts.c +1 -1

index adc9559..173de6d 100644
@@ -194,7 +194,7 @@ int scsi_add_host(struct Scsi_Host *shost, struct device *dev)
 	struct scsi_host_template *sht = shost->hostt;
 	int error = -EINVAL;
 
-	printk(KERN_INFO "scsi%d : %s\n", shost->host_no,
+	shost_printk(KERN_INFO, shost, "%s\n", 
 			sht->info ? sht->info(shost) : sht->name);
 
 	if (!shost->can_queue) {

Signature · 4 lines

Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

James Bottomley

2007-10-02 19:16 UTC #1 lore raw

Quoted 45 lines · Matthew Wilcox

Using dev_printk to implement shost_printk leads to somewhat lacklustre results:
host2: SCSI BUS has been reset
scsi2 : sym-2.2.3
By reimplementing shost_printk directly on top of printk, we can get the more pleasing:
scsi 0: SCSI BUS has been reset
scsi 0: sym-2.2.3
It fits in well with
scsi 0:0:1:0: Optical Device MATSHITA PD-1 LF-1000 A105 PQ: 0 ANSI: 2
I'm less sure about the part of this patch which applies to starget_printk. It would be quite awkward to find the scsi_host, print its number, then print the channel and id of the starget. The compromise in this patch is to print:
scsi target0:0:1: Beginning Domain Validation
which at least doesn't penalise the caller of starget_printk. Maybe we could stash a pointer to the scsi_host directly in scsi_target, or just the scsi_host's number.
include/scsi/scsi_host.h +1 -1
index 5b79697..2268aa8 100644
@@ -675,7 +675,7 @@ struct Scsi_Host {
 	container_of(d, struct Scsi_Host, shost_classdev)
 
 #define shost_printk(prefix, shost, fmt, a...)	\
-	dev_printk(prefix, &(shost)->shost_gendev, fmt, ##a)
+	printk(prefix "scsi %d: " fmt, (shost)->host_no , ##a)
 
 static inline void *shost_priv(struct Scsi_Host *shost)
 {
include/scsi/scsi_device.h +0 -0
index d5057bc..4437b60 100644

include/scsi/scsi_device.h static inline struct scsi_target *scsi_target(struct scsi_device *sdev) · line 205

Quoted 5 lines

include/scsi/scsi_device.h +1 -1

@@ -202,7 +202,7 @@ static inline struct scsi_target *scsi_target(struct scsi_device *sdev)
 	to_scsi_target(class_dev->dev)
 
 #define starget_printk(prefix, starget, fmt, a...)	\
-	dev_printk(prefix, &(starget)->dev, fmt, ##a)
+	printk(prefix "scsi %s: " fmt, (starget)->dev.bus_id , ##a)

This is the bit I don't like. The idea is to consolidate all printing through dev_printk so we can use it as a tap into the non existent kernel logging infrastructure when someone gullible^Wenthusiastic enough comes along to do it.

Quoted 15 lines

include/scsi/scsi_device.h +0 -0

 extern struct scsi_device *__scsi_add_device(struct Scsi_Host *,
 		uint, uint, uint, void *hostdata);

drivers/scsi/hosts.c +1 -1

index adc9559..173de6d 100644
@@ -194,7 +194,7 @@ int scsi_add_host(struct Scsi_Host *shost, struct device *dev)
 	struct scsi_host_template *sht = shost->hostt;
 	int error = -EINVAL;
 
-	printk(KERN_INFO "scsi%d : %s\n", shost->host_no,
+	shost_printk(KERN_INFO, shost, "%s\n", 
 			sht->info ? sht->info(shost) : sht->name);
 
 	if (!shost->can_queue) {