[PATCH] libata-core.c: restore configuration boot messages in ata_dev_configure()

[PATCH] libata-core.c: restore configuration boot messages in ata_dev_configure()

[Borislav Petkov]

Hi Tejun, Jeff,
   is this one like what you had in mind? I left out the ENTER/EXIT printk's
   since they are debug level. By the way, should I convert the dbg message
   levels to the scheme you both proposed now while not everything is converted
   or wait first. However, i'd rather do it now since it is less work than
   later :)?



This restores the default libata configuration messages printed during booting.

Signed-off-by: <petkov@math.uni-muenster.de>


--- libata-dev/drivers/scsi/libata-core.c.orig2	2006-06-27 09:21:44.000000000 +0200
+++ libata-dev/drivers/scsi/libata-core.c	2006-06-27 09:23:54.000000000 +0200
@@ -1359,11 +1359,10 @@ int ata_dev_configure(struct ata_device 
 			       __FUNCTION__, ap->id, dev->devno);
 
 	/* print device capabilities */
-	if (ata_msg_probe(ap))
-		ata_dev_printk(dev, KERN_DEBUG,
-			       "%s: cfg 49:%04x 82:%04x 83:%04x 84:%04x "
+	if (ata_msg_drv(ap))
+		ata_dev_printk(dev, KERN_INFO,
+			       "cfg 49:%04x 82:%04x 83:%04x 84:%04x "
 			       "85:%04x 86:%04x 87:%04x 88:%04x\n",
-			       __FUNCTION__,
 			       id[49], id[82], id[83], id[84],
 			       id[85], id[86], id[87], id[88]);
 
@@ -1405,7 +1404,7 @@ int ata_dev_configure(struct ata_device 
 			ata_dev_config_ncq(dev, ncq_desc, sizeof(ncq_desc));
 
 			/* print device info to dmesg */
-			if (ata_msg_info(ap))
+			if (ata_msg_drv(ap))
 				ata_dev_printk(dev, KERN_INFO, "ATA-%d, "
 					"max %s, %Lu sectors: %s %s\n",
 					ata_id_major_version(id),
@@ -1428,7 +1427,7 @@ int ata_dev_configure(struct ata_device 
 			}
 
 			/* print device info to dmesg */
-			if (ata_msg_info(ap))
+			if (ata_msg_drv(ap))
 				ata_dev_printk(dev, KERN_INFO, "ATA-%d, "
 					"max %s, %Lu sectors: CHS %u/%u/%u\n",
 					ata_id_major_version(id),
@@ -1440,7 +1439,7 @@ int ata_dev_configure(struct ata_device 
 
 		if (dev->id[59] & 0x100) {
 			dev->multi_count = dev->id[59] & 0xff;
-			if (ata_msg_info(ap))
+			if (ata_msg_drv(ap))
 				ata_dev_printk(dev, KERN_INFO,
 					"ata%u: dev %u multi count %u\n",
 					ap->id, dev->devno, dev->multi_count);
@@ -1469,7 +1468,7 @@ int ata_dev_configure(struct ata_device 
 		}
 
 		/* print device info to dmesg */
-		if (ata_msg_info(ap))
+		if (ata_msg_drv(ap))
 			ata_dev_printk(dev, KERN_INFO, "ATAPI, max %s%s\n",
 				       ata_mode_string(xfer_mask),
 				       cdb_intr_string);
@@ -1483,7 +1482,7 @@ int ata_dev_configure(struct ata_device 
 
 	/* limit bridge transfers to udma5, 200 sectors */
 	if (ata_dev_knobble(dev)) {
-		if (ata_msg_info(ap))
+		if (ata_msg_drv(ap))
 			ata_dev_printk(dev, KERN_INFO,
 				       "applying bridge limits\n");
 		dev->udma_mask &= ATA_UDMA5;

		
___________________________________________________________ 
Telefonate ohne weitere Kosten vom PC zum PC: http://messenger.yahoo.de

Re: [PATCH] libata-core.c: restore configuration boot messages in ata_dev_configure()

[Tejun Heo]

Borislav Petkov wrote:
> Hi Tejun, Jeff,
>    is this one like what you had in mind? I left out the ENTER/EXIT printk's
>    since they are debug level. By the way, should I convert the dbg message
>    levels to the scheme you both proposed now while not everything is converted
>    or wait first. However, i'd rather do it now since it is less work than
>    later :)?

IMHO, this should be done in the following steps.

* define ATA_MSG_* and ata_msg_* macros which map 1:1 to the current 
message levels.

* make ata_*_prink()s use ATA_MSG_* instead of KERN_* and embed 
ata_msg_enable() into ata_*_printk()s.  Convert ALL ata_*_printk()s and 
convertible DEBUG/VDEBUG()s in single sweep - it doesn't have to be a 
single patch but post them together.  These conversions touch a lot of 
places and other patches have to be regenerated afterward.

* define more ATA_MSG_* and convert.

> This restores the default libata configuration messages printed during booting.
> 
> Signed-off-by: <petkov@math.uni-muenster.de>
> 
> 
> --- libata-dev/drivers/scsi/libata-core.c.orig2	2006-06-27 09:21:44.000000000 +0200
> +++ libata-dev/drivers/scsi/libata-core.c	2006-06-27 09:23:54.000000000 +0200
> @@ -1359,11 +1359,10 @@ int ata_dev_configure(struct ata_device 
>  			       __FUNCTION__, ap->id, dev->devno);
>  
>  	/* print device capabilities */
> -	if (ata_msg_probe(ap))
> -		ata_dev_printk(dev, KERN_DEBUG,
> -			       "%s: cfg 49:%04x 82:%04x 83:%04x 84:%04x "
> +	if (ata_msg_drv(ap))
> +		ata_dev_printk(dev, KERN_INFO,
> +			       "cfg 49:%04x 82:%04x 83:%04x 84:%04x "
>  			       "85:%04x 86:%04x 87:%04x 88:%04x\n",
> -			       __FUNCTION__,
>  			       id[49], id[82], id[83], id[84],
>  			       id[85], id[86], id[87], id[88]);

This is a debug message and shouldn't be printed by default.

> @@ -1405,7 +1404,7 @@ int ata_dev_configure(struct ata_device 
>  			ata_dev_config_ncq(dev, ncq_desc, sizeof(ncq_desc));
>  
>  			/* print device info to dmesg */
> -			if (ata_msg_info(ap))
> +			if (ata_msg_drv(ap))
>  				ata_dev_printk(dev, KERN_INFO, "ATA-%d, "
>  					"max %s, %Lu sectors: %s %s\n",
>  					ata_id_major_version(id),

This should be printed by default but only when @print_info is non-zero.

> @@ -1428,7 +1427,7 @@ int ata_dev_configure(struct ata_device 
>  			}
>  
>  			/* print device info to dmesg */
> -			if (ata_msg_info(ap))
> +			if (ata_msg_drv(ap))
>  				ata_dev_printk(dev, KERN_INFO, "ATA-%d, "
>  					"max %s, %Lu sectors: CHS %u/%u/%u\n",
>  					ata_id_major_version(id),

Ditto.

> @@ -1440,7 +1439,7 @@ int ata_dev_configure(struct ata_device 
>  
>  		if (dev->id[59] & 0x100) {
>  			dev->multi_count = dev->id[59] & 0xff;
> -			if (ata_msg_info(ap))
> +			if (ata_msg_drv(ap))
>  				ata_dev_printk(dev, KERN_INFO,
>  					"ata%u: dev %u multi count %u\n",
>  					ap->id, dev->devno, dev->multi_count);

Ditto.

> @@ -1469,7 +1468,7 @@ int ata_dev_configure(struct ata_device 
>  		}
>  
>  		/* print device info to dmesg */
> -		if (ata_msg_info(ap))
> +		if (ata_msg_drv(ap))
>  			ata_dev_printk(dev, KERN_INFO, "ATAPI, max %s%s\n",
>  				       ata_mode_string(xfer_mask),
>  				       cdb_intr_string);

Ditto.

> @@ -1483,7 +1482,7 @@ int ata_dev_configure(struct ata_device 
>  
>  	/* limit bridge transfers to udma5, 200 sectors */
>  	if (ata_dev_knobble(dev)) {
> -		if (ata_msg_info(ap))
> +		if (ata_msg_drv(ap))
>  			ata_dev_printk(dev, KERN_INFO,
>  				       "applying bridge limits\n");
>  		dev->udma_mask &= ATA_UDMA5;

Ditto.

And, now we're using ata_msg_drv() which is reserved for driver loading 
messages for info messages.  See how the scheme is broken?  :-p

Thanks.

-- 
tejun

Re: [PATCH] libata-core.c: restore configuration boot messages in ata_dev_configure()

[Borislav Petkov]

On Tue, Jun 27, 2006 at 09:25:26PM +0900, Tejun Heo wrote:

Hi, 
> IMHO, this should be done in the following steps.
> 
> * define ATA_MSG_* and ata_msg_* macros which map 1:1 to the current 
> message levels.

How about we pin down these final dbg levels to the following (both proposals 
merged):

ATA_MSG_ERR
ATA_MSG_WARNING
ATA_MSG_DRV ("standard" driver info, initial cfg messages)

ATA_MSG_INFO maybe) /* revalidation messages, EH progress, more verbose msgs,
                       feats */
ATA_MSG_VDEBUG  /* verbose hot path */, by the way, which are those hotpaths?
ATA_MSG_CMD     /* issue / completion */
ATA_MSG_SG      /* SG map/unmap handling */
ATA_MGS_TRACE   /* function enter/exit */

> * make ata_*_prink()s use ATA_MSG_* instead of KERN_* and embed 
> ata_msg_enable() into ata_*_printk()s.  Convert ALL ata_*_printk()s and 
> convertible DEBUG/VDEBUG()s in single sweep - it doesn't have to be a 
> single patch but post them together.  These conversions touch a lot of 
> places and other patches have to be regenerated afterward.

then do something like

#define ata_(ap|dev)_printk((ap|dev), lv, fmt, args...) \
    if (ata_msg_err(ap)) \
        printk(KERN_ERR"ata%u: "fmt, ...); \
    else if (ata_msg_warn(ap)) \
        printk(KERN_WARNING"ata%u: "fmt, ...); \
        .
        .
        .

and then call them like so:

ata_dev_printk(dev, ATA_MSG_ERR, "Error!%d", i);

and so on. This looks pretty compact to me, no?

Regards,
    Boris.

	

	
		
___________________________________________________________ 
Gesendet von Yahoo! Mail - Jetzt mit 1GB Speicher kostenlos - Hier anmelden: http://mail.yahoo.de

Re: [PATCH] libata-core.c: restore configuration boot messages in ata_dev_configure()

[Tejun Heo]

Borislav Petkov wrote:
> On Tue, Jun 27, 2006 at 09:25:26PM +0900, Tejun Heo wrote:
> 
> Hi, 
>> IMHO, this should be done in the following steps.
>>
>> * define ATA_MSG_* and ata_msg_* macros which map 1:1 to the current 
>> message levels.
> 
> How about we pin down these final dbg levels to the following (both proposals 
> merged):
> 
> ATA_MSG_ERR
> ATA_MSG_WARNING
> ATA_MSG_DRV ("standard" driver info, initial cfg messages)
> 
> ATA_MSG_INFO maybe) /* revalidation messages, EH progress, more verbose msgs,
>                        feats */
> ATA_MSG_VDEBUG  /* verbose hot path */, by the way, which are those hotpaths?

I don't know but left VDEBUG()s after CMD/SG/TRACE conversion should be 
a good start.

> ATA_MSG_CMD     /* issue / completion */
> ATA_MSG_SG      /* SG map/unmap handling */
> ATA_MGS_TRACE   /* function enter/exit */

Looks okay to me.

>> * make ata_*_prink()s use ATA_MSG_* instead of KERN_* and embed 
>> ata_msg_enable() into ata_*_printk()s.  Convert ALL ata_*_printk()s and 
>> convertible DEBUG/VDEBUG()s in single sweep - it doesn't have to be a 
>> single patch but post them together.  These conversions touch a lot of 
>> places and other patches have to be regenerated afterward.
> 
> then do something like
> 
> #define ata_(ap|dev)_printk((ap|dev), lv, fmt, args...) \
>     if (ata_msg_err(ap)) \
>         printk(KERN_ERR"ata%u: "fmt, ...); \
>     else if (ata_msg_warn(ap)) \
>         printk(KERN_WARNING"ata%u: "fmt, ...); \
>         .

Actually, how about...

enum {
	ATA_MSG_ERR,
	ATA_MSG_WARNING,
	...
};

const char *__ata_msg_lvs[] = {
	[ATA_MSG_ERR]		= KERN_ERR,
	[ATA_MSG_WARNING]	= KERN_WARNING,
	...
};

#define ata_port_printk(ap, lv, fmt, args...) do { \
	if (unlikely((ap)->msg_enable & (1 << (lv))))
		printk(__ata_msg_lvs[lv]"ata%u: "fmt, (ap)->id , #args);
} while (0)

Note that regardless of message level, the msg_enable is unlikely to hit 
in hot patch, so the 'unlikely()' hint.

> and then call them like so:
> 
> ata_dev_printk(dev, ATA_MSG_ERR, "Error!%d", i);
> 
> and so on. This looks pretty compact to me, no?

Yeap, the call looks good.

Thanks.

-- 
tejun

Re: [PATCH] libata-core.c: restore configuration boot messages in ata_dev_configure()

[Tejun Heo]

Tejun Heo wrote:
> Actually, how about...
> 
> enum {
>     ATA_MSG_ERR,
>     ATA_MSG_WARNING,
>     ...
> };
> 
> const char *__ata_msg_lvs[] = {
>     [ATA_MSG_ERR]        = KERN_ERR,
>     [ATA_MSG_WARNING]    = KERN_WARNING,
>     ...
> };
> 
> #define ata_port_printk(ap, lv, fmt, args...) do { \
>     if (unlikely((ap)->msg_enable & (1 << (lv))))
>         printk(__ata_msg_lvs[lv]"ata%u: "fmt, (ap)->id , #args);

Oops,
	printk("%sata%u: "fmt, __ata_msg_lvs[lv], (ap)->id, #args);

-- 
tejun

Re: [PATCH] libata-core.c: restore configuration boot messages in ata_dev_configure()

[Borislav Petkov]

On Tue, Jun 27, 2006 at 10:33:38PM +0900, Tejun Heo wrote:
> Tejun Heo wrote:
> >Actually, how about...
> >
> >enum {
> >    ATA_MSG_ERR,
> >    ATA_MSG_WARNING,
> >    ...
> >};
> >
> >const char *__ata_msg_lvs[] = {
> >    [ATA_MSG_ERR]        = KERN_ERR,
> >    [ATA_MSG_WARNING]    = KERN_WARNING,
> >    ...
> >};
> >
> >#define ata_port_printk(ap, lv, fmt, args...) do { \
> >    if (unlikely((ap)->msg_enable & (1 << (lv))))
> >        printk(__ata_msg_lvs[lv]"ata%u: "fmt, (ap)->id , #args);
> 
> Oops,
> 	printk("%sata%u: "fmt, __ata_msg_lvs[lv], (ap)->id, #args);
Tejun,
    are you sure about that, printk gets as first arg the KERN_* message level,
    so your first suggestion should be right as passing __ata_msg_lvs[lv] is the
    first arg, no?

		
___________________________________________________________ 
Telefonate ohne weitere Kosten vom PC zum PC: http://messenger.yahoo.de

Re: [PATCH] libata-core.c: restore configuration boot messages in ata_dev_configure()

[Borislav Petkov]

On Tue, Jun 27, 2006 at 10:33:38PM +0900, Tejun Heo wrote:
> Tejun Heo wrote:
> >Actually, how about...
> >
> >enum {
> >    ATA_MSG_ERR,
> >    ATA_MSG_WARNING,
> >    ...
> >};
> >
> >const char *__ata_msg_lvs[] = {
> >    [ATA_MSG_ERR]        = KERN_ERR,
> >    [ATA_MSG_WARNING]    = KERN_WARNING,
> >    ...
> >};
> >
> >#define ata_port_printk(ap, lv, fmt, args...) do { \
> >    if (unlikely((ap)->msg_enable & (1 << (lv))))
> >        printk(__ata_msg_lvs[lv]"ata%u: "fmt, (ap)->id , #args);
> 
> Oops,
> 	printk("%sata%u: "fmt, __ata_msg_lvs[lv], (ap)->id, #args);
Oops, forget it, the coffee hasn't entered my brain yet so things like that might
    happen, __ata_msg_lvs[lv] is a char array, so the second one _is_ correct.

Regards,
    Boris.


		
___________________________________________________________ 
Telefonate ohne weitere Kosten vom PC zum PC: http://messenger.yahoo.de