diff mbox

[2/2] libata-scsi: do not return t10 designator if drive has WWN

Message ID 577d824d.a10c420a.da50c.ffff9544@mx.google.com
State Not Applicable
Delegated to: David Miller
Headers show

Commit Message

Tom Yan July 6, 2016, 10:12 p.m. UTC 
From: Tom Yan <tom.ty89@gmail.com>

SAT (as of sat4r05f.pdf) only requires the t10 designator if the
drive does not support/have WWN. Besides, we already have the ATA
information VPD.

Signed-off-by: Tom Yan <tom.ty89@gmail.com>

Comments

Sergei Shtylyov July 7, 2016, 10:51 a.m. UTC | #1 
Hello.

On 7/7/2016 1:12 AM, tom.ty89@gmail.com wrote:

> From: Tom Yan <tom.ty89@gmail.com>
>
> SAT (as of sat4r05f.pdf) only requires the t10 designator if the
> drive does not support/have WWN. Besides, we already have the ATA
> information VPD.
>
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 9f478ad..84b3d42 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
[...]
> @@ -2236,6 +2221,23 @@ static unsigned int ata_scsiop_inq_83(struct ata_scsi_args *args, u8 *rbuf)
>  			      ATA_ID_WWN, ATA_ID_WWN_LEN);
>  		num += ATA_ID_WWN_LEN;
>  	}
> +	else {

    CodingStyle, should be:

	} else {

[...]

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hannes Reinecke July 7, 2016, 10:56 a.m. UTC | #2 
On 07/07/2016 12:12 AM, tom.ty89@gmail.com wrote:
> From: Tom Yan <tom.ty89@gmail.com>
> 
> SAT (as of sat4r05f.pdf) only requires the t10 designator if the
> drive does not support/have WWN. Besides, we already have the ATA
> information VPD.
> 
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 9f478ad..84b3d42 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2210,21 +2210,6 @@ static unsigned int ata_scsiop_inq_83(struct ata_scsi_args *args, u8 *rbuf)
>  	rbuf[1] = 0x83;			/* this page code */
>  	num = 4;
>  
> -	/* SAT defined lu model and serial numbers descriptor */
> -	/* piv=0, assoc=lu, code_set=ACSII, designator=t10 vendor id */
> -	rbuf[num + 0] = 2;
> -	rbuf[num + 1] = 1;
> -	rbuf[num + 3] = sat_model_serial_desc_len;
> -	num += 4;
> -	memcpy(rbuf + num, "ATA     ", 8);
> -	num += 8;
> -	ata_id_string(args->id, (unsigned char *) rbuf + num, ATA_ID_PROD,
> -		      ATA_ID_PROD_LEN);
> -	num += ATA_ID_PROD_LEN;
> -	ata_id_string(args->id, (unsigned char *) rbuf + num, ATA_ID_SERNO,
> -		      ATA_ID_SERNO_LEN);
> -	num += ATA_ID_SERNO_LEN;
> -
>  	if (ata_id_has_wwn(args->id)) {
>  		/* SAT defined lu world wide name */
>  		/* piv=0, assoc=lu, code_set=binary, designator=NAA */
> @@ -2236,6 +2221,23 @@ static unsigned int ata_scsiop_inq_83(struct ata_scsi_args *args, u8 *rbuf)
>  			      ATA_ID_WWN, ATA_ID_WWN_LEN);
>  		num += ATA_ID_WWN_LEN;
>  	}
> +	else {
> +		/* SAT defined lu model and serial numbers descriptor */
> +		/* piv=0, assoc=lu, code_set=ACSII, designator=t10 vendor id */
> +		rbuf[num + 0] = 2;
> +		rbuf[num + 1] = 1;
> +		rbuf[num + 3] = sat_model_serial_desc_len;
> +		num += 4;
> +		memcpy(rbuf + num, "ATA     ", 8);
> +		num += 8;
> +		ata_id_string(args->id, (unsigned char *) rbuf + num, ATA_ID_PROD,
> +			      ATA_ID_PROD_LEN);
> +		num += ATA_ID_PROD_LEN;
> +		ata_id_string(args->id, (unsigned char *) rbuf + num, ATA_ID_SERNO,
> +			      ATA_ID_SERNO_LEN);
> +		num += ATA_ID_SERNO_LEN;
> +	}
> +
>  	rbuf[3] = num - 4;    /* page len (assume less than 256 bytes) */
>  	return 0;
>  }
> 
Nope.
We cannot go about and remove VPD descriptors.
Existing systems might rely on the VPD attribute to be present (think of
udev persistent symlinks), and the system will refuse to boot.
NACK.

Cheers,

Hannes
Tom Yan July 7, 2016, 12:40 p.m. UTC | #3 
Well, udev uses its own `ata_id` (which issues IDENTIFY DEVICE through
ATA PASS-THROUGH) though.

Anyway I expected the reasoning you gave and I can't really argue with
you. It's just personally I still prefer a cleaner SATL implementation
(considering Linux is open source and can be deemed as some sort of
reference), so I gave it a go.

Not that SAT requires the DI VPD return only one desingator / LU name though.

On 7 July 2016 at 18:56, Hannes Reinecke <hare@suse.de> wrote:
> On 07/07/2016 12:12 AM, tom.ty89@gmail.com wrote:
>> From: Tom Yan <tom.ty89@gmail.com>
>>
>> SAT (as of sat4r05f.pdf) only requires the t10 designator if the
>> drive does not support/have WWN. Besides, we already have the ATA
>> information VPD.
>>
>> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
>>
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 9f478ad..84b3d42 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -2210,21 +2210,6 @@ static unsigned int ata_scsiop_inq_83(struct ata_scsi_args *args, u8 *rbuf)
>>       rbuf[1] = 0x83;                 /* this page code */
>>       num = 4;
>>
>> -     /* SAT defined lu model and serial numbers descriptor */
>> -     /* piv=0, assoc=lu, code_set=ACSII, designator=t10 vendor id */
>> -     rbuf[num + 0] = 2;
>> -     rbuf[num + 1] = 1;
>> -     rbuf[num + 3] = sat_model_serial_desc_len;
>> -     num += 4;
>> -     memcpy(rbuf + num, "ATA     ", 8);
>> -     num += 8;
>> -     ata_id_string(args->id, (unsigned char *) rbuf + num, ATA_ID_PROD,
>> -                   ATA_ID_PROD_LEN);
>> -     num += ATA_ID_PROD_LEN;
>> -     ata_id_string(args->id, (unsigned char *) rbuf + num, ATA_ID_SERNO,
>> -                   ATA_ID_SERNO_LEN);
>> -     num += ATA_ID_SERNO_LEN;
>> -
>>       if (ata_id_has_wwn(args->id)) {
>>               /* SAT defined lu world wide name */
>>               /* piv=0, assoc=lu, code_set=binary, designator=NAA */
>> @@ -2236,6 +2221,23 @@ static unsigned int ata_scsiop_inq_83(struct ata_scsi_args *args, u8 *rbuf)
>>                             ATA_ID_WWN, ATA_ID_WWN_LEN);
>>               num += ATA_ID_WWN_LEN;
>>       }
>> +     else {
>> +             /* SAT defined lu model and serial numbers descriptor */
>> +             /* piv=0, assoc=lu, code_set=ACSII, designator=t10 vendor id */
>> +             rbuf[num + 0] = 2;
>> +             rbuf[num + 1] = 1;
>> +             rbuf[num + 3] = sat_model_serial_desc_len;
>> +             num += 4;
>> +             memcpy(rbuf + num, "ATA     ", 8);
>> +             num += 8;
>> +             ata_id_string(args->id, (unsigned char *) rbuf + num, ATA_ID_PROD,
>> +                           ATA_ID_PROD_LEN);
>> +             num += ATA_ID_PROD_LEN;
>> +             ata_id_string(args->id, (unsigned char *) rbuf + num, ATA_ID_SERNO,
>> +                           ATA_ID_SERNO_LEN);
>> +             num += ATA_ID_SERNO_LEN;
>> +     }
>> +
>>       rbuf[3] = num - 4;    /* page len (assume less than 256 bytes) */
>>       return 0;
>>  }
>>
> Nope.
> We cannot go about and remove VPD descriptors.
> Existing systems might rely on the VPD attribute to be present (think of
> udev persistent symlinks), and the system will refuse to boot.
> NACK.
>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke                Teamlead Storage & Networking
> hare@suse.de                                   +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
> HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hannes Reinecke July 7, 2016, 12:44 p.m. UTC | #4 
On 07/07/2016 02:40 PM, Tom Yan wrote:
> Well, udev uses its own `ata_id` (which issues IDENTIFY DEVICE through
> ATA PASS-THROUGH) though.
> 
> Anyway I expected the reasoning you gave and I can't really argue with
> you. It's just personally I still prefer a cleaner SATL implementation
> (considering Linux is open source and can be deemed as some sort of
> reference), so I gave it a go.
> 
> Not that SAT requires the DI VPD return only one desingator / LU name though.
> 
Really?

sat-r08 has:

One identification descriptor for a logical unit (i.e., a logical unit
name) shall be included (see clause 10.3.4.2).
In some environments, one or more additional identification descriptors
may be included (see clause 10.3.4.3).

Am I misreading something?

Cheers,

Hannes
Tom Yan July 7, 2016, 1:18 p.m. UTC | #5 
Yeah it said "One identification descriptor..." but not "Only one
identification descriptor...". So I suppose it is *alright* to also
return t10 designator in addition to the WWN. It's just redundunt and
unnecessary (and that's why I sent the patch), since we only need one
LU name (either derive from the WWN, or the t10 identification when
WWN is not available):

If the ATA IDENTIFY DEVICE data word 87 bit 8 is set to one indicating
that the ATA device supports the
WORLD WIDE NAME field (i.e., ATA IDENTIFY DEVICE data words 108 to
111), then the SATL shall include a
designation descriptor containing a logical unit name as defined in 10.5.4.2.2.

If the ATA IDENTIFY DEVICE data word 87 bit 8 is set to zero,
indicating that the ATA device does not support
the WORLD WIDE NAME field (i.e., ATA IDENTIFY DEVICE data words 108 to
111), then the SATL shall include
an identification descriptor containing a logical unit name as defined
in 10.5.4.2.3.

(sat4r05f.pdf)

On 7 July 2016 at 20:44, Hannes Reinecke <hare@suse.de> wrote:
> On 07/07/2016 02:40 PM, Tom Yan wrote:
>> Well, udev uses its own `ata_id` (which issues IDENTIFY DEVICE through
>> ATA PASS-THROUGH) though.
>>
>> Anyway I expected the reasoning you gave and I can't really argue with
>> you. It's just personally I still prefer a cleaner SATL implementation
>> (considering Linux is open source and can be deemed as some sort of
>> reference), so I gave it a go.
>>
>> Not that SAT requires the DI VPD return only one desingator / LU name though.
>>
> Really?
>
> sat-r08 has:
>
> One identification descriptor for a logical unit (i.e., a logical unit
> name) shall be included (see clause 10.3.4.2).
> In some environments, one or more additional identification descriptors
> may be included (see clause 10.3.4.3).
>
> Am I misreading something?
>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke                Teamlead Storage & Networking
> hare@suse.de                                   +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
> HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 9f478ad..84b3d42 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2210,21 +2210,6 @@  static unsigned int ata_scsiop_inq_83(struct ata_scsi_args *args, u8 *rbuf)
 	rbuf[1] = 0x83;			/* this page code */
 	num = 4;
 
-	/* SAT defined lu model and serial numbers descriptor */
-	/* piv=0, assoc=lu, code_set=ACSII, designator=t10 vendor id */
-	rbuf[num + 0] = 2;
-	rbuf[num + 1] = 1;
-	rbuf[num + 3] = sat_model_serial_desc_len;
-	num += 4;
-	memcpy(rbuf + num, "ATA     ", 8);
-	num += 8;
-	ata_id_string(args->id, (unsigned char *) rbuf + num, ATA_ID_PROD,
-		      ATA_ID_PROD_LEN);
-	num += ATA_ID_PROD_LEN;
-	ata_id_string(args->id, (unsigned char *) rbuf + num, ATA_ID_SERNO,
-		      ATA_ID_SERNO_LEN);
-	num += ATA_ID_SERNO_LEN;
-
 	if (ata_id_has_wwn(args->id)) {
 		/* SAT defined lu world wide name */
 		/* piv=0, assoc=lu, code_set=binary, designator=NAA */
@@ -2236,6 +2221,23 @@  static unsigned int ata_scsiop_inq_83(struct ata_scsi_args *args, u8 *rbuf)
 			      ATA_ID_WWN, ATA_ID_WWN_LEN);
 		num += ATA_ID_WWN_LEN;
 	}
+	else {
+		/* SAT defined lu model and serial numbers descriptor */
+		/* piv=0, assoc=lu, code_set=ACSII, designator=t10 vendor id */
+		rbuf[num + 0] = 2;
+		rbuf[num + 1] = 1;
+		rbuf[num + 3] = sat_model_serial_desc_len;
+		num += 4;
+		memcpy(rbuf + num, "ATA     ", 8);
+		num += 8;
+		ata_id_string(args->id, (unsigned char *) rbuf + num, ATA_ID_PROD,
+			      ATA_ID_PROD_LEN);
+		num += ATA_ID_PROD_LEN;
+		ata_id_string(args->id, (unsigned char *) rbuf + num, ATA_ID_SERNO,
+			      ATA_ID_SERNO_LEN);
+		num += ATA_ID_SERNO_LEN;
+	}
+
 	rbuf[3] = num - 4;    /* page len (assume less than 256 bytes) */
 	return 0;
 }