[PATCH] Improve shost_printk

[PATCH] Improve shost_printk

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

diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 5b79697..2268aa8 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -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)
 {
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index d5057bc..4437b60 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -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);
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index adc9559..173de6d 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -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) {
-- 
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."

Re: [PATCH] Improve shost_printk

[James Bottomley]

On Tue, 2007-10-02 at 09:45 -0600, Matthew Wilcox wrote:
> 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.
> 
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 5b79697..2268aa8 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -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)
>  {
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index d5057bc..4437b60 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -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.
 
>  extern struct scsi_device *__scsi_add_device(struct Scsi_Host *,
>  		uint, uint, uint, void *hostdata);
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index adc9559..173de6d 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -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) {