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

[Jeremy Linton <jeremy.linton@arm.com>]

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
>