[v3] ATAPI support on SiI SATA adapter

[edk2] [PATCH v3 2/7] MdePkg IndustryStandard/Scsi.h: Add sense code macro

Posted by Jeremy Linton 7 years, 8 months ago

Add some definitions to mask the sense key from sense data,
and check the validity of the returned sense data.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 MdePkg/Include/IndustryStandard/Scsi.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MdePkg/Include/IndustryStandard/Scsi.h b/MdePkg/Include/IndustryStandard/Scsi.h
index 0d81314..802479e 100644
--- a/MdePkg/Include/IndustryStandard/Scsi.h
+++ b/MdePkg/Include/IndustryStandard/Scsi.h
@@ -369,6 +369,8 @@ typedef struct {
 //
 // Sense Key
 //
+#define EFI_SCSI_REQUEST_SENSE_ERROR  (0x70)
+#define EFI_SCSI_SK_VALUE(byte)       (byte&0x0F)
 #define EFI_SCSI_SK_NO_SENSE          (0x0)
 #define EFI_SCSI_SK_RECOVERY_ERROR    (0x1)
 #define EFI_SCSI_SK_NOT_READY         (0x2)
-- 
2.9.3

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v3 2/7] MdePkg IndustryStandard/Scsi.h: Add sense code macro

Posted by Tian, Feng 7 years, 8 months ago

Linton,

Could you let me know where the +#define EFI_SCSI_REQUEST_SENSE_ERROR  (0x70) comes from?

According to SCSI SPC5r08 spec, the first byte of sense data is the RESPONSE CODE field. 

And 0x70 means

a) the result of an error, exception condition, or protocol specific failure that is associated with CHECK
CONDITION status; or
b) additional information that is associated with a status other than CHECK CONDITION.

It's possible that 0x70 means success... so this naming may be not quite appropriate.

And in your usage, you forcedly convert Packet->SenseData to UINT8 and access each sense data field by array index. I think it's unnecessary. Packet->SenseData is EFI_SCSI_SENSE_DATA type, which has defined the meaning of each field. With this structure, you don't need introduce EFI_SCSI_SK_VALUE macro again.

+        sense = (UINT8 *)Packet->SenseData;
+        if ((Packet->SenseDataLength > 13) &&
+            (sense[0] & EFI_SCSI_REQUEST_SENSE_ERROR)) {
+          DEBUG ((DEBUG_INFO, "SiI3132ScsiPassRead() Key %X ASC(Q) %02X%02X\n",
+                  EFI_SCSI_SK_VALUE (sense[2]), sense[12], sense[13]));
+        }

Thanks
Feng

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jeremy Linton
Sent: Friday, February 24, 2017 6:34 AM
To: edk2-devel@lists.01.org
Cc: ard.biesheuvel@linaro.org; Steve.Capper@arm.com; ryan.harkin@linaro.org; leif.lindholm@linaro.org; linaro-uefi@lists.linaro.org
Subject: [edk2] [PATCH v3 2/7] MdePkg IndustryStandard/Scsi.h: Add sense code macro

Add some definitions to mask the sense key from sense data, and check the validity of the returned sense data.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 MdePkg/Include/IndustryStandard/Scsi.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MdePkg/Include/IndustryStandard/Scsi.h b/MdePkg/Include/IndustryStandard/Scsi.h
index 0d81314..802479e 100644
--- a/MdePkg/Include/IndustryStandard/Scsi.h
+++ b/MdePkg/Include/IndustryStandard/Scsi.h
@@ -369,6 +369,8 @@ typedef struct {
 //
 // Sense Key
 //
+#define EFI_SCSI_REQUEST_SENSE_ERROR  (0x70)
+#define EFI_SCSI_SK_VALUE(byte)       (byte&0x0F)
 #define EFI_SCSI_SK_NO_SENSE          (0x0)
 #define EFI_SCSI_SK_RECOVERY_ERROR    (0x1)
 #define EFI_SCSI_SK_NOT_READY         (0x2)
--
2.9.3

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v3 2/7] MdePkg IndustryStandard/Scsi.h: Add sense code macro

Posted by Jeremy Linton 7 years, 8 months ago

Hi,

On 02/24/2017 12:42 AM, Tian, Feng wrote:
> Linton,
>
> Could you let me know where the +#define EFI_SCSI_REQUEST_SENSE_ERROR  (0x70) comes from?

Its being anded with the response code field in an attempt to pick off 
0x70..0x72 as a sanity check that the response data looks valid given 
that its also long enough to be a valid return.

So, your right its not well named either. Likely as its just a debug 
print, the best plan is to drop the whole thing, particularly since the 
transport code is now stable enough that the sense data is usually valid.





>
> According to SCSI SPC5r08 spec, the first byte of sense data is the RESPONSE CODE field.
>
> And 0x70 means
>
> a) the result of an error, exception condition, or protocol specific failure that is associated with CHECK
> CONDITION status; or
> b) additional information that is associated with a status other than CHECK CONDITION.
>
> It's possible that 0x70 means success... so this naming may be not quite appropriate.
>
> And in your usage, you forcedly convert Packet->SenseData to UINT8 and access each sense data field by array index. I think it's unnecessary. Packet->SenseData is EFI_SCSI_SENSE_DATA type, which has defined the meaning of each field. With this structure, you don't need introduce EFI_SCSI_SK_VALUE macro again.
>
> +        sense = (UINT8 *)Packet->SenseData;
> +        if ((Packet->SenseDataLength > 13) &&
> +            (sense[0] & EFI_SCSI_REQUEST_SENSE_ERROR)) {
> +          DEBUG ((DEBUG_INFO, "SiI3132ScsiPassRead() Key %X ASC(Q) %02X%02X\n",
> +                  EFI_SCSI_SK_VALUE (sense[2]), sense[12], sense[13]));
> +        }
>
> Thanks
> Feng
>
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jeremy Linton
> Sent: Friday, February 24, 2017 6:34 AM
> To: edk2-devel@lists.01.org
> Cc: ard.biesheuvel@linaro.org; Steve.Capper@arm.com; ryan.harkin@linaro.org; leif.lindholm@linaro.org; linaro-uefi@lists.linaro.org
> Subject: [edk2] [PATCH v3 2/7] MdePkg IndustryStandard/Scsi.h: Add sense code macro
>
> Add some definitions to mask the sense key from sense data, and check the validity of the returned sense data.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  MdePkg/Include/IndustryStandard/Scsi.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/MdePkg/Include/IndustryStandard/Scsi.h b/MdePkg/Include/IndustryStandard/Scsi.h
> index 0d81314..802479e 100644
> --- a/MdePkg/Include/IndustryStandard/Scsi.h
> +++ b/MdePkg/Include/IndustryStandard/Scsi.h
> @@ -369,6 +369,8 @@ typedef struct {
>  //
>  // Sense Key
>  //
> +#define EFI_SCSI_REQUEST_SENSE_ERROR  (0x70)
> +#define EFI_SCSI_SK_VALUE(byte)       (byte&0x0F)
>  #define EFI_SCSI_SK_NO_SENSE          (0x0)
>  #define EFI_SCSI_SK_RECOVERY_ERROR    (0x1)
>  #define EFI_SCSI_SK_NOT_READY         (0x2)
> --
> 2.9.3
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel