Re: [PATCH 10/15] lpfc: Fix NVME I+T not registering NVME as a supported FC4 type

James Smart
2017-05-15T21:57:57Z

On 5/11/2017 12:35 AM, Hannes Reinecke wrote:
> On 05/10/2017 09:07 PM, James Smart wrote:
>> When the driver send the RPA command, it does not
>> send supported FC4 Type NVME to the management server.
>>
>> Encode NVME (type x28) in the AttribEntry in the
>> RPA command.
>>
>> Signed-off-by: Dick Kennedy <dick.kennedy@broadcom.com>
>> Signed-off-by: James Smart <james.smart@broadcom.com>
>> ---
>> drivers/scsi/lpfc/lpfc_ct.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/scsi/lpfc/lpfc_ct.c b/drivers/scsi/lpfc/lpfc_ct.c
>> index c7962dae4dab..f2cd19c6c2df 100644
>> --- a/drivers/scsi/lpfc/lpfc_ct.c
>> +++ b/drivers/scsi/lpfc/lpfc_ct.c
>> @@ -2092,6 +2092,7 @@ lpfc_fdmi_port_attr_fc4type(struct lpfc_vport *vport,
>>
>> ae->un.AttrTypes[3] = 0x02; /* Type 1 - ELS */
>> ae->un.AttrTypes[2] = 0x01; /* Type 8 - FCP */
>> + ae->un.AttrTypes[6] = 0x01; /* Type 40 - NVME */
>> ae->un.AttrTypes[7] = 0x01; /* Type 32 - CT */
>> size = FOURBYTES + 32;
>> ad->AttrLen = cpu_to_be16(size);
>>
> Are you sure about the 'Type 40' ?
> I was under the impression that FC-4 Types is a bitmask of available
> types, so if bit6 really refer to Type 40, how can bit7 refer to
> type 32 ?

The code is actually correct.

Here's what's happening:
The RPA payload contains a bitmask with a bit per type bit. In their
infinite wisdom, FC decided that the bitmask was actually an array of
32-bit words, with bits set within the words. But, as FC is a wire
endianness of BE, it means the bytestream on the wire is W0B3, W0B2,
W0B1, W0B0, W1B3, W1B2, W1B1, W1B0, and so on. Ugly... Because of this
weird byte order due to BE words, FCP type, which is a lower bit number
than ELS type, is actually in a higher byte. Same goes for NVME (0x28)
vs CT (0x20).

I agree, it's really hard to follow this convolution. For now, please
accept the change. I've assigned a task on our end to refactor the code
so its understandable, and as it's common code, add the implementation
to the general fc framework.

-- james