On 12/23/16, 1:01 AM, "Bart Van Assche" <bart.vanass...@sandisk.com> wrote:
>On Wed, 2016-12-21 at 13:57 -0800, Himanshu Madhani wrote:
>> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
>> index f7df01b..b14455e 100644
>> --- a/drivers/scsi/qla2xxx/qla_def.h
>> +++ b/drivers/scsi/qla2xxx/qla_def.h
>> @@ -1556,7 +1556,8 @@ struct link_statistics {
>> struct atio {
>> uint8_t entry_type; /* Entry type. */
>> uint8_t entry_count; /* Entry count. */
>> - uint8_t data[58];
>> + uint16_t attr_n_length;
>> + uint8_t data[56];
>> uint32_t signature;
>> #define ATIO_PROCESSED 0xDEADDEAD /* Signature */
>> };
>
>Are struct atio and/or struct atio_from_isp the description of a firmware
>data structure? If so, do all firmware versions initialize the attr_n_length
>field or is there a minimum firmware version required for this field to be
>valid? Please mention any changed dependencies on the firmware version in the
>patch description. Please also fix the typo in the patch title when reposting
>this patch ("corrputed").
They represent same data structure from firmware. We will send follow up patch
to
Clean up redundant structure.
>
>> + /* This packet is corrupted. The header + payload
>> + * can not be trusted. There is no point in passing
>> + * it further up.
>> + */
>
>This is not the proper Linux kernel comment style (see also
>Documentation/process/coding-style.rst).
Ack. Will update patch with correct format.
>
>> diff --git a/drivers/scsi/qla2xxx/qla_target.h
>> b/drivers/scsi/qla2xxx/qla_target.h
>> index f26c5f6..f31d343 100644
>> --- a/drivers/scsi/qla2xxx/qla_target.h
>> +++ b/drivers/scsi/qla2xxx/qla_target.h
>> @@ -427,13 +427,33 @@ struct atio_from_isp {
>> struct {
>> uint8_t entry_type; /* Entry type. */
>> uint8_t entry_count; /* Entry count. */
>> - uint8_t data[58];
>> + uint16_t attr_n_length;
>> +#define FCP_CMD_LENGTH_MASK 0x0fff
>> +#define FCP_CMD_LENGTH_MIN 0x38
>> + uint8_t data[56];
>> uint32_t signature;
>> #define ATIO_PROCESSED 0xDEADDEAD /* Signature */
>> } raw;
>> } u;
>> } __packed;
>>
>> +static inline int fcpcmd_is_corrupted(struct atio *atio)
>> +{
>> + if (atio->entry_type == ATIO_TYPE7 &&
>> + (le16_to_cpu(atio->attr_n_length & FCP_CMD_LENGTH_MASK) <
>> + FCP_CMD_LENGTH_MIN))
>> + return 1;
>> + else
>> + return 0;
>> +}
>
>The above code shows that attr_n_length is a little endian field so please
>declare it as little endian where appropriate (__le16 instead of uint16_t).
Ack. Will update the patch
>
>Bart.
