[PATCH 1/1] libata: rearrange dmesg info to add full ATA revision

[PATCH 1/1] libata: rearrange dmesg info to add full ATA revision

[Eric D. Mudama]

Per Jeff's suggestion, this patch rearranges the info printed for ATA
drives into dmesg to add the full ATA firmware revision and model
information, while keeping the output to 2 lines.

Signed-off-by: Eric D. Mudama <edmudama@gmail.com>

---

This extra information is helpful for debugging drive-related issues
on ATA drives connected with libata, especially when the user can't
easily run hdparm.

Diff is against 2.6.20-rc6 (roughly).

Here's a snippet of my new dmesg with this patch, showing the full and
truncated firmware revision information:

[   44.559351] ata3: SATA max UDMA/133 cmd 0xB400 ctl 0xB802 bmdma 0xC400 irq 19
[   44.559407] ata4: SATA max UDMA/133 cmd 0xBC00 ctl 0xC002 bmdma 0xC408 irq 19
[   44.559450] scsi2 : ata_piix
[   44.723970] ata3.00: ATA-7: ST3250820AS, 3.AAC, max UDMA/133
[   44.724012] ata3.00: 488395055 sectors, multi 16: LBA48 NCQ (depth 0/32)
[   44.732057] ata3.00: configured for UDMA/133
[   44.732099] scsi3 : ata_piix
[   44.897435] ata4.00: ATA-7: Maxtor 6V100E0, VA111910, max UDMA/133
[   44.897476] ata4.00: 195813072 sectors, multi 16: LBA48 NCQ (depth 0/32)
[   44.909506] ata4.00: configured for UDMA/133
[   44.909618] scsi 2:0:0:0: Direct-Access     ATA      ST3250820AS      3.AA PQ: 0 ANSI: 5
[   44.910500] scsi 3:0:0:0: Direct-Access     ATA      Maxtor 6V100E0   VA11 PQ: 0 ANSI: 5


 libata-core.c |   46 ++++++++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 16 deletions(-)


diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index a388a8d..b8f7223 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1607,6 +1607,8 @@ int ata_dev_configure(struct ata_device *dev)
 	const u16 *id = dev->id;
 	unsigned int xfer_mask;
 	char revbuf[7];		/* XYZ-99\0 */
+	char fwrevbuf[9];
+	char modelbuf[41];
 	int rc;
 
 	if (!ata_dev_enabled(dev) && ata_msg_info(ap)) {
@@ -1661,6 +1663,16 @@ int ata_dev_configure(struct ata_device *dev)
 
 		dev->n_sectors = ata_id_n_sectors(id);
 
+		/* SCSI only uses 4-char revisions, dump full 8 chars from ATA */
+		ata_id_c_string(dev->id, fwrevbuf, ATA_ID_FW_REV_OFS,
+				sizeof(fwrevbuf));
+
+		ata_id_c_string(dev->id, modelbuf, ATA_ID_PROD_OFS,
+				sizeof(modelbuf));
+
+		if (dev->id[59] & 0x100)
+			dev->multi_count = dev->id[59] & 0xff;
+
 		if (ata_id_has_lba(id)) {
 			const char *lba_desc;
 			char ncq_desc[20];
@@ -1680,13 +1692,18 @@ int ata_dev_configure(struct ata_device *dev)
 			ata_dev_config_ncq(dev, ncq_desc, sizeof(ncq_desc));
 
 			/* print device info to dmesg */
-			if (ata_msg_drv(ap) && print_info)
-				ata_dev_printk(dev, KERN_INFO, "%s, "
-					"max %s, %Lu sectors: %s %s\n",
+			if (ata_msg_drv(ap) && print_info) {
+				ata_dev_printk(dev, KERN_INFO,
+					"%s: %s, %s, max %s\n",
 					revbuf,
-					ata_mode_string(xfer_mask),
+					modelbuf, fwrevbuf,
+					ata_mode_string(xfer_mask));
+				ata_dev_printk(dev, KERN_INFO,
+					"%Lu sectors, multi %u: %s %s\n",
 					(unsigned long long)dev->n_sectors,
+					dev->multi_count,
 					lba_desc, ncq_desc);
+			}
 		} else {
 			/* CHS */
 
@@ -1703,22 +1720,19 @@ int ata_dev_configure(struct ata_device *dev)
 			}
 
 			/* print device info to dmesg */
-			if (ata_msg_drv(ap) && print_info)
-				ata_dev_printk(dev, KERN_INFO, "%s, "
-					"max %s, %Lu sectors: CHS %u/%u/%u\n",
+			if (ata_msg_drv(ap) && print_info) {
+				ata_dev_printk(dev, KERN_INFO,
+					"%s: %s, %s, max %s\n",
 					revbuf,
-					ata_mode_string(xfer_mask),
+					modelbuf, fwrevbuf,
+					ata_mode_string(xfer_mask));
+				ata_dev_printk(dev, KERN_INFO, 
+					"%Lu sectors, multi %u, CHS %u/%u/%u\n",
 					(unsigned long long)dev->n_sectors,
+					dev->multi_count,
 					dev->cylinders, dev->heads,
 					dev->sectors);
-		}
-
-		if (dev->id[59] & 0x100) {
-			dev->multi_count = dev->id[59] & 0xff;
-			if (ata_msg_drv(ap) && print_info)
-				ata_dev_printk(dev, KERN_INFO,
-					"ata%u: dev %u multi count %u\n",
-					ap->id, dev->devno, dev->multi_count);
+			}
 		}
 
 		dev->cdb_len = 16;

Re: [PATCH 1/1] libata: rearrange dmesg info to add full ATA revision

[Tejun Heo]

Hello, Eric.

Eric D. Mudama wrote:
> Per Jeff's suggestion, this patch rearranges the info printed for ATA
> drives into dmesg to add the full ATA firmware revision and model
> information, while keeping the output to 2 lines.
> 
> Signed-off-by: Eric D. Mudama <edmudama@gmail.com>

The patch is formatted and applies perfectly.  I'm glad to see this
change.  Just a few nits below.

>  	char revbuf[7];		/* XYZ-99\0 */
> +	char fwrevbuf[9];
> +	char modelbuf[41];

Please use ATA_ID_FW_REV_LEN + 1 and ATA_ID_PROD_LEN + 1.

> @@ -1680,13 +1692,18 @@ int ata_dev_configure(struct ata_device *dev)
>  			ata_dev_config_ncq(dev, ncq_desc, sizeof(ncq_desc));
>  
>  			/* print device info to dmesg */
> -			if (ata_msg_drv(ap) && print_info)
> -				ata_dev_printk(dev, KERN_INFO, "%s, "
> -					"max %s, %Lu sectors: %s %s\n",
> +			if (ata_msg_drv(ap) && print_info) {
> +				ata_dev_printk(dev, KERN_INFO,
> +					"%s: %s, %s, max %s\n",
>  					revbuf,
> -					ata_mode_string(xfer_mask),
> +					modelbuf, fwrevbuf,

Please merge the above line with the line above it.

> +					ata_mode_string(xfer_mask));
> +				ata_dev_printk(dev, KERN_INFO,
> +					"%Lu sectors, multi %u: %s %s\n",
>  					(unsigned long long)dev->n_sectors,
> +					dev->multi_count,
>  					lba_desc, ncq_desc);

Ditto.

> +			}
>  		} else {
>  			/* CHS */
>  
> @@ -1703,22 +1720,19 @@ int ata_dev_configure(struct ata_device *dev)
>  			}
>  
>  			/* print device info to dmesg */
> -			if (ata_msg_drv(ap) && print_info)
> -				ata_dev_printk(dev, KERN_INFO, "%s, "
> -					"max %s, %Lu sectors: CHS %u/%u/%u\n",
> +			if (ata_msg_drv(ap) && print_info) {
> +				ata_dev_printk(dev, KERN_INFO,
> +					"%s: %s, %s, max %s\n",
>  					revbuf,
> -					ata_mode_string(xfer_mask),
> +					modelbuf, fwrevbuf,

Ditto.

> +					ata_mode_string(xfer_mask));
> +				ata_dev_printk(dev, KERN_INFO, 
> +					"%Lu sectors, multi %u, CHS %u/%u/%u\n",
>  					(unsigned long long)dev->n_sectors,
> +					dev->multi_count,
>  					dev->cylinders, dev->heads,
>  					dev->sectors);

I would prefer

dev->multi_count, dev->cylinders,
dev->heads, dev->sectors

Other than these,

Acked-by: Tejun Heo <htejun@gmail.com>

-- 
tejun

Re: [PATCH 1/1] libata: rearrange dmesg info to add full ATA revision

[Tejun Heo]

[The previous replay mysteriously didn't include Eric in To:, sorry,
quoting whole message here.]

Tejun Heo wrote:
> Hello, Eric.
> 
> Eric D. Mudama wrote:
>> Per Jeff's suggestion, this patch rearranges the info printed for ATA
>> drives into dmesg to add the full ATA firmware revision and model
>> information, while keeping the output to 2 lines.
>>
>> Signed-off-by: Eric D. Mudama <edmudama@gmail.com>
> 
> The patch is formatted and applies perfectly.  I'm glad to see this
> change.  Just a few nits below.
> 
>>  	char revbuf[7];		/* XYZ-99\0 */
>> +	char fwrevbuf[9];
>> +	char modelbuf[41];
> 
> Please use ATA_ID_FW_REV_LEN + 1 and ATA_ID_PROD_LEN + 1.

This depends on to which version this patch applies.  For
#upstream-fixes (2.6.20-rc6), you have to use raw numbers as you did.
For #upstream, there are above two constants to use.  I think this patch
is good for 2.6.20 as it's safe && will help us analyzing bug reports
for 2.6.20.  So, ignore this part of the comment.

Jeff, if it gets merged into #upstream through #upstream-fixes, please
don't forget to change above constants.

>> @@ -1680,13 +1692,18 @@ int ata_dev_configure(struct ata_device *dev)
>>  			ata_dev_config_ncq(dev, ncq_desc, sizeof(ncq_desc));
>>  
>>  			/* print device info to dmesg */
>> -			if (ata_msg_drv(ap) && print_info)
>> -				ata_dev_printk(dev, KERN_INFO, "%s, "
>> -					"max %s, %Lu sectors: %s %s\n",
>> +			if (ata_msg_drv(ap) && print_info) {
>> +				ata_dev_printk(dev, KERN_INFO,
>> +					"%s: %s, %s, max %s\n",
>>  					revbuf,
>> -					ata_mode_string(xfer_mask),
>> +					modelbuf, fwrevbuf,
> 
> Please merge the above line with the line above it.
> 
>> +					ata_mode_string(xfer_mask));
>> +				ata_dev_printk(dev, KERN_INFO,
>> +					"%Lu sectors, multi %u: %s %s\n",
>>  					(unsigned long long)dev->n_sectors,
>> +					dev->multi_count,
>>  					lba_desc, ncq_desc);
> 
> Ditto.
> 
>> +			}
>>  		} else {
>>  			/* CHS */
>>  
>> @@ -1703,22 +1720,19 @@ int ata_dev_configure(struct ata_device *dev)
>>  			}
>>  
>>  			/* print device info to dmesg */
>> -			if (ata_msg_drv(ap) && print_info)
>> -				ata_dev_printk(dev, KERN_INFO, "%s, "
>> -					"max %s, %Lu sectors: CHS %u/%u/%u\n",
>> +			if (ata_msg_drv(ap) && print_info) {
>> +				ata_dev_printk(dev, KERN_INFO,
>> +					"%s: %s, %s, max %s\n",
>>  					revbuf,
>> -					ata_mode_string(xfer_mask),
>> +					modelbuf, fwrevbuf,
> 
> Ditto.
> 
>> +					ata_mode_string(xfer_mask));
>> +				ata_dev_printk(dev, KERN_INFO, 
>> +					"%Lu sectors, multi %u, CHS %u/%u/%u\n",
>>  					(unsigned long long)dev->n_sectors,
>> +					dev->multi_count,
>>  					dev->cylinders, dev->heads,
>>  					dev->sectors);
> 
> I would prefer
> 
> dev->multi_count, dev->cylinders,
> dev->heads, dev->sectors
> 
> Other than these,
> 
> Acked-by: Tejun Heo <htejun@gmail.com>
> 

-- 
tejun

Re: [PATCH 1/1] libata: rearrange dmesg info to add full ATA revision

[Eric D. Mudama]

Per Jeff's suggestion, this patch rearranges the info printed for ATA
drives into dmesg to add the full ATA firmware revision and model
information, while keeping the output to 2 lines.

Signed-off-by: Eric D. Mudama <edmudama@gmail.com>

---

This extra information is helpful for debugging drive-related issues
on ATA drives connected with libata, especially when the user can't
easily run hdparm.

Update: added Tejun's formatting requests, and created the constants
necessary.

Diff is against 2.6.20-rc6 (roughly).  Here's a snippet of my new
dmesg with this patch, showing the full and truncated firmware
revision information:

[   43.747027] scsi2 : ata_piix
[   43.911113] ata3.00: ATA-7: ST3250820AS, 3.AAC   ST325082, max UDMA/133
[   43.911155] ata3.00: 488395055 sectors, multi 16: LBA48 NCQ (depth 0/32)
[   43.919201] ata3.00: configured for UDMA/133
[   43.919241] scsi3 : ata_piix
[   44.084459] ata4.00: ATA-7: Maxtor 6V100E0, VA111910Maxtor 6, max UDMA/133
[   44.084502] ata4.00: 195813072 sectors, multi 16: LBA48 NCQ (depth 0/32)
[   44.096522] ata4.00: configured for UDMA/133
[   44.096638] scsi 2:0:0:0: Direct-Access     ATA      ST3250820AS      3.AA PQ: 0 ANSI: 5
[   44.097521] scsi 3:0:0:0: Direct-Access     ATA      Maxtor 6V100E0   VA11 PQ: 0 ANSI: 5


 drivers/ata/libata-core.c |   52 +++++++++++++++++++++++++++-------------------
 include/linux/ata.h       |    2 +
 2 files changed, 33 insertions(+), 21 deletions(-)


diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index a388a8d..f628dc5 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1607,6 +1607,8 @@ int ata_dev_configure(struct ata_device *dev)
 	const u16 *id = dev->id;
 	unsigned int xfer_mask;
 	char revbuf[7];		/* XYZ-99\0 */
+	char fwrevbuf[ATA_ID_FW_REV_LEN+1];
+	char modelbuf[ATA_ID_PROD_LEN+1];
 	int rc;
 
 	if (!ata_dev_enabled(dev) && ata_msg_info(ap)) {
@@ -1661,6 +1663,16 @@ int ata_dev_configure(struct ata_device *dev)
 
 		dev->n_sectors = ata_id_n_sectors(id);
 
+		/* SCSI only uses 4-char revisions, dump full 8 chars from ATA */
+		ata_id_c_string(dev->id, fwrevbuf, ATA_ID_FW_REV_OFS,
+				sizeof(fwrevbuf));
+
+		ata_id_c_string(dev->id, modelbuf, ATA_ID_PROD_OFS,
+				sizeof(modelbuf));
+
+		if (dev->id[59] & 0x100)
+			dev->multi_count = dev->id[59] & 0xff;
+
 		if (ata_id_has_lba(id)) {
 			const char *lba_desc;
 			char ncq_desc[20];
@@ -1680,13 +1692,16 @@ int ata_dev_configure(struct ata_device *dev)
 			ata_dev_config_ncq(dev, ncq_desc, sizeof(ncq_desc));
 
 			/* print device info to dmesg */
-			if (ata_msg_drv(ap) && print_info)
-				ata_dev_printk(dev, KERN_INFO, "%s, "
-					"max %s, %Lu sectors: %s %s\n",
-					revbuf,
-					ata_mode_string(xfer_mask),
+			if (ata_msg_drv(ap) && print_info) {
+				ata_dev_printk(dev, KERN_INFO,
+					"%s: %s, %s, max %s\n",
+					revbuf, modelbuf, fwrevbuf,
+					ata_mode_string(xfer_mask));
+				ata_dev_printk(dev, KERN_INFO,
+					"%Lu sectors, multi %u: %s %s\n",
 					(unsigned long long)dev->n_sectors,
-					lba_desc, ncq_desc);
+					dev->multi_count, lba_desc, ncq_desc);
+			}
 		} else {
 			/* CHS */
 
@@ -1703,22 +1718,17 @@ int ata_dev_configure(struct ata_device *dev)
 			}
 
 			/* print device info to dmesg */
-			if (ata_msg_drv(ap) && print_info)
-				ata_dev_printk(dev, KERN_INFO, "%s, "
-					"max %s, %Lu sectors: CHS %u/%u/%u\n",
-					revbuf,
-					ata_mode_string(xfer_mask),
-					(unsigned long long)dev->n_sectors,
-					dev->cylinders, dev->heads,
-					dev->sectors);
-		}
-
-		if (dev->id[59] & 0x100) {
-			dev->multi_count = dev->id[59] & 0xff;
-			if (ata_msg_drv(ap) && print_info)
+			if (ata_msg_drv(ap) && print_info) {
 				ata_dev_printk(dev, KERN_INFO,
-					"ata%u: dev %u multi count %u\n",
-					ap->id, dev->devno, dev->multi_count);
+					"%s: %s, %s, max %s\n",
+					revbuf,	modelbuf, fwrevbuf,
+					ata_mode_string(xfer_mask));
+				ata_dev_printk(dev, KERN_INFO, 
+					"%Lu sectors, multi %u, CHS %u/%u/%u\n",
+					(unsigned long long)dev->n_sectors,
+					dev->multi_count, dev->cylinders,
+					dev->heads, dev->sectors);
+			}
 		}
 
 		dev->cdb_len = 16;
diff --git a/include/linux/ata.h b/include/linux/ata.h
index 1df9416..aa9c1fd 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -60,6 +60,8 @@ enum {
 
 	ATA_PCI_CTL_OFS		= 2,
 	ATA_SERNO_LEN		= 20,
+	ATA_ID_PROD_LEN         = 40,
+	ATA_ID_FW_REV_LEN       = 16,
 	ATA_UDMA0		= (1 << 0),
 	ATA_UDMA1		= ATA_UDMA0 | (1 << 1),
 	ATA_UDMA2		= ATA_UDMA1 | (1 << 2),

Re: [PATCH 1/1] libata: rearrange dmesg info to add full ATA revision

[Jeff Garzik]

Tejun Heo wrote:
> [The previous replay mysteriously didn't include Eric in To:, sorry,
> quoting whole message here.]
> 
> Tejun Heo wrote:
>> Hello, Eric.
>>
>> Eric D. Mudama wrote:
>>> Per Jeff's suggestion, this patch rearranges the info printed for ATA
>>> drives into dmesg to add the full ATA firmware revision and model
>>> information, while keeping the output to 2 lines.
>>>
>>> Signed-off-by: Eric D. Mudama <edmudama@gmail.com>
>> The patch is formatted and applies perfectly.  I'm glad to see this
>> change.  Just a few nits below.
>>
>>>  	char revbuf[7];		/* XYZ-99\0 */
>>> +	char fwrevbuf[9];
>>> +	char modelbuf[41];
>> Please use ATA_ID_FW_REV_LEN + 1 and ATA_ID_PROD_LEN + 1.
> 
> This depends on to which version this patch applies.  For
> #upstream-fixes (2.6.20-rc6), you have to use raw numbers as you did.
> For #upstream, there are above two constants to use.  I think this patch
> is good for 2.6.20 as it's safe && will help us analyzing bug reports
> for 2.6.20.  So, ignore this part of the comment.

We have time to revise.  I only want to commit true bug fixes at this 
point, to 2.6.20.

	Jeff

Re: [PATCH 1/1] libata: rearrange dmesg info to add full ATA revision

[Jeff Garzik]

Eric D. Mudama wrote:
> Per Jeff's suggestion, this patch rearranges the info printed for ATA
> drives into dmesg to add the full ATA firmware revision and model
> information, while keeping the output to 2 lines.
> 
> Signed-off-by: Eric D. Mudama <edmudama@gmail.com>
> 
> ---
> 
> This extra information is helpful for debugging drive-related issues
> on ATA drives connected with libata, especially when the user can't
> easily run hdparm.
> 
> Update: added Tejun's formatting requests, and created the constants
> necessary.
> 
> Diff is against 2.6.20-rc6 (roughly).  Here's a snippet of my new
> dmesg with this patch, showing the full and truncated firmware
> revision information:

Looks pretty good to me at first glance.  I'll review it again tonight 
or tomorrow, when I make the next "apply SATA patches" pass.

FWIW, since this is not a bug fix, it will be going into 2.6.21 rather 
than 2.6.20.  As such, it will be applied to 
git://git.kernel.org/pub/scm/linux/kernel/git/jgarzik/libata-dev.git 
branch 'upstream' rather than branch 'upstream-fixes'.

Or, in shorthand, libata-dev.git#upstream.

Cheers and thanks,

	Jeff

Re: [PATCH 1/1] libata: rearrange dmesg info to add full ATA revision

[Jeff Garzik]

Eric D. Mudama wrote:
> Per Jeff's suggestion, this patch rearranges the info printed for ATA
> drives into dmesg to add the full ATA firmware revision and model
> information, while keeping the output to 2 lines.
> 
> Signed-off-by: Eric D. Mudama <edmudama@gmail.com>

> diff --git a/include/linux/ata.h b/include/linux/ata.h
> index 1df9416..aa9c1fd 100644
> --- a/include/linux/ata.h
> +++ b/include/linux/ata.h
> @@ -60,6 +60,8 @@ enum {
>  
>  	ATA_PCI_CTL_OFS		= 2,
>  	ATA_SERNO_LEN		= 20,
> +	ATA_ID_PROD_LEN         = 40,
> +	ATA_ID_FW_REV_LEN       = 16,
>  	ATA_UDMA0		= (1 << 0),
>  	ATA_UDMA1		= ATA_UDMA0 | (1 << 1),
>  	ATA_UDMA2		= ATA_UDMA1 | (1 << 2),

applied to netdev#upstream, sans the above part.  I'm not sure where you 
got 16 there?  It's 8 ASCII chars in the spec.

	Jeff

Re: [PATCH 1/1] libata: rearrange dmesg info to add full ATA revision

[Eric D. Mudama]

On 2/6/07, Jeff Garzik <jeff@garzik.org> wrote:
> >       ATA_SERNO_LEN           = 20,
> > +     ATA_ID_PROD_LEN         = 40,
> > +     ATA_ID_FW_REV_LEN       = 16,
> >       ATA_UDMA0               = (1 << 0),
> >       ATA_UDMA1               = ATA_UDMA0 | (1 << 1),
> >       ATA_UDMA2               = ATA_UDMA1 | (1 << 2),
>
> applied to netdev#upstream, sans the above part.  I'm not sure where you
> got 16 there?  It's 8 ASCII chars in the spec.

Yea, I'm not sure either.  I got the model length right, but brain
farted on the FW rev for some reason (which was the whole point)

Thx
--eric