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.
Thanks Joe for reviewing these patches.
Vasu
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
