[PATCH 1/2] sdev_printk - scsi_device helper macro

[PATCH 1/2] sdev_printk - scsi_device helper macro

[Daniel Stekloff]

I originally posted this patch for comments:

http://marc.theaimsgroup.com/?l=linux-scsi&m=106487047707538&w=2

The sdev_printk macro is meant to provide two things for the mid-layer and 
LLDs:

1) Add consistency to printing out scsi_device information.

2) Identifying printk with a specific scsi_device.

I've updated it to the latest scsi tree and I've split it into two parts. The 
first patch adds the macro to scsi_device.h and moves setting scsi_device's 
sdev_gendev's bus_id from scsi_sysfs.c to scsi_scan.c. The second patch, 
which I'll send after this message, adds using the macro to scsi mid-layer. 

I split them up in case there might be some discussion on how I applied the 
macro, whether I've missed places or put it in places where it shouldn't be.

Please apply, or let me know if you have any objections.

Thanks,

Dan

Patch #1 Follows:
-------------------


diff -urN --exclude=SCCS scsi-misc-2.5/include/scsi/scsi_device.h 
scsi-misc-2.5-sdev/include/scsi/scsi_device.h
--- scsi-misc-2.5/include/scsi/scsi_device.h	2003-10-06 16:13:57.000000000 
-0700
+++ scsi-misc-2.5-sdev/include/scsi/scsi_device.h	2003-10-06 
20:04:19.671217176 -0700
@@ -105,6 +105,9 @@
 #define	class_to_sdev(d)	\
 	container_of(d, struct scsi_device, sdev_classdev)
 
+#define sdev_printk(level, sdev, format, arg...) \
+	printk(level "scsi <%s>: " format , (sdev)->sdev_gendev.bus_id , ## arg)
+
 extern struct scsi_device *scsi_add_device(struct Scsi_Host *,
 		uint, uint, uint);
 extern void scsi_remove_device(struct scsi_device *);
diff -urN --exclude=SCCS scsi-misc-2.5/drivers/scsi/scsi_scan.c 
scsi-misc-2.5-sdev/drivers/scsi/scsi_scan.c
--- scsi-misc-2.5/drivers/scsi/scsi_scan.c	2003-10-06 16:13:29.000000000 -0700
+++ scsi-misc-2.5-sdev/drivers/scsi/scsi_scan.c	2003-10-06 20:08:42.428272032 
-0700
@@ -258,6 +258,9 @@
 	if (!sdev->scsi_level)
 		sdev->scsi_level = SCSI_2;
 
+	sprintf(sdev->sdev_gendev.bus_id,"%d:%d:%d:%d",
+		sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
+
 	/*
 	 * Add it to the end of the shost->my_devices list.
 	 */
diff -urN --exclude=SCCS scsi-misc-2.5/drivers/scsi/scsi_sysfs.c 
scsi-misc-2.5-sdev/drivers/scsi/scsi_sysfs.c
--- scsi-misc-2.5/drivers/scsi/scsi_sysfs.c	2003-10-06 16:13:29.000000000 
-0700
+++ scsi-misc-2.5-sdev/drivers/scsi/scsi_sysfs.c	2003-10-06 20:09:14.553388272 
-0700
@@ -340,8 +340,6 @@
 
 	set_bit(SDEV_ADD, &sdev->sdev_state);
 	device_initialize(&sdev->sdev_gendev);
-	sprintf(sdev->sdev_gendev.bus_id,"%d:%d:%d:%d",
-		sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
 	sdev->sdev_gendev.parent = &sdev->host->shost_gendev;
 	sdev->sdev_gendev.bus = &scsi_bus_type;
 	sdev->sdev_gendev.release = scsi_device_dev_release;

Re: [PATCH 1/2] sdev_printk - scsi_device helper macro

[Hironobu Ishii]

Hi Dan,

>
> I originally posted this patch for comments:
>
> http://marc.theaimsgroup.com/?l=linux-scsi&m=106487047707538&w=2
>
> The sdev_printk macro is meant to provide two things for the mid-layer and
> LLDs:
>
> 1) Add consistency to printing out scsi_device information.
>
> 2) Identifying printk with a specific scsi_device.
>

I agree these.


> diff -urN --exclude=SCCS scsi-misc-2.5/include/scsi/scsi_device.h
> scsi-misc-2.5-sdev/include/scsi/scsi_device.h
> --- scsi-misc-2.5/include/scsi/scsi_device.h 2003-10-06 16:13:57.000000000
> -0700
> +++ scsi-misc-2.5-sdev/include/scsi/scsi_device.h 2003-10-06
> 20:04:19.671217176 -0700
> @@ -105,6 +105,9 @@
>  #define class_to_sdev(d) \
>   container_of(d, struct scsi_device, sdev_classdev)
>
> +#define sdev_printk(level, sdev, format, arg...) \
> + printk(level "scsi <%s>: " format , (sdev)->sdev_gendev.bus_id , ## arg)
> +

It would be nice if the message format is the same with the dev_printk().

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


And, there are many colon separated bus_id, I prefer adding "scsi" before that.
Also, bus_id is separated with ":", so your idea that it is quoted by "< >" is
nice.
I propose the folloing format.

#define sdev_printk(level, sdev, format, arg...) \
  printk(level "%s <scsi%s>: " format , (sdev)->sdev_gendev.driver->name, \
                (sdev)->sdev_gendev.bus_id , ## arg)

It would produce a message like this:
    st <scsi0:0:4:0>: Device offlined - not ready after error recovery

Thanks,
Hironobu Ishii

Re: [PATCH 1/2] sdev_printk - scsi_device helper macro

[Daniel Stekloff]

Hi,


On Wednesday 08 October 2003 07:03 pm, Hironobu Ishii wrote:
> Hi Dan,
>
> > I originally posted this patch for comments:
> >
> > http://marc.theaimsgroup.com/?l=linux-scsi&m=106487047707538&w=2
> >
> > The sdev_printk macro is meant to provide two things for the mid-layer
> > and LLDs:
> >
> > 1) Add consistency to printing out scsi_device information.
> >
> > 2) Identifying printk with a specific scsi_device.
>
> I agree these.
>
> > diff -urN --exclude=SCCS scsi-misc-2.5/include/scsi/scsi_device.h
> > scsi-misc-2.5-sdev/include/scsi/scsi_device.h
> > --- scsi-misc-2.5/include/scsi/scsi_device.h 2003-10-06
> > 16:13:57.000000000 -0700
> > +++ scsi-misc-2.5-sdev/include/scsi/scsi_device.h 2003-10-06
> > 20:04:19.671217176 -0700
> > @@ -105,6 +105,9 @@
> >  #define class_to_sdev(d) \
> >   container_of(d, struct scsi_device, sdev_classdev)
> >
> > +#define sdev_printk(level, sdev, format, arg...) \
> > + printk(level "scsi <%s>: " format , (sdev)->sdev_gendev.bus_id , ##
> > arg) +
>
> It would be nice if the message format is the same with the dev_printk().
>
>     #define dev_printk(level, dev, format, arg...)  \
>         printk(level "%s %s: " format , (dev)->driver->name , (dev)->bus_id
> , ##
>
>
> And, there are many colon separated bus_id, I prefer adding "scsi" before
> that. Also, bus_id is separated with ":", so your idea that it is quoted by
> "< >" is nice.
> I propose the folloing format.
>
> #define sdev_printk(level, sdev, format, arg...) \
>   printk(level "%s <scsi%s>: " format , (sdev)->sdev_gendev.driver->name, \
>                 (sdev)->sdev_gendev.bus_id , ## arg)
>
> It would produce a message like this:
>     st <scsi0:0:4:0>: Device offlined - not ready after error recovery



I am open to other formats, but the problem with doing something like 
dev_printk() is the driver isn't always there, especially during scan. Or, am 
I missing something? I decided not to put the driver in because the 
sdev_printk could be used more throughout the mid-layer and especially in 
scan when the driver isn't set yet. Please let me know if I'm missing 
something. 

Thanks,

Dan


> Thanks,
> Hironobu Ishii

Re: [PATCH 1/2] sdev_printk - scsi_device helper macro

[Patrick Mansfield]

On Thu, Oct 09, 2003 at 10:24:58AM -0700, Daniel Stekloff wrote:
> On Wednesday 08 October 2003 07:03 pm, Hironobu Ishii wrote:

> > And, there are many colon separated bus_id, I prefer adding "scsi" before
> > that. Also, bus_id is separated with ":", so your idea that it is quoted by
> > "< >" is nice.
> > I propose the folloing format.
> >
> > #define sdev_printk(level, sdev, format, arg...) \
> >   printk(level "%s <scsi%s>: " format , (sdev)->sdev_gendev.driver->name, \
> >                 (sdev)->sdev_gendev.bus_id , ## arg)
> >
> > It would produce a message like this:
> >     st <scsi0:0:4:0>: Device offlined - not ready after error recovery

> I am open to other formats, but the problem with doing something like 
> dev_printk() is the driver isn't always there, especially during scan. Or, am 
> I missing something? I decided not to put the driver in because the 
> sdev_printk could be used more throughout the mid-layer and especially in 
> scan when the driver isn't set yet. Please let me know if I'm missing 
> something. 

Right, at scan time we won't have a sdev_gendev.driver.

And, dev_printk won't ever print sg as a driver, since sg is implemented as
a class, so errors when using sg would print st, sd, sr, or maybe crash ;-)

If we have an sdev_printk, it can be modified later to display however you
want.

-- Patrick Mansfield

Re: [PATCH 1/2] sdev_printk - scsi_device helper macro

[James Bottomley]

On Tue, 2003-10-07 at 16:18, Daniel Stekloff wrote:
> +#define sdev_printk(level, sdev, format, arg...) \
> +	printk(level "scsi <%s>: " format , (sdev)->sdev_gendev.bus_id , ## arg)
> +

OK, after six months, I've reconsidered on this one.

Make it #define to dev_printk() and it can go in.

James

Re: [PATCH 1/2] sdev_printk - scsi_device helper macro

[Daniel Stekloff]

On Wednesday 31 March 2004 10:35 am, James Bottomley wrote:
> On Tue, 2003-10-07 at 16:18, Daniel Stekloff wrote:
> > +#define sdev_printk(level, sdev, format, arg...) \
> > +	printk(level "scsi <%s>: " format , (sdev)->sdev_gendev.bus_id , ##
> > arg) +
>
> OK, after six months, I've reconsidered on this one.
>
> Make it #define to dev_printk() and it can go in.


Hi James,

One of the reasons why I didn't make this a #define to dev_printk() in the 
first place was because dev_printk() references "(dev)->driver->name", which 
isn't always valid when sdev_printk() is used. Please see scsi_scan.c as an 
example. The dev_printk() macro has that limitation. 

How would you like to proceed? 

1) Make the #define to dev_printk and only use sdev_printk in situations where 
struct device_driver is valid? This will reduce the number of places it can 
be used.

2) Keep sdev_printk() as is.

3) Create a check in the macro to see if dev->driver is valid.

4) Don't use sdev_printk().

I've noticed a number of patches lately that print out SCSI device information 
and believe this macro is useful for cleaning things up a bit and providing a 
standard format for sdev information. 

Sorry this isn't a slam dunk.

Thanks,

Dan

Re: [PATCH 1/2] sdev_printk - scsi_device helper macro

[James Bottomley]

On Wed, 2004-03-31 at 14:23, Daniel Stekloff wrote:
> One of the reasons why I didn't make this a #define to dev_printk() in the 
> first place was because dev_printk() references "(dev)->driver->name", which 
> isn't always valid when sdev_printk() is used. Please see scsi_scan.c as an 
> example. The dev_printk() macro has that limitation. 
> 
> How would you like to proceed? 
> 
> 1) Make the #define to dev_printk and only use sdev_printk in situations where 
> struct device_driver is valid? This will reduce the number of places it can 
> be used.

I think this is my preferred option.  That only leaves our scanning and
inquiry code as the exception.  It still sweeps up most of the driver
stuff.

It might be possible to improve this situation by initialising the
device earlier too.

> 2) Keep sdev_printk() as is.
> 
> 3) Create a check in the macro to see if dev->driver is valid.
> 
> 4) Don't use sdev_printk().
> 
> I've noticed a number of patches lately that print out SCSI device information 
> and believe this macro is useful for cleaning things up a bit and providing a 
> standard format for sdev information. 

Yes, that's why I dusted this off again.

James

Re: [PATCH 1/2] sdev_printk - scsi_device helper macro

[Daniel Stekloff]

On Wednesday 31 March 2004 10:35 am, James Bottomley wrote:
> On Tue, 2003-10-07 at 16:18, Daniel Stekloff wrote:
> > +#define sdev_printk(level, sdev, format, arg...) \
> > +	printk(level "scsi <%s>: " format , (sdev)->sdev_gendev.bus_id , ##
> > arg) +
>
> OK, after six months, I've reconsidered on this one.
>
> Make it #define to dev_printk() and it can go in.
>
> James


Hi James,

I really don't think it's worth putting in sdev_printk() if it points to 
dev_printk(). As you have seen recently:

http://marc.theaimsgroup.com/?l=linux-scsi&m=108189253319586&w=2

dev_printk() is only useful when a driver is bound to the specific device. 
This would make sdev_printk() useless really except for upper level drivers. 
It can't safely be used in the mid-layer. If you're interested in cleaning up 
some of the printks in the midlayer and identifying messages with consistant 
scsi information, I would suggest using the sdev_printk() as I had submitted 
it. 

Thanks,

Dan