Zou, Yi wrote:
>> Joe Eykholt wrote:
>>> Zou, Yi wrote:
>>>>> Yi Zou wrote:
>>>>>> We are always sending the max frame size from lport->mfs when sending
>> the
>>>>> FIP
>>>>>> keep alive even there are no vport desciptors. There is no need to send
>> the
>>>>>> full sized frame for keep-alive.
>>>>>>
>>>>>> Signed-off-by: Yi Zou <[email protected]>
>>>>>> ---
>>>>>>
>>>>>> drivers/scsi/fcoe/libfcoe.c | 5 +++--
>>>>>> 1 files changed, 3 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/scsi/fcoe/libfcoe.c b/drivers/scsi/fcoe/libfcoe.c
>>>>>> index 2988b71..b9c56ff 100644
>>>>>> --- a/drivers/scsi/fcoe/libfcoe.c
>>>>>> +++ b/drivers/scsi/fcoe/libfcoe.c
>>>>>> @@ -351,8 +351,9 @@ static void fcoe_ctlr_send_keep_alive(struct
>> fcoe_ctlr
>>>>> *fip,
>>>>>> if (!fcf || !fc_host_port_id(lp->host))
>>>>>> return;
>>>>>>
>>>>>> - len = fcoe_ctlr_fcoe_size(fip) + sizeof(struct ethhdr);
>>>>>> - BUG_ON(len < sizeof(*kal) + sizeof(*vn));
>>>>>> + len = sizeof(*kal) + ports * sizeof(*vn);
>>>>>> + if (len < ETH_ZLEN)
>>>>>> + len = ETH_ZLEN;
>>>>>> skb = dev_alloc_skb(len);
>>>>>> if (!skb)
>>>>>> return;
>>>>>>
>>>>> I guess a preliminary version of FIP must've specified that Keep Alives
>> were
>>>>> padded in order to verify the fabric can still carry jumbo frames.
>>>>> I may have just heard this in a conversation somewhere.
>>>>> It doens't have anything to do with whether there is a port parameter or
>>>>> vports. The BUG_ON is just paranoia to say the padding is always going
>>>>> to give us a big enough skb.
>>>>>
>>>>> However, I can't find anything about padding keep-alives in the
>>>>> current spec. It only talks about padding the solicited advertisements.
>>>>>
>>>>> I still think it's a good idea to pad the keep-alives.
>>>>> Maybe we should get the spec. to state that.
>>>>>
>>>>> Joe
>>>> I was looking at the T11/09-117v1 as the FC-BB-5 for updated FIP updated,
>>>> where in the end of 7.7.6.2, it says "FIP_Pad field shall be of zero
>> length
>>>> for all other FIP operations.", where as you mentioned only the solicited
>>>> advertisements must have the FIP pad.
>>>>
>>>> I will double-check on the spec tomorrow to make sure.
>>> I agree, the spec says there should be no PAD on keep alives.
>>> That's the same version I checked.
>>>
>>> However, extra bytes past the end of the frame are not going to cause
>>> a problem. Note that the descriptor length field of the
>>> keep-alive does not include those extra bytes.
>>>
>>> kal->fip.fip_subcode = FIP_SC_KEEP_ALIVE;
>>> kal->fip.fip_dl_len = htons((sizeof(kal->mac) +
>>> ports * sizeof(*vn)) / FIP_BPW);
>>>
>>> BTW, in your change, the check against ETH_ZLEN is not necessary, the
>>> NIC will do any padding needed to get to the minimum frame size.
>>> Otherwise, we would have to check that in other places.
>>>
>>> But, padding to ETH_ZLEN proves my point. It's OK to have the
>>> extra bytes there.
>> On further thought, I have no objections to shortening the keep-alives.
>> But, the check for ETH_ZLEN should be left out.
>>
>> Also, the patch commentary shouldn't mention vports. That had nothing
>> to do with the padding.
>>
>> Joe
> I think I added ETH_ZLEN as I read in the spec somewhere the KLA needs be
> at least minimum size. So, my read is that the KLA should not be padded if
> exceeding minimum ethernet frame size.
I don't understand what you're saying here. We already agreed not to pad the
keep-alive. The NIC cannot transmit less than the minimum Ethernet frame
size, but we don't know whether there will be a VLAN header or not, and
the NIC does know. The upper layers (like FIP) don't do the padding
to the minimum size ... that's up to the NIC. So it's correct to
leave out the ETH_ZLEN check.
> Yes, the patch description is misleading and thanks for pointing that out.
>
> Apart from the kla msg size, I also think the fcoe_send_keep_alive() may need
> some more fixing as it does not seem to be able to send kla for different
> Vx_ports, since it is always taking the fcoe_ctrl lport's fcid and wwpn. I
> am not completely sure though.
Each vx_port has an lport that is used for its port keep-alives. There's no
problem.
Cheers,
Joe
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
