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