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
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
