>Zou, Yi wrote:
>>> Yi Zou wrote:
>>>> Upon receiving ELS_RLS, send the Link Error Status Block (LESB) back.
>>>>
>>>> Signed-off-by: Yi Zou <[email protected]>
>>>> ---
>>>>
>>>> drivers/scsi/libfc/fc_lport.c | 124
>>> +++++++++++++++++++++++++++++++++++++++++
>>>> 1 files changed, 124 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/scsi/libfc/fc_lport.c
>b/drivers/scsi/libfc/fc_lport.c
>>>> index bbf4152..48b8aae 100644
>>>> --- a/drivers/scsi/libfc/fc_lport.c
>>>> +++ b/drivers/scsi/libfc/fc_lport.c
>>>> @@ -526,6 +526,127 @@ static void fc_lport_recv_logo_req(struct fc_seq
>*sp,
>>> struct fc_frame *fp,
>>>> }
>>>>
>>>> /**
>>>> + * fc_lport_get_lesb() - Fill the Link Error Status Block
>>>> + * @lport: The local port recieving the RLS
>>>> + * @lesb: The FC-BB-5 Link Error Status Block to be filled in
>>>> + *
>>>> + * Fill the LESB with from the per-cpu fcoe statistics, return 0
>>>> + * when stats are successfully retrieved.
>>>> + */
>>>> +static int fc_lport_get_lesb(struct fc_lport *lport,
>>>> + struct fc_els_lesb_bb5 *lesb)
>
>This can return void.
Yeah, I was thinking if lld needs to return an error code upon
calling lld_stats().
>>>> +{
>>>> + unsigned int cpu;
>>>> + u32 lfc, vlfc, mdac;
>>>> + struct fcoe_dev_stats *devst;
>>>> + struct fcoe_lld_stats *lldst;
>>>> +
>>>> + lfc = 0;
>>>> + vlfc = 0;
>>>> + mdac = 0;
>>>> + memset(lesb, 0, sizeof(*lesb));
>>>> + for_each_possible_cpu(cpu) {
>>>> + devst = per_cpu_ptr(lport->dev_stats, cpu);
>>>> + lfc += devst->LinkFailureCount;
>>>> + vlfc += devst->VLinkFailureCount;
>>>> + mdac += devst->MissDiscAdvCount;
>>>> + }
>>>> + lesb->lesb_link_fail = __cpu_to_be32(lfc);
>>>> + lesb->lesb_vlink_fail = __cpu_to_be32(vlfc);
>>>> + lesb->lesb_miss_fip = __cpu_to_be32(mdac);
>>>> +
>>>> + lldst = &lport->lld_stats;
>>>> + if (lport->tt.lld_stats) {
>>>> + lport->tt.lld_stats(lport, lldst);
>>>> + lesb->lesb_symb_err = __cpu_to_be32(lldst->SymbolErrorCount);
>>>> + lesb->lesb_err_block = __cpu_to_be32(lldst->ErroredBlockCount);
>>>> + lesb->lesb_fcs_error = __cpu_to_be32(lldst->FCSErrorCount);
>
>I think most other places in libfc we use htonl(). Is that deprecated?
>Let's be consistent.
Will fix this. BTW, I'd want stick w/ "errored block count", as it is the term
used in most literatures I have found.
On a separate note for previous patch, for "struct fc_els_lesb_bb5", I think
it better to still stay in fc_els.h not fc_fcoe.h, since fc_rport.c has to
include fc_fcoe.h just for this sake. If you fill it up in libfcoe, the
lld_stats() has to take lesb struct as an input, making it a per els function
pointer, rather, we can extend fcoe_lld_stats struct to pass more stats from
LLD later if needed, not just lesb. It's part of fc-bb-5, does not make libfc
fcoe specific.
>
>>>> + }
>>>> + return 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * fc_lport_recv_rls_req() - Handle received Request Node ID data
>request
>
>Is the above comment wrong?
Will fix this as well.
>
>>>> + * @sp: The sequence in the RLS exchange
>>>> + * @fp: The RLS request frame
>>>> + * @lport: The local port recieving the RLS
>>>> + *
>>>> + * Locking Note: The lport lock is expected to be held before calling
>>>> + * this function.
>>>> + *
>>>> + * FIXME: how to check/indicate if we want support RLS for LESB?
>>> If the LLD supports it?
>> I don't know how the LLD will indicate to FCoE stack this is supported,
>> possibly by netdev flags as well? I wanted call it out as I did not
>> find any way to query if this feature is supported.
>
>I think RLS is always supported, right? Make the LLDs all support it,
>or indicate support with lport->tt.ldd_stats != NULL.
I guess so, the spec says to reject if RLS is not supported, which is why
I was wondering under what condition to not support it.
>>
>>>> + *
>>>> + */
>>>> +static void fc_lport_recv_rls_req(struct fc_seq *sp, struct fc_frame
>>> *in_fp,
>>>> + struct fc_lport *lport)
>>>> +{
>>>> + struct fc_frame *fp;
>>>> + struct fc_exch *ep = fc_seq_exch(sp);
>>>> + struct fc_els_rls *rls;
>>>> + struct fc_els_rls_resp *rsp;
>>>> + struct fc_seq_els_data rjt_data;
>>>> + struct fc_els_lesb_bb5 *lesb;
>>>> + struct fc_rport_priv *rdata;
>>>> + struct fc_frame_header *fh;
>>>> + size_t len;
>>>> + u32 f_ctl;
>>>> + u32 sid;
>>>> +
>>>> + FC_LPORT_DBG(lport, "Received RLS request while in state %s\n",
>>>> + fc_lport_state(lport));
>>>> +
>>>> + /* check if this port is logged in */
>>>> + fh = fc_frame_header_get(in_fp);
>>>> + sid = ntoh24(fh->fh_s_id);
>>>> + mutex_lock(&lport->disc.disc_mutex);
>>>> + rdata = lport->tt.rport_lookup(lport, sid);
>>>> + mutex_unlock(&lport->disc.disc_mutex);
>>> This should be done in fc_rport_recv_els_req(). The rport lookup is
>>> already done for you and the PLOGI needed error check is also done by then.
>>>
>> Good point, I missed that, plus your comment below. I initially started
>this
>> by sending ELS_NOLOGIN handled by lport. This should really be in rport.
>>
>> Thanks.
>> yi
>>
>>>> + if (!rdata) {
>>>> + FC_LPORT_DBG(lport, "LS_RJT %x:PLOGI needed\n", sid);
>>>> + rjt_data.reason = ELS_RJT_UNAB;
>>>> + rjt_data.explan = ELS_EXPL_PLOGI_REQD;
>>>> + goto out_rjt;
>>>> + }
>>>> + /* no rls payload */
>>>> + rls = fc_frame_payload_get(in_fp, sizeof(*rls));
>>>> + if (!rls) {
>>>> + rjt_data.reason = ELS_RJT_LOGIC;
>>>> + rjt_data.explan = ELS_EXPL_NONE;
>>>> + goto out_rjt;
>>>> + }
>>>> +
>>>> + len = sizeof(*rsp);
>>>> + fp = fc_frame_alloc(lport, len);
>>>> + if (!fp) {
>>>> + rjt_data.reason = ELS_RJT_UNAB;
>>>> + rjt_data.explan = ELS_EXPL_INSUF_RES;
>>>> + goto out_rjt;
>>>> + }
>>>> + rsp = fc_frame_payload_get(fp, len);
>>>> + memset(rsp, 0, len);
>>>> + rsp->rls_cmd = ELS_LS_ACC;
>>>> + lesb = (struct fc_els_lesb_bb5 *)&rsp->rls_lesb;
>>>> + if (fc_lport_get_lesb(lport, lesb)) {
>
>That always works, so no need to check for error or get a return value.
Yes.
>
>>>> + FC_LPORT_DBG(lport, "LS_RJT %x:failed to get lesb\n", sid);
>>>> + rjt_data.reason = ELS_RJT_UNAB;
>>>> + rjt_data.explan = ELS_EXPL_INSUF_RES;
>>>> + goto out_rjt;
>>>> + }
>>>> + /* send response */
>>>> + sp = lport->tt.seq_start_next(sp);
>>>> + f_ctl = FC_FC_EX_CTX | FC_FC_LAST_SEQ | FC_FC_END_SEQ;
>>>> + fc_fill_fc_hdr(fp, FC_RCTL_ELS_REP, ep->did, ep->sid,
>>>> + FC_TYPE_ELS, f_ctl, 0);
>>>> + lport->tt.seq_send(lport, sp, fp);
>>>> + goto out;
>>>> +out_rjt:
>>>> + rjt_data.fp = NULL;
>>>> + lport->tt.seq_els_rsp_send(sp, ELS_LS_RJT, &rjt_data);
>>>> +out:
>>>> + fc_frame_free(in_fp);
>>>> +}
>>>> +
>>>> +/**
>>>> * fc_fabric_login() - Start the lport state machine
>>>> * @lport: The local port that should log into the fabric
>>>> *
>>>> @@ -898,6 +1019,9 @@ static void fc_lport_recv_req(struct fc_lport *lport,
>>> struct fc_seq *sp,
>>>> case ELS_RNID:
>>>> recv = fc_lport_recv_rnid_req;
>>>> break;
>>>> + case ELS_RLS:
>>>> + recv = fc_lport_recv_rls_req;
>>>> + break;
>>> This case should be added to fc_rport_recv_req() instead.
>>>
>>>> }
>>>>
>>>> recv(sp, fp, lport);
>>>>
>>>> _______________________________________________
>>>> devel mailing list
>>>> [email protected]
>>>> http://www.open-fcoe.org/mailman/listinfo/devel
>>
Thanks for the review, I will update my patches to include your
comments and resubmit.
yi
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
