>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. I probably did not make it clear. Yes, I agree with you the check should be left out.
> >> 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 > You are right, I went to read the spec. on vx_port descriptor, what I suspected is not true, so we are good there. Thanks for the clarification. So, I think the only fix needed here is to not pad the kla. yi _______________________________________________ devel mailing list [email protected] http://www.open-fcoe.org/mailman/listinfo/devel
