[PATCH 1/n] libata-core.c conversion to new debugging scheme, part 1 (25% done)

[PATCH 1/n] libata-core.c conversion to new debugging scheme, part 1 (25% done)

[Borislav Petkov]

This patch converts the first 25% of libata-core.c to the new debugging scheme.

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


--- libata-dev/drivers/scsi/libata-core.c.orig	2006-05-30 22:02:51.000000000 +0200
+++ libata-dev/drivers/scsi/libata-core.c	2006-05-31 07:37:44.000000000 +0200
@@ -411,7 +411,7 @@ static const char *sata_spd_string(unsig
 
 void ata_dev_disable(struct ata_device *dev)
 {
-	if (ata_dev_enabled(dev)) {
+	if (ata_dev_enabled(dev) && ata_msg_drv(dev->ap)) {
 		ata_dev_printk(dev, KERN_WARNING, "disabled\n");
 		dev->class++;
 	}
@@ -770,8 +770,12 @@ void ata_std_dev_select (struct ata_port
 void ata_dev_select(struct ata_port *ap, unsigned int device,
 			   unsigned int wait, unsigned int can_sleep)
 {
-	VPRINTK("ENTER, ata%u: device %u, wait %u\n",
-		ap->id, device, wait);
+	if (ata_msg_probe(ap)) {
+		struct ata_device *dev = container_of(&ap, struct ata_device, ap);
+		ata_dev_printk(dev, KERN_INFO, "ata_dev_select: ENTER, ata%u: "
+				"device %u, wait %u\n",
+				ap->id, device, wait);
+	}
 
 	if (wait)
 		ata_wait_idle(ap);
@@ -1297,18 +1301,21 @@ static int ata_dev_configure(struct ata_
 	unsigned int xfer_mask;
 	int i, rc;
 
-	if (!ata_dev_enabled(dev)) {
-		DPRINTK("ENTER/EXIT (host %u, dev %u) -- nodev\n",
-			ap->id, dev->devno);
+	if (!ata_dev_enabled(dev) && ata_msg_info(ap)) {
+		ata_dev_printk(dev, KERN_INFO, "%s: ENTER/EXIT (host %u, dev %u) -- nodev\n",
+			__FUNCTION__, ap->id, dev->devno);
 		return 0;
 	}
 
-	DPRINTK("ENTER, host %u, dev %u\n", ap->id, dev->devno);
+	if (ata_msg_probe(ap))
+		ata_dev_printk(dev, KERN_DEBUG, "%s: ENTER, host %u, dev %u\n", 
+			__FUNCTION__, ap->id, dev->devno);
 
 	/* print device capabilities */
-	if (print_info)
-		ata_dev_printk(dev, KERN_DEBUG, "cfg 49:%04x 82:%04x 83:%04x "
+	if (ata_msg_probe(ap))
+		ata_dev_printk(dev, KERN_DEBUG, "%s: 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]);
 
@@ -1328,7 +1335,8 @@ static int ata_dev_configure(struct ata_
 	/* find max transfer mode; for printk only */
 	xfer_mask = ata_id_xfermask(id);
 
-	ata_dump_id(id);
+	if (ata_msg_probe(ap))
+		ata_dump_id(id);
 
 	/* ATA-specific feature tests */
 	if (dev->class == ATA_DEV_ATA) {
@@ -1349,7 +1357,7 @@ static int ata_dev_configure(struct ata_
 			ata_dev_config_ncq(dev, ncq_desc, sizeof(ncq_desc));
 
 			/* print device info to dmesg */
-			if (print_info)
+			if (ata_msg_info(ap))
 				ata_dev_printk(dev, KERN_INFO, "ATA-%d, "
 					"max %s, %Lu sectors: %s %s\n",
 					ata_id_major_version(id),
@@ -1372,7 +1380,7 @@ static int ata_dev_configure(struct ata_
 			}
 
 			/* print device info to dmesg */
-			if (print_info)
+			if (ata_msg_info(ap))
 				ata_dev_printk(dev, KERN_INFO, "ATA-%d, "
 					"max %s, %Lu sectors: CHS %u/%u/%u\n",
 					ata_id_major_version(id),
@@ -1383,7 +1391,8 @@ static int ata_dev_configure(struct ata_
 
 		if (dev->id[59] & 0x100) {
 			dev->multi_count = dev->id[59] & 0xff;
-			DPRINTK("ata%u: dev %u multi count %u\n",
+			if (ata_msg_info(ap))
+				ata_dev_printk(dev, KERN_INFO, "ata%u: dev %u multi count %u\n",
 				ap->id, dev->devno, dev->multi_count);
 		}
 
@@ -1396,8 +1405,9 @@ static int ata_dev_configure(struct ata_
 
 		rc = atapi_cdb_len(id);
 		if ((rc < 12) || (rc > ATAPI_CDB_LEN)) {
-			ata_dev_printk(dev, KERN_WARNING,
-				       "unsupported CDB len\n");
+			if (ata_msg_warn(ap))
+				ata_dev_printk(dev, KERN_WARNING, 
+					"unsupported CDB len\n");
 			rc = -EINVAL;
 			goto err_out_nosup;
 		}
@@ -1409,7 +1419,7 @@ static int ata_dev_configure(struct ata_
 		}
 
 		/* print device info to dmesg */
-		if (print_info)
+		if (ata_msg_info(ap))
 			ata_dev_printk(dev, KERN_INFO, "ATAPI, max %s%s\n",
 				       ata_mode_string(xfer_mask),
 				       cdb_intr_string);
@@ -1423,7 +1433,7 @@ static int ata_dev_configure(struct ata_
 
 	/* limit bridge transfers to udma5, 200 sectors */
 	if (ata_dev_knobble(dev)) {
-		if (print_info)
+		if (ata_msg_info(ap))
 			ata_dev_printk(dev, KERN_INFO,
 				       "applying bridge limits\n");
 		dev->udma_mask &= ATA_UDMA5;
@@ -1433,11 +1443,15 @@ static int ata_dev_configure(struct ata_
 	if (ap->ops->dev_config)
 		ap->ops->dev_config(ap, dev);
 
-	DPRINTK("EXIT, drv_stat = 0x%x\n", ata_chk_status(ap));
+	if (ata_msg_probe(ap))
+		ata_dev_printk(dev, KERN_DEBUG, "%s: EXIT, drv_stat = 0x%x\n",
+			__FUNCTION__, ata_chk_status(ap));
 	return 0;
 
 err_out_nosup:
-	DPRINTK("EXIT, err\n");
+	if (ata_msg_probe(ap))
+		ata_dev_printk(dev, KERN_DEBUG, 
+				"%s: EXIT, err\n", __FUNCTION__);
 	return rc;
 }
 
@@ -1478,8 +1492,9 @@ static int ata_bus_probe(struct ata_port
 	if (ap->ops->probe_reset) {
 		rc = ap->ops->probe_reset(ap, classes);
 		if (rc) {
-			ata_port_printk(ap, KERN_ERR,
-					"reset failed (errno=%d)\n", rc);
+			if (ata_msg_err(ap))
+				ata_port_printk(ap, KERN_ERR, 
+						"reset failed (errno=%d)\n", rc);
 			return rc;
 		}
 	} else {

	

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

Re: [PATCH 1/n] libata-core.c conversion to new debugging scheme, part 1 (25% done)

[Jeff Garzik]

On Wed, May 31, 2006 at 07:46:33AM +0200, Borislav Petkov wrote:
> This patch converts the first 25% of libata-core.c to the new debugging scheme.
> 
> Signed-off-by: <petkov@uni-muenster.de>

Looks good at first glance, though I'll hold off on apply until I return
from the Red Hat Summit in Nashville.

It made me remember another point, though:  in order to avoid
regressions, after applying your patch, I would think that you would
want to create a patch which did something like

#ifndef ATA_VERBOSE_DEBUG
	ap->msg_enable = xxx
#else ATA_DEBUG
	ap->msg_enable = yyy
#else
	ap->msg_enable = ...
#endif

Thus, we are assured that applying your patch will not change the
behavior(much?) when ATA_VERBOSE_DEBUG is enabled.

Regards,

	Jeff

Re: [PATCH 1/n] libata-core.c conversion to new debugging scheme, part 1 (25% done)

[Borislav Petkov]

On Wed, May 31, 2006 at 02:39:57AM -0400, Jeff Garzik wrote:
> On Wed, May 31, 2006 at 07:46:33AM +0200, Borislav Petkov wrote:
> > This patch converts the first 25% of libata-core.c to the new debugging scheme.
> > 
> > Signed-off-by: <petkov@uni-muenster.de>
> 
> Looks good at first glance, though I'll hold off on apply until I return
> from the Red Hat Summit in Nashville.
> 
> It made me remember another point, though:  in order to avoid
> regressions, after applying your patch, I would think that you would
> want to create a patch which did something like
> 
> #ifndef ATA_VERBOSE_DEBUG
> 	ap->msg_enable = xxx
> #else ATA_DEBUG
> 	ap->msg_enable = yyy
> #else
> 	ap->msg_enable = ...
> #endif

Ok, i was thinking something along the lines of the following; for better
granularity we might add more ATA_XXX defines or let the user subsequently
adjust dbg level through sysfs...? (The patch applies on top of the one i sent you
before.)

Adjust initial debugging levels through preprocessor defines.

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

 
--- libata-dev/drivers/scsi/libata-core.c.orig1	2006-06-01 07:02:42.000000000 +0200
+++ libata-dev/drivers/scsi/libata-core.c	2006-06-01 07:32:44.000000000 +0200
@@ -5200,7 +5200,15 @@ static void ata_host_init(struct ata_por
 	ap->sata_spd_limit = UINT_MAX;
 	ap->active_tag = ATA_TAG_POISON;
 	ap->last_ctl = 0xFF;
-	ap->msg_enable = ATA_MSG_DRV;
+
+#if defined(ATA_VERBOSE_DEBUG)
+	/* turn on all debugging levels */
+	ap->msg_enable = 0x00FF;
+#elif defined(ATA_DEBUG)
+	ap->msg_enable = ATA_MSG_DRV | ATA_MSG_INFO | ATA_MSG_CTL | ATA_MSG_WARN | ATA_MSG_ERR;
+#else 
+	ap->msg_enable = ATA_MSG_DRV | ATA_MSG_ERR;
+#endif
 
 	INIT_WORK(&ap->port_task, NULL, NULL);
 	INIT_LIST_HEAD(&ap->eh_done_q);

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

Re: [PATCH 1/n] libata-core.c conversion to new debugging scheme, part 1 (25% done)

[Tejun Heo]

Borislav Petkov wrote:
> On Wed, May 31, 2006 at 02:39:57AM -0400, Jeff Garzik wrote:
>> On Wed, May 31, 2006 at 07:46:33AM +0200, Borislav Petkov wrote:
>>> This patch converts the first 25% of libata-core.c to the new debugging scheme.
>>>
>>> Signed-off-by: <petkov@uni-muenster.de>
>> Looks good at first glance, though I'll hold off on apply until I return
>> from the Red Hat Summit in Nashville.
>>
>> It made me remember another point, though:  in order to avoid
>> regressions, after applying your patch, I would think that you would
>> want to create a patch which did something like
>>
>> #ifndef ATA_VERBOSE_DEBUG
>> 	ap->msg_enable = xxx
>> #else ATA_DEBUG
>> 	ap->msg_enable = yyy
>> #else
>> 	ap->msg_enable = ...
>> #endif
> 
> Ok, i was thinking something along the lines of the following; for better
> granularity we might add more ATA_XXX defines or let the user subsequently
> adjust dbg level through sysfs...? (The patch applies on top of the one i sent you
> before.)
> 
> Adjust initial debugging levels through preprocessor defines.
> 
> Signed-off-by: <petkov@uni-muenster.de>
> 
>  
> --- libata-dev/drivers/scsi/libata-core.c.orig1	2006-06-01 07:02:42.000000000 +0200
> +++ libata-dev/drivers/scsi/libata-core.c	2006-06-01 07:32:44.000000000 +0200
> @@ -5200,7 +5200,15 @@ static void ata_host_init(struct ata_por
>  	ap->sata_spd_limit = UINT_MAX;
>  	ap->active_tag = ATA_TAG_POISON;
>  	ap->last_ctl = 0xFF;
> -	ap->msg_enable = ATA_MSG_DRV;
> +
> +#if defined(ATA_VERBOSE_DEBUG)
> +	/* turn on all debugging levels */
> +	ap->msg_enable = 0x00FF;
> +#elif defined(ATA_DEBUG)
> +	ap->msg_enable = ATA_MSG_DRV | ATA_MSG_INFO | ATA_MSG_CTL | ATA_MSG_WARN | ATA_MSG_ERR;
> +#else 
> +	ap->msg_enable = ATA_MSG_DRV | ATA_MSG_ERR;
> +#endif
>  
>  	INIT_WORK(&ap->port_task, NULL, NULL);
>  	INIT_LIST_HEAD(&ap->eh_done_q);

Hello,

Those ATA_MSG_* constants designates two things..

* message level (debug, info, warning...)
* message origin (probe, intr...)

although above distinction isn't clear for some constants.  libata now 
uses ata_port/dev_printk() macros to print messages and the second 
argument is message level (KERN_INFO, KERN_WARNING...), which carries 
duplicate information as above ATA_* constants.  IMHO, it would be 
better to fold the two into one.  e.g.

   ata_port_printk(ap, ATA_MSG_INFO, "blah blah\n");

instead of

   if (ata_msg_info(ap))
	ata_port_printk(ap, KERN_INFO, "blah blah\n");

Some constants probably need to be adjusted a bit though.

Thanks.

-- 
tejun

Re: [PATCH 1/n] libata-core.c conversion to new debugging scheme, part 1 (25% done)

[Jeff Garzik]

On Thu, Jun 01, 2006 at 03:09:31PM +0900, Tejun Heo wrote:
> Those ATA_MSG_* constants designates two things..
> 
> * message level (debug, info, warning...)
> * message origin (probe, intr...)
> 
> although above distinction isn't clear for some constants.  libata now 
> uses ata_port/dev_printk() macros to print messages and the second 
> argument is message level (KERN_INFO, KERN_WARNING...), which carries 
> duplicate information as above ATA_* constants.  IMHO, it would be 
> better to fold the two into one.  e.g.
> 
>   ata_port_printk(ap, ATA_MSG_INFO, "blah blah\n");
> 
> instead of
> 
>   if (ata_msg_info(ap))
> 	ata_port_printk(ap, KERN_INFO, "blah blah\n");
> 
> Some constants probably need to be adjusted a bit though.

Although I agree, it is best to integrate the msg_enable support in
"waves", not doing too much at once, so that we have time to reflect
better on the next step -- just like what we're doing now.

The ata_msg_xxx level and KERN_xxx level are not perfectly mapped, nor
should they be, so the proposed scheme may not work out once we have all
the message levels integrated.

The most important goal to achieve is (a) eliminating the need for users
to rebuild libata to get verbose messages and (b) allowing fine-grained
per-port verbose debugging and tracing.  We can address cleanups after
those goals.

	Jeff

Re: [PATCH 1/n] libata-core.c conversion to new debugging scheme, part 1 (25% done)

[Borislav Petkov]

> Some constants probably need to be adjusted a bit though.
I couldn't agree more, I'll think of some mapping later on and post it here.

Regards,
	Boris.

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

Re: [PATCH 1/n] libata-core.c conversion to new debugging scheme, part 1 (25% done)

[Borislav Petkov]

On Thu, Jun 01, 2006 at 02:14:33AM -0400, Jeff Garzik wrote:
> The ata_msg_xxx level and KERN_xxx level are not perfectly mapped, nor
> should they be, so the proposed scheme may not work out once we have all
> the message levels integrated.

How about the folowing mapping for ATA-to-printk debugging levels:

ATA_MSG_DRV		----> KERN_INFO
ATA_MSG_INFO

ATA_MSG_PROBE	----> KERN_DEBUG
ATA_MSG_MALLOC
ATA_MSG_CTL
ATA_MSG_INTR

since those are in their nature debugging infos containing ENTER/EXIT
function_name calls

ATA_MSG_WARN	----> KERN_WARNING

ATA_MSG_ERR		----> KERN_ERR

but this I could do after the whole conversion thing is done so we have the new
dbg infrastructure first.

Regards,
	Boris.

	

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

Re: [PATCH 1/n] libata-core.c conversion to new debugging scheme, part 1 (25% done)

[Jeff Garzik]

Borislav Petkov wrote:
> On Wed, May 31, 2006 at 02:39:57AM -0400, Jeff Garzik wrote:
>> On Wed, May 31, 2006 at 07:46:33AM +0200, Borislav Petkov wrote:
>>> This patch converts the first 25% of libata-core.c to the new debugging scheme.
>>>
>>> Signed-off-by: <petkov@uni-muenster.de>
>> Looks good at first glance, though I'll hold off on apply until I return
>> from the Red Hat Summit in Nashville.
>>
>> It made me remember another point, though:  in order to avoid
>> regressions, after applying your patch, I would think that you would
>> want to create a patch which did something like
>>
>> #ifndef ATA_VERBOSE_DEBUG
>> 	ap->msg_enable = xxx
>> #else ATA_DEBUG
>> 	ap->msg_enable = yyy
>> #else
>> 	ap->msg_enable = ...
>> #endif
> 
> Ok, i was thinking something along the lines of the following; for better
> granularity we might add more ATA_XXX defines or let the user subsequently
> adjust dbg level through sysfs...? (The patch applies on top of the one i sent you
> before.)
> 
> Adjust initial debugging levels through preprocessor defines.
> 
> Signed-off-by: <petkov@uni-muenster.de>
> 
>  
> --- libata-dev/drivers/scsi/libata-core.c.orig1	2006-06-01 07:02:42.000000000 +0200
> +++ libata-dev/drivers/scsi/libata-core.c	2006-06-01 07:32:44.000000000 +0200
> @@ -5200,7 +5200,15 @@ static void ata_host_init(struct ata_por
>  	ap->sata_spd_limit = UINT_MAX;
>  	ap->active_tag = ATA_TAG_POISON;
>  	ap->last_ctl = 0xFF;
> -	ap->msg_enable = ATA_MSG_DRV;
> +
> +#if defined(ATA_VERBOSE_DEBUG)
> +	/* turn on all debugging levels */
> +	ap->msg_enable = 0x00FF;
> +#elif defined(ATA_DEBUG)
> +	ap->msg_enable = ATA_MSG_DRV | ATA_MSG_INFO | ATA_MSG_CTL | ATA_MSG_WARN | ATA_MSG_ERR;
> +#else 
> +	ap->msg_enable = ATA_MSG_DRV | ATA_MSG_ERR;
> +#endif

applied

Re: [PATCH 1/n] libata-core.c conversion to new debugging scheme, part 1 (25% done)

[Jeff Garzik]

Borislav Petkov wrote:
> This patch converts the first 25% of libata-core.c to the new debugging scheme.
> 
> Signed-off-by: <petkov@uni-muenster.de>

After I hand-applied a lot of this, and looked at the result, I would 
request further updates:

1) The message level order is a bit off.  Following the original 
Becker-designed strategy (netif.txt), the ATA_MSG_xxx bits should be 
ordered in order of verbosity, where that order is passed to 
ata_msg_init() from the user.

This implies that that ATA_MSG_ERR (the most important) should be 0x1, 
_WARN should be 0x2, PROBE(?), DRV(?), and so on.

2) rediff and resend this "part 1" patch against the latest 
libata-dev.git#upstream, which looked mostly OK

	Jeff