Vasu Dev wrote:
> On Tue, 2009-03-24 at 18:45 -0700, Joe Eykholt wrote:
>> Vasu Dev wrote:
>>> On Mon, 2009-03-23 at 16:49 -0700, Joe Eykholt wrote:
>>>> Vasu Dev wrote:
>>>>> + if (netdev->netdev_ops &&
>> netdev->netdev_ops->ndo_get_storage_address) {
>>>>> + memcpy(fc->ctlr.ctl_src_addr,
>>>>> + netdev->netdev_ops->ndo_get_storage_address(netdev),
>>>>> + ETH_ALEN);
>>>>> + if (is_valid_ether_addr(fc->ctlr.ctl_src_addr))
>>>>> + fc->ctlr.spma = 1;
>>>> Wouldn't it be a bug in the LLD to give an invalid MAC here?
>>>> It's OK to check, though.
>>>>
>>> Let us leave this in to ensure MAC address validity, however LLD are
>> not
>>> expected to give bad MAC address, anyway this code is not in fast
>> path
>>> so one additional check won't be expensive here.
>>>
>>>>> sol->fip.fip_dl_len = htons(sizeof(sol->desc) / FIP_BPW);
>>>>> sol->fip.fip_flags = htons(FIP_FL_FPMA);
>>>>> + if (fip->spma)
>>>>> + sol->fip.fip_flags |= htons(FIP_FL_SPMA);
>>>> Lets's add a u16 capability field in the fcoe_ctlr, which would
>>>> contain FPMA or SPMA or both, and here we would just use that to
>> set the
>>>> flags in the packet. We could use a be16 field, but u16 is better
>> so
>>>> we can test it instead of using fip->spma.
>>>>
>>> I think using u8 spma flag makes code clean by simply checking a
>> flag,
>>> otherwise spma bit in u16 or b16 would have required read,
>> bit-wise-AND
>>> and then check in execution. We are not forcing spma or fpma mode
>> only
>>> with these patches so adding a explicit bit or non-bit flag for fpma
>>> might me redundant at this point. If needed then I'll add u16 flag
>>> later.
>>>>> - if (dtype != ELS_FLOGI)
>>>>> - memcpy(mac->fd_mac, fip->data_src_addr, ETH_ALEN);
>>>>> + if (fip->spma)
>>>>> + memcpy(mac->fd_mac, fip->ctl_src_addr, ETH_ALEN);
>>>> For non-FLOGI, non-SPMA, I think we want the data_src address
>> still.
>>> The fd_mac was never set before due to a wrong ELS_FLOGI check and now
>>> with this change it will be set to only ctl_src_addr, I see this is also
>>> not good. Good catch, I'll provide fix for this.
>> No, it was correct before.
>> On FLOGIs we want the MAC descriptor mac to be zero
>> unless we're using SPMA (that should be based on the selected FCF, not
>> fip->spma).
>
> The MAC descriptor is still zero with this patch for FLOGIs in fpma only
> mode since in that case spma flag won't be set and in turn the above
> if(fip-spma) condition will fail before MAC descriptor setting. It'll be
> set to proposed spma MAC address from initiator when spma flag set which
> is as per spec you referred below, so all looks good for both modes for
> FLOGIs, I'm addressing issue with other ELSs and MAC descriptor setting
> later below.
>
> My previous response was inaccurate when I referred to removed wrong
> check (dtype != ELS_FLOGI), the ELS_FLOGI = 0x4 was compared against
> FIP descriptor dtype, thus check was mostly true including for FLOGI
> dtype FIP_DT_FLOGI = 0x7. So fd_mac was always/mostly set, not never set
> as I said before. However fip->data_src_addr happened to be zero for
> least very first FLOGI, so perhaps that wrong check was not an issue
> before.
>
>
>> For other ELSes, we want the assigned data_src_addr, as it was before.
>
> I already acked this issue and said will provide fix for this, how
> about fixing this as:-
>
>
> if (dtype != FIP_DT_FLOGI)
> memcpy(mac->fd_mac, fip->data_src_addr, ETH_ALEN);
> if (fip->spma && dtype == FIP_DT_FLOGI)
> memcpy(mac->fd_mac, fip->ctl_src_addr, ETH_ALEN);
>
> Vasu
>
OK, I didn't see the problem you were trying to point out before.
How about:
> if (dtype != FIP_DT_FLOGI)
> memcpy(mac->fd_mac, fip->data_src_addr, ETH_ALEN);
> else if (fip->spma)
> memcpy(mac->fd_mac, fip->ctl_src_addr, ETH_ALEN);
Just a tiny improvement.
Thanks,
Joe
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
