Open-FCoE Submission (round 2)
Subsystems (3)
cover Open-FCoE Submission (round 2)
The following series adds the following code to the SCSI subsystem.
1) FC and FCoE protocol definitions in header files
- Files are added in include/scsi/fc/
- New files are: fc_els.h, fc_encaps.h, fc_fc2.h, fc_fcp.h, fc_fs.h, fc_gs.h and fc_ns.h
2) libfc - a FC assist library
- Composed of 5 sub-components: Exchange Manager, Local Port Remote Port, Discovery and SCSI-FCP
- Uses libfc_function_template for cross-layer calls
- Uses struct fc_lport as primary object shared by all sub-modules
- Common code defined in include/scsi/libfc.h
3) fcoe - a FCoE initiator implementation
- Simple encapsulation and unpacking layer
- Interacts with netdev
- Initial support for FCoE offload
- Uses /sys/module/fcoe/parameters/create and destroy for enabling/disabling FCoE connection over a particular Ethernet adapter
- Works on any Ethernet adapter
These patches apply to both the scsi-misc and scsi-rc-fixes trees.
I beleive that we've addressed each of Andi's comments. The largest being a security review where we mostly focused on input validation from the wire as well as removing BUG_ON and WARN_ON statements that could be initiated from an external entity. For a detailed list of recent activity please view our git log on Open-FCoE. Here is a link to our gitweb site:
http://www.open-fcoe.org/openfc/gitweb/?p=openfc/open-fcoe-initiator.git;a=summary
We did not break the patch-set down any further as it would not be bisect-able. Our libfc blocks are organized by file, so using splitdiff or diffsplit anyone should be able to break these patches down into smaller, more review friendly chunks.
Locking comments are mostly in comment blocks at the tops of some files. The largest comment block is in drivers/scsi/libfc/fc_lport.c and would be a good starting point when reviewing the locking.
---
Robert Love (3):
fcoe: Fibre Channel over Ethernet
libfc: A modular Fibre Channel library
FC protocol definition header files
drivers/scsi/Kconfig | 13 drivers/scsi/Makefile | 2 drivers/scsi/fcoe/Makefile | 8 drivers/scsi/fcoe/fc_transport_fcoe.c | 446 +++++++ drivers/scsi/fcoe/fcoe_sw.c | 494 ++++++++ drivers/scsi/fcoe/libfcoe.c | 1510 +++++++++++++++++++++++ drivers/scsi/libfc/Makefile | 12 drivers/scsi/libfc/fc_disc.c | 845 +++++++++++++ drivers/scsi/libfc/fc_elsct.c | 71 + drivers/scsi/libfc/fc_exch.c | 1970 +++++++++++++++++++++++++++++++ drivers/scsi/libfc/fc_fcp.c | 2130 +++++++++++++++++++++++++++++++++ drivers/scsi/libfc/fc_frame.c | 89 + drivers/scsi/libfc/fc_lport.c | 1604 +++++++++++++++++++++++++ drivers/scsi/libfc/fc_rport.c | 1291 ++++++++++++++++++++ include/scsi/fc/fc_els.h | 816 +++++++++++++ include/scsi/fc/fc_encaps.h | 138 ++ include/scsi/fc/fc_fc2.h | 124 ++ include/scsi/fc/fc_fcoe.h | 114 ++ include/scsi/fc/fc_fcp.h | 199 +++ include/scsi/fc/fc_fs.h | 340 +++++ include/scsi/fc/fc_gs.h | 93 + include/scsi/fc/fc_ns.h | 159 ++ include/scsi/fc_encode.h | 309 +++++ include/scsi/fc_frame.h | 242 ++++ include/scsi/fc_transport_fcoe.h | 54 + include/scsi/libfc.h | 938 +++++++++++++++ include/scsi/libfcoe.h | 176 +++ 27 files changed, 14187 insertions(+), 0 deletions(-) create mode 100644 drivers/scsi/fcoe/Makefile create mode 100644 drivers/scsi/fcoe/fc_transport_fcoe.c create mode 100644 drivers/scsi/fcoe/fcoe_sw.c create mode 100644 drivers/scsi/fcoe/libfcoe.c create mode 100644 drivers/scsi/libfc/Makefile create mode 100644 drivers/scsi/libfc/fc_disc.c create mode 100644 drivers/scsi/libfc/fc_elsct.c create mode 100644 drivers/scsi/libfc/fc_exch.c create mode 100644 drivers/scsi/libfc/fc_fcp.c create mode 100644 drivers/scsi/libfc/fc_frame.c create mode 100644 drivers/scsi/libfc/fc_lport.c create mode 100644 drivers/scsi/libfc/fc_rport.c create mode 100644 include/scsi/fc/fc_els.h create mode 100644 include/scsi/fc/fc_encaps.h create mode 100644 include/scsi/fc/fc_fc2.h create mode 100644 include/scsi/fc/fc_fcoe.h create mode 100644 include/scsi/fc/fc_fcp.h create mode 100644 include/scsi/fc/fc_fs.h create mode 100644 include/scsi/fc/fc_gs.h create mode 100644 include/scsi/fc/fc_ns.h create mode 100644 include/scsi/fc_encode.h create mode 100644 include/scsi/fc_frame.h create mode 100644 include/scsi/fc_transport_fcoe.h create mode 100644 include/scsi/libfc.h create mode 100644 include/scsi/libfcoe.h
Signature · 1 line
//Rob
PATCH 1/3 FC protocol definition header files
---
include/scsi/fc/fc_els.h | 816 +++++++++++++++++++++++++++++++++++++++++++ include/scsi/fc/fc_encaps.h | 138 +++++++ include/scsi/fc/fc_fc2.h | 124 +++++++ include/scsi/fc/fc_fcoe.h | 114 ++++++ include/scsi/fc/fc_fcp.h | 199 ++++++++++ include/scsi/fc/fc_fs.h | 340 ++++++++++++++++++ include/scsi/fc/fc_gs.h | 93 +++++ include/scsi/fc/fc_ns.h | 159 ++++++++ 8 files changed, 1983 insertions(+), 0 deletions(-) create mode 100644 include/scsi/fc/fc_els.h create mode 100644 include/scsi/fc/fc_encaps.h create mode 100644 include/scsi/fc/fc_fc2.h create mode 100644 include/scsi/fc/fc_fcoe.h create mode 100644 include/scsi/fc/fc_fcp.h create mode 100644 include/scsi/fc/fc_fs.h create mode 100644 include/scsi/fc/fc_gs.h create mode 100644 include/scsi/fc/fc_ns.h
include/scsi/fc/fc_els.h new file +816 -0
@@ -0,0 +1,816 @@ +/* +* Copyright(c) 2007 Intel Corporation. All rights reserved. +* +* This program is free software; you can redistribute it and/or modify it +* under the terms and conditions of the GNU General Public License, +* version 2, as published by the Free Software Foundation. +* +* This program is distributed in the hope it will be useful, but WITHOUT +* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +* more details. +* +* You should have received a copy of the GNU General Public License along with +* this program; if not, write to the Free Software Foundation, Inc., +* 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA. +* +* Maintained at www.Open-FCoE.org +*/ + +#ifndef _FC_ELS_H_ +#define _FC_ELS_H_ + +/* +* Fibre Channel Switch - Enhanced Link Services definitions. +* From T11 FC-LS Rev 1.2 June 7, 2005. +*/ + +/* +* ELS Command codes - byte 0 of the frame payload +*/ +enum fc_els_cmd { +ELS_LS_RJT =0x01 ,/* ESL reject */ +ELS_LS_ACC =0x02 ,/* ESL Accept */ +ELS_PLOGI =0x03 ,/* N_Port login */ +ELS_FLOGI =0x04 ,/* F_Port login */ +ELS_LOGO =0x05 ,/* Logout */ +ELS_ABTX =0x06 ,/* Abort exchange - obsolete */ +ELS_RCS =0x07 ,/* read connection status */ +ELS_RES =0x08 ,/* read exchange status block */ +ELS_RSS =0x09 ,/* read sequence status block */ +ELS_RSI =0x0a ,/* read sequence initiative */ +ELS_ESTS =0x0b ,/* establish streaming */ +ELS_ESTC =0x0c ,/* estimate credit */ +ELS_ADVC =0x0d ,/* advise credit */ +ELS_RTV =0x0e ,/* read timeout value */ +ELS_RLS =0x0f ,/* read link error status block */ +ELS_ECHO =0x10 ,/* echo */ +ELS_TEST =0x11 ,/* test */ +ELS_RRQ =0x12 ,/* reinstate recovery qualifier */ +ELS_REC =0x13 ,/* read exchange concise */ +ELS_SRR =0x14 ,/* sequence retransmission request */ +ELS_PRLI =0x20 ,/* process login */ +ELS_PRLO =0x21 ,/* process logout */ +ELS_SCN =0x22 ,/* state change notification */ +ELS_TPLS =0x23 ,/* test process login state */ +ELS_TPRLO =0x24 ,/* third party process logout */ +ELS_LCLM =0x25 ,/* login control list mgmt (obs) */ +ELS_GAID =0x30 ,/* get alias_ID */ +ELS_FACT =0x31 ,/* fabric activate alias_id */ +ELS_FDACDT =0x32 ,/* fabric deactivate alias_id */ +ELS_NACT =0x33 ,/* N-port activate alias_id */ +ELS_NDACT =0x34 ,/* N-port deactivate alias_id */ +ELS_QOSR =0x40 ,/* quality of service request */ +ELS_RVCS =0x41 ,/* read virtual circuit status */ +ELS_PDISC =0x50 ,/* discover N_port service params */ +ELS_FDISC =0x51 ,/* discover F_port service params */ +ELS_ADISC =0x52 ,/* discover address */ +ELS_RNC =0x53 ,/* report node cap (obs) */ +ELS_FARP_REQ =0x54 ,/* FC ARP request */ +ELS_FARP_REPL =0x55 ,/* FC ARP reply */ +ELS_RPS =0x56 ,/* read port status block */ +ELS_RPL =0x57 ,/* read port list */ +ELS_RPBC =0x58 ,/* read port buffer condition */ +ELS_FAN =0x60 ,/* fabric address notification */ +ELS_RSCN =0x61 ,/* registered state change notification */ +ELS_SCR =0x62 ,/* state change registration */ +ELS_RNFT =0x63 ,/* report node FC-4 types */ +ELS_CSR =0x68 ,/* clock synch. request */ +ELS_CSU =0x69 ,/* clock synch. update */ +ELS_LINIT =0x70 ,/* loop initialize */ +ELS_LSTS =0x72 ,/* loop status */ +ELS_RNID =0x78 ,/* request node ID data */ +ELS_RLIR =0x79 ,/* registered link incident report */ +ELS_LIRR =0x7a ,/* link incident record registration */ +ELS_SRL =0x7b ,/* scan remote loop */ +ELS_SBRP =0x7c ,/* set bit-error reporting params */ +ELS_RPSC =0x7d ,/* report speed capabilities */ +ELS_QSA =0x7e ,/* query security attributes */ +ELS_EVFP =0x7f ,/* exchange virt. fabrics params */ +ELS_LKA =0x80 ,/* link keep-alive */ +ELS_AUTH_ELS =0x90 ,/* authentication ELS */ +}; + +/* +* Initializer useful for decoding table. +* Please keep this in sync with the above definitions. +*/ +#define FC_ELS_CMDS_INIT { \ + [ELS_LS_RJT] = "LS_RJT", \ + [ELS_LS_ACC] = "LS_ACC", \ + [ELS_PLOGI] = "PLOGI", \ + [ELS_FLOGI] = "FLOGI", \ + [ELS_LOGO] = "LOGO", \ + [ELS_ABTX] = "ABTX", \ + [ELS_RCS] = "RCS", \ + [ELS_RES] = "RES", \ + [ELS_RSS] = "RSS", \ + [ELS_RSI] = "RSI", \ + [ELS_ESTS] = "ESTS", \ + [ELS_ESTC] = "ESTC", \ + [ELS_ADVC] = "ADVC", \ + [ELS_RTV] = "RTV", \ + [ELS_RLS] = "RLS", \ + [ELS_ECHO] = "ECHO", \ + [ELS_TEST] = "TEST", \ + [ELS_RRQ] = "RRQ", \ + [ELS_REC] = "REC", \ + [ELS_SRR] = "SRR", \ + [ELS_PRLI] = "PRLI", \ + [ELS_PRLO] = "PRLO", \ + [ELS_SCN] = "SCN", \ + [ELS_TPLS] = "TPLS", \ + [ELS_TPRLO] = "TPRLO", \ + [ELS_LCLM] = "LCLM", \ + [ELS_GAID] = "GAID", \ + [ELS_FACT] = "FACT", \ + [ELS_FDACDT] = "FDACDT", \ + [ELS_NACT] = "NACT", \ + [ELS_NDACT] = "NDACT", \ + [ELS_QOSR] = "QOSR", \ + [ELS_RVCS] = "RVCS", \ + [ELS_PDISC] = "PDISC", \ + [ELS_FDISC] = "FDISC", \ + [ELS_ADISC] = "ADISC", \ + [ELS_RNC] = "RNC", \ + [ELS_FARP_REQ] = "FARP_REQ", \ + [ELS_FARP_REPL] = "FARP_REPL", \ + [ELS_RPS] = "RPS", \ + [ELS_RPL] = "RPL", \ + [ELS_RPBC] = "RPBC", \ + [ELS_FAN] = "FAN", \ + [ELS_RSCN] = "RSCN", \ + [ELS_SCR] = "SCR", \ + [ELS_RNFT] = "RNFT", \ + [ELS_CSR] = "CSR", \ + [ELS_CSU] = "CSU", \ + [ELS_LINIT] = "LINIT", \ + [ELS_LSTS] = "LSTS", \ + [ELS_RNID] = "RNID", \ + [ELS_RLIR] = "RLIR", \ + [ELS_LIRR] = "LIRR", \ + [ELS_SRL] = "SRL", \ + [ELS_SBRP] = "SBRP", \ + [ELS_RPSC] = "RPSC", \ + [ELS_QSA] = "QSA", \ + [ELS_EVFP] = "EVFP", \ + [ELS_LKA] = "LKA", \ + [ELS_AUTH_ELS] = "AUTH_ELS", \ +} + +/* +* LS_ACC payload. +*/ +struct fc_els_ls_acc { +__u8 la_cmd;/* command code ELS_LS_ACC */ +__u8 la_resv[3 ];/* reserved */ +}; + +/* +* ELS reject payload. +*/ +struct fc_els_ls_rjt { +__u8 er_cmd;/* command code ELS_LS_RJT */ +__u8 er_resv[4 ];/* reserved must be zero */ +__u8 er_reason;/* reason (enum fc_els_rjt_reason below) */ +__u8 er_explan;/* explanation (enum fc_els_rjt_explan below) */ +__u8 er_vendor;/* vendor specific code */ +}; + +/* +* ELS reject reason codes (er_reason). +*/ +enum fc_els_rjt_reason { +ELS_RJT_NONE =0 ,/* no reject - not to be sent */ +ELS_RJT_INVAL =0x01 ,/* invalid ELS command code */ +ELS_RJT_LOGIC =0x03 ,/* logical error */ +ELS_RJT_BUSY =0x05 ,/* logical busy */ +ELS_RJT_PROT =0x07 ,/* protocol error */ +ELS_RJT_UNAB =0x09 ,/* unable to perform command request */ +ELS_RJT_UNSUP =0x0b ,/* command not supported */ +ELS_RJT_INPROG =0x0e ,/* command already in progress */ +ELS_RJT_VENDOR =0xff ,/* vendor specific error */ +}; + + +/* +* reason code explanation (er_explan). +*/ +enum fc_els_rjt_explan { +ELS_EXPL_NONE =0x00 ,/* No additional explanation */ +ELS_EXPL_SPP_OPT_ERR =0x01 ,/* service parameter error - options */ +ELS_EXPL_SPP_ICTL_ERR =0x03 ,/* service parm error - initiator ctl */ +ELS_EXPL_AH =0x11 ,/* invalid association header */ +ELS_EXPL_AH_REQ =0x13 ,/* association_header required */ +ELS_EXPL_SID =0x15 ,/* invalid originator S_ID */ +ELS_EXPL_OXID_RXID =0x17 ,/* invalid OX_ID-RX_ID combination */ +ELS_EXPL_INPROG =0x19 ,/* Request already in progress */ +ELS_EXPL_PLOGI_REQD =0x1e ,/* N_Port login required */ +ELS_EXPL_INSUF_RES =0x29 ,/* insufficient resources */ +ELS_EXPL_UNAB_DATA =0x2a ,/* unable to supply requested data */ +ELS_EXPL_UNSUPR =0x2c ,/* Request not supported */ +ELS_EXPL_INV_LEN =0x2d ,/* Invalid payload length */ +/* TBD - above definitions incomplete */ +}; + +/* +* Common service parameters (N ports). +*/ +struct fc_els_csp { +__u8 sp_hi_ver;/* highest version supported (obs.) */ +__u8 sp_lo_ver;/* highest version supported (obs.) */ +__be16 sp_bb_cred;/* buffer-to-buffer credits */ +__be16 sp_features;/* common feature flags */ +__be16 sp_bb_data;/* b-b state number and data field sz */ +union { +struct { +__be16 _sp_tot_seq;/* total concurrent sequences */ +__be16 _sp_rel_off;/* rel. offset by info cat */ + } sp_plogi; +struct { +__be32 _sp_r_a_tov;/* resource alloc. timeout msec */ + } sp_flogi_acc; + } sp_u; +__be32 sp_e_d_tov;/* error detect timeout value */ +}; +#define sp_tot_seq sp_u.sp_plogi._sp_tot_seq +#define sp_rel_off sp_u.sp_plogi._sp_rel_off +#define sp_r_a_tov sp_u.sp_flogi_acc._sp_r_a_tov + +#define FC_SP_BB_DATA_MASK 0xfff/* mask for data field size in sp_bb_data */ + +/* +* Minimum and maximum values for max data field size in service parameters. +*/ +#define FC_SP_MIN_MAX_PAYLOAD FC_MIN_MAX_PAYLOAD +#define FC_SP_MAX_MAX_PAYLOAD FC_MAX_PAYLOAD + +/* +* sp_features +*/ +#define FC_SP_FT_CIRO 0x8000/* continuously increasing rel. off. */ +#define FC_SP_FT_CLAD 0x8000/* clean address (in FLOGI LS_ACC) */ +#define FC_SP_FT_RAND 0x4000/* random relative offset */ +#define FC_SP_FT_VAL 0x2000/* valid vendor version level */ +#define FC_SP_FT_FPORT 0x1000/* F port (1) vs. N port (0) */ +#define FC_SP_FT_ABB 0x0800/* alternate BB_credit management */ +#define FC_SP_FT_EDTR 0x0400/* E_D_TOV Resolution is nanoseconds */ +#define FC_SP_FT_MCAST 0x0200/* multicast */ +#define FC_SP_FT_BCAST 0x0100/* broadcast */ +#define FC_SP_FT_HUNT 0x0080/* hunt group */ +#define FC_SP_FT_SIMP 0x0040/* dedicated simplex */ +#define FC_SP_FT_SEC 0x0020/* reserved for security */ +#define FC_SP_FT_CSYN 0x0010/* clock synch. supported */ +#define FC_SP_FT_RTTOV 0x0008/* R_T_TOV value 100 uS, else 100 mS */ +#define FC_SP_FT_HALF 0x0004/* dynamic half duplex */ +#define FC_SP_FT_SEQC 0x0002/* SEQ_CNT */ +#define FC_SP_FT_PAYL 0x0001/* FLOGI payload length 256, else 116 */ + +/* +* Class-specific service parameters. +*/ +struct fc_els_cssp { +__be16 cp_class;/* class flags */ +__be16 cp_init;/* initiator flags */ +__be16 cp_recip;/* recipient flags */ +__be16 cp_rdfs;/* receive data field size */ +__be16 cp_con_seq;/* concurrent sequences */ +__be16 cp_ee_cred;/* N-port end-to-end credit */ +__u8 cp_resv1;/* reserved */ +__u8 cp_open_seq;/* open sequences per exchange */ +__u8 _cp_resv2[2 ];/* reserved */ +}; + +/* +* cp_class flags. +*/ +#define FC_CPC_VALID 0x8000/* class valid */ +#define FC_CPC_IMIX 0x4000/* intermix mode */ +#define FC_CPC_SEQ 0x0800/* sequential delivery */ +#define FC_CPC_CAMP 0x0200/* camp-on */ +#define FC_CPC_PRI 0x0080/* priority */ + +/* +* cp_init flags. +* (TBD: not all flags defined here). +*/ +#define FC_CPI_CSYN 0x0010/* clock synch. capable */ + +/* +* cp_recip flags. +*/ +#define FC_CPR_CSYN 0x0008/* clock synch. capable */ + +/* +* NFC_ELS_FLOGI: Fabric login request. +* NFC_ELS_PLOGI: Port login request (same format). +*/ +struct fc_els_flogi { +__u8 fl_cmd;/* command */ +__u8 _fl_resvd[3 ];/* must be zero */ +struct fc_els_csp fl_csp;/* common service parameters */ +__be64 fl_wwpn;/* port name */ +__be64 fl_wwnn;/* node name */ +struct fc_els_cssp fl_cssp[4 ];/* class 1-4 service parameters */ +__u8 fl_vend[16 ];/* vendor version level */ +} __attribute__((__packed__ )); + +/* +* Process login service parameter page. +*/ +struct fc_els_spp { +__u8 spp_type;/* type code or common service params */ +__u8 spp_type_ext;/* type code extension */ +__u8 spp_flags; +__u8 _spp_resvd; +__be32 spp_orig_pa;/* originator process associator */ +__be32 spp_resp_pa;/* responder process associator */ +__be32 spp_params;/* service parameters */ +}; + +/* +* spp_flags. +*/ +#define FC_SPP_OPA_VAL 0x80/* originator proc. assoc. valid */ +#define FC_SPP_RPA_VAL 0x40/* responder proc. assoc. valid */ +#define FC_SPP_EST_IMG_PAIR 0x20/* establish image pair */ +#define FC_SPP_RESP_MASK 0x0f/* mask for response code (below) */ + +/* +* SPP response code in spp_flags - lower 4 bits. +*/ +enum fc_els_spp_resp { +FC_SPP_RESP_ACK =1 ,/* request executed */ +FC_SPP_RESP_RES =2 ,/* unable due to lack of resources */ +FC_SPP_RESP_INIT =3 ,/* initialization not complete */ +FC_SPP_RESP_NO_PA =4 ,/* unknown process associator */ +FC_SPP_RESP_CONF =5 ,/* configuration precludes image pair */ +FC_SPP_RESP_COND =6 ,/* request completed conditionally */ +FC_SPP_RESP_MULT =7 ,/* unable to handle multiple SPPs */ +FC_SPP_RESP_INVL =8 ,/* SPP is invalid */ +}; + +/* +* ELS_RRQ - Reinstate Recovery Qualifier +*/ +struct fc_els_rrq { +__u8 rrq_cmd;/* command (0x12) */ +__u8 rrq_zero[3 ];/* specified as zero - part of cmd */ +__u8 rrq_resvd;/* reserved */ +__u8 rrq_s_id[3 ];/* originator FID */ +__be16 rrq_ox_id;/* originator exchange ID */ +__be16 rrq_rx_id;/* responders exchange ID */ +}; + +/* +* ELS_REC - Read exchange concise. +*/ +struct fc_els_rec { +__u8 rec_cmd;/* command (0x13) */ +__u8 rec_zero[3 ];/* specified as zero - part of cmd */ +__u8 rec_resvd;/* reserved */ +__u8 rec_s_id[3 ];/* originator FID */ +__be16 rec_ox_id;/* originator exchange ID */ +__be16 rec_rx_id;/* responders exchange ID */ +}; + +/* +* ELS_REC LS_ACC payload. +*/ +struct fc_els_rec_acc { +__u8 reca_cmd;/* accept (0x02) */ +__u8 reca_zero[3 ];/* specified as zero - part of cmd */ +__be16 reca_ox_id;/* originator exchange ID */ +__be16 reca_rx_id;/* responders exchange ID */ +__u8 reca_resvd1;/* reserved */ +__u8 reca_ofid[3 ];/* originator FID */ +__u8 reca_resvd2;/* reserved */ +__u8 reca_rfid[3 ];/* responder FID */ +__be32 reca_fc4value;/* FC4 value */ +__be32 reca_e_stat;/* ESB (exchange status block) status */ +}; + +/* +* ELS_PRLI - Process login request and response. +*/ +struct fc_els_prli { +__u8 prli_cmd;/* command */ +__u8 prli_spp_len;/* length of each serv. parm. page */ +__be16 prli_len;/* length of entire payload */ +/* service parameter pages follow */ +}; + +/* +* ELS_ADISC payload +*/ +struct fc_els_adisc { +__u8 adisc_cmd; +__u8 adisc_resv[3 ]; +__u8 adisc_resv1; +__u8 adisc_hard_addr[3 ]; +__be64 adisc_wwpn; +__be64 adisc_wwnn; +__u8 adisc_resv2; +__u8 adisc_port_id[3 ]; +} __attribute__((__packed__ )); + +/* +* ELS_LOGO - process or fabric logout. +*/ +struct fc_els_logo { +__u8 fl_cmd;/* command code */ +__u8 fl_zero[3 ];/* specified as zero - part of cmd */ +__u8 fl_resvd;/* reserved */ +__u8 fl_n_port_id[3 ];/* N port ID */ +__be64 fl_n_port_wwn;/* port name */ +}; + +/* +* ELS_RTV - read timeout value. +*/ +struct fc_els_rtv { +__u8 rtv_cmd;/* command code 0x0e */ +__u8 rtv_zero[3 ];/* specified as zero - part of cmd */ +}; + +/* +* LS_ACC for ELS_RTV - read timeout value. +*/ +struct fc_els_rtv_acc { +__u8 rtv_cmd;/* command code 0x02 */ +__u8 rtv_zero[3 ];/* specified as zero - part of cmd */ +__be32 rtv_r_a_tov;/* resource allocation timeout value */ +__be32 rtv_e_d_tov;/* error detection timeout value */ +__be32 rtv_toq;/* timeout qualifier (see below) */ +}; + +/* +* rtv_toq bits. +*/ +#define FC_ELS_RTV_EDRES (1 << 26)/* E_D_TOV resolution is nS else mS */ +#define FC_ELS_RTV_RTTOV (1 << 19)/* R_T_TOV is 100 uS else 100 mS */ + +/* +* ELS_SCR - state change registration payload. +*/ +struct fc_els_scr { +__u8 scr_cmd;/* command code */ +__u8 scr_resv[6 ];/* reserved */ +__u8 scr_reg_func;/* registration function (see below) */ +}; + +enum fc_els_scr_func { +ELS_SCRF_FAB =1 ,/* fabric-detected registration */ +ELS_SCRF_NPORT =2 ,/* Nx_Port-detected registration */ +ELS_SCRF_FULL =3 ,/* full registration */ +ELS_SCRF_CLEAR =255 ,/* remove any current registrations */ +}; + +/* +* ELS_RSCN - registered state change notification payload. +*/ +struct fc_els_rscn { +__u8 rscn_cmd;/* RSCN opcode (0x61) */ +__u8 rscn_page_len;/* page length (4) */ +__be16 rscn_plen;/* payload length including this word */ + +/* followed by 4-byte generic affected Port_ID pages */ +}; + +struct fc_els_rscn_page { +__u8 rscn_page_flags;/* event and address format */ +__u8 rscn_fid[3 ];/* fabric ID */ +}; + +#define ELS_RSCN_EV_QUAL_BIT 2/* shift count for event qualifier */ +#define ELS_RSCN_EV_QUAL_MASK 0xf/* mask for event qualifier */ +#define ELS_RSCN_ADDR_FMT_BIT 0/* shift count for address format */ +#define ELS_RSCN_ADDR_FMT_MASK 0x3/* mask for address format */ + +enum fc_els_rscn_ev_qual { +ELS_EV_QUAL_NONE =0 ,/* unspecified */ +ELS_EV_QUAL_NS_OBJ =1 ,/* changed name server object */ +ELS_EV_QUAL_PORT_ATTR =2 ,/* changed port attribute */ +ELS_EV_QUAL_SERV_OBJ =3 ,/* changed service object */ +ELS_EV_QUAL_SW_CONFIG =4 ,/* changed switch configuration */ +ELS_EV_QUAL_REM_OBJ =5 ,/* removed object */ +}; + +enum fc_els_rscn_addr_fmt { +ELS_ADDR_FMT_PORT =0 ,/* rscn_fid is a port address */ +ELS_ADDR_FMT_AREA =1 ,/* rscn_fid is a area address */ +ELS_ADDR_FMT_DOM =2 ,/* rscn_fid is a domain address */ +ELS_ADDR_FMT_FAB =3 ,/* anything on fabric may have changed */ +}; + +/* +* ELS_RNID - request Node ID. +*/ +struct fc_els_rnid { +__u8 rnid_cmd;/* RNID opcode (0x78) */ +__u8 rnid_resv[3 ];/* reserved */ +__u8 rnid_fmt;/* data format */ +__u8 rnid_resv2[3 ];/* reserved */ +}; + +/* +* Node Identification Data formats (rnid_fmt) +*/ +enum fc_els_rnid_fmt { +ELS_RNIDF_NONE =0 ,/* no specific identification data */ +ELS_RNIDF_GEN =0xdf ,/* general topology discovery format */ +}; + +/* +* ELS_RNID response. +*/ +struct fc_els_rnid_resp { +__u8 rnid_cmd;/* response code (LS_ACC) */ +__u8 rnid_resv[3 ];/* reserved */ +__u8 rnid_fmt;/* data format */ +__u8 rnid_cid_len;/* common ID data length */ +__u8 rnid_resv2;/* reserved */ +__u8 rnid_sid_len;/* specific ID data length */ +}; + +struct fc_els_rnid_cid { +__be64 rnid_wwpn;/* N port name */ +__be64 rnid_wwnn;/* node name */ +}; + +struct fc_els_rnid_gen { +__u8 rnid_vend_id[16 ];/* vendor-unique ID */ +__be32 rnid_atype;/* associated type (see below) */ +__be32 rnid_phys_port;/* physical port number */ +__be32 rnid_att_nodes;/* number of attached nodes */ +__u8 rnid_node_mgmt;/* node management (see below) */ +__u8 rnid_ip_ver;/* IP version (see below) */ +__be16 rnid_prot_port;/* UDP / TCP port number */ +__be32 rnid_ip_addr[4 ];/* IP address */ +__u8 rnid_resvd[2 ];/* reserved */ +__be16 rnid_vend_spec;/* vendor-specific field */ +}; + +enum fc_els_rnid_atype { +ELS_RNIDA_UNK =0x01 ,/* unknown */ +ELS_RNIDA_OTHER =0x02 ,/* none of the following */ +ELS_RNIDA_HUB =0x03 , +ELS_RNIDA_SWITCH =0x04 , +ELS_RNIDA_GATEWAY =0x05 , +ELS_RNIDA_CONV =0x06 ,/* Obsolete, do not use this value */ +ELS_RNIDA_HBA =0x07 ,/* Obsolete, do not use this value */ +ELS_RNIDA_PROXY =0x08 ,/* Obsolete, do not use this value */ +ELS_RNIDA_STORAGE =0x09 , +ELS_RNIDA_HOST =0x0a , +ELS_RNIDA_SUBSYS =0x0b ,/* storage subsystem (e.g., RAID) */ +ELS_RNIDA_ACCESS =0x0e ,/* access device (e.g. media changer) */ +ELS_RNIDA_NAS =0x11 ,/* NAS server */ +ELS_RNIDA_BRIDGE =0x12 ,/* bridge */ +ELS_RNIDA_VIRT =0x13 ,/* virtualization device */ +ELS_RNIDA_MF =0xff ,/* multifunction device (bits below) */ +ELS_RNIDA_MF_HUB =1UL <<31 ,/* hub */ +ELS_RNIDA_MF_SW =1UL <<30 ,/* switch */ +ELS_RNIDA_MF_GW =1UL <<29 ,/* gateway */ +ELS_RNIDA_MF_ST =1UL <<28 ,/* storage */ +ELS_RNIDA_MF_HOST =1UL <<27 ,/* host */ +ELS_RNIDA_MF_SUB =1UL <<26 ,/* storage subsystem */ +ELS_RNIDA_MF_ACC =1UL <<25 ,/* storage access dev */ +ELS_RNIDA_MF_WDM =1UL <<24 ,/* wavelength division mux */ +ELS_RNIDA_MF_NAS =1UL <<23 ,/* NAS server */ +ELS_RNIDA_MF_BR =1UL <<22 ,/* bridge */ +ELS_RNIDA_MF_VIRT =1UL <<21 ,/* virtualization device */ +}; + +enum fc_els_rnid_mgmt { +ELS_RNIDM_SNMP =0 , +ELS_RNIDM_TELNET =1 , +ELS_RNIDM_HTTP =2 , +ELS_RNIDM_HTTPS =3 , +ELS_RNIDM_XML =4 ,/* HTTP + XML */ +}; + +enum fc_els_rnid_ipver { +ELS_RNIDIP_NONE =0 ,/* no IP support or node mgmt. */ +ELS_RNIDIP_V4 =1 ,/* IPv4 */ +ELS_RNIDIP_V6 =2 ,/* IPv6 */ +}; + +/* +* ELS RPL - Read Port List. +*/ +struct fc_els_rpl { +__u8 rpl_cmd;/* command */ +__u8 rpl_resv[5 ];/* reserved - must be zero */ +__be16 rpl_max_size;/* maximum response size or zero */ +__u8 rpl_resv1;/* reserved - must be zero */ +__u8 rpl_index[3 ];/* starting index */ +}; + +/* +* Port number block in RPL response. +*/ +struct fc_els_pnb { +__be32 pnb_phys_pn;/* physical port number */ +__u8 pnb_resv;/* reserved */ +__u8 pnb_port_id[3 ];/* port ID */ +__be64 pnb_wwpn;/* port name */ +}; + +/* +* RPL LS_ACC response. +*/ +struct fc_els_rpl_resp { +__u8 rpl_cmd;/* ELS_LS_ACC */ +__u8 rpl_resv1;/* reserved - must be zero */ +__be16 rpl_plen;/* payload length */ +__u8 rpl_resv2;/* reserved - must be zero */ +__u8 rpl_llen[3 ];/* list length */ +__u8 rpl_resv3;/* reserved - must be zero */ +__u8 rpl_index[3 ];/* starting index */ +struct fc_els_pnb rpl_pnb[1 ];/* variable number of PNBs */ +}; + +/* +* Link Error Status Block. +*/ +struct fc_els_lesb { +__be32 lesb_link_fail;/* link failure count */ +__be32 lesb_sync_loss;/* loss of synchronization count */ +__be32 lesb_sig_loss;/* loss of signal count */ +__be32 lesb_prim_err;/* primitive sequence error count */ +__be32 lesb_inv_word;/* invalid transmission word count */ +__be32 lesb_inv_crc;/* invalid CRC count */ +}; + +/* +* ELS RPS - Read Port Status Block request. +*/ +struct fc_els_rps { +__u8 rps_cmd;/* command */ +__u8 rps_resv[2 ];/* reserved - must be zero */ +__u8 rps_flag;/* flag - see below */ +__be64 rps_port_spec;/* port selection */ +}; + +enum fc_els_rps_flag { +FC_ELS_RPS_DID =0x00 ,/* port identified by D_ID of req. */ +FC_ELS_RPS_PPN =0x01 ,/* port_spec is physical port number */ +FC_ELS_RPS_WWPN =0x02 ,/* port_spec is port WWN */ +}; + +/* +* ELS RPS LS_ACC response. +*/ +struct fc_els_rps_resp { +__u8 rps_cmd;/* command - LS_ACC */ +__u8 rps_resv[2 ];/* reserved - must be zero */ +__u8 rps_flag;/* flag - see below */ +__u8 rps_resv2[2 ];/* reserved */ +__be16 rps_status;/* port status - see below */ +struct fc_els_lesb rps_lesb;/* link error status block */ +}; + +enum fc_els_rps_resp_flag { +FC_ELS_RPS_LPEV =0x01 ,/* L_port extension valid */ +}; + +enum fc_els_rps_resp_status { +FC_ELS_RPS_PTP =1 <<5 ,/* point-to-point connection */ +FC_ELS_RPS_LOOP =1 <<4 ,/* loop mode */ +FC_ELS_RPS_FAB =1 <<3 ,/* fabric present */ +FC_ELS_RPS_NO_SIG =1 <<2 ,/* loss of signal */ +FC_ELS_RPS_NO_SYNC =1 <<1 ,/* loss of synchronization */ +FC_ELS_RPS_RESET =1 <<0 ,/* in link reset protocol */ +}; + +/* +* ELS LIRR - Link Incident Record Registration request. +*/ +struct fc_els_lirr { +__u8 lirr_cmd;/* command */ +__u8 lirr_resv[3 ];/* reserved - must be zero */ +__u8 lirr_func;/* registration function */ +__u8 lirr_fmt;/* FC-4 type of RLIR requested */ +__u8 lirr_resv2[2 ];/* reserved - must be zero */ +}; + +enum fc_els_lirr_func { +ELS_LIRR_SET_COND =0x01 ,/* set - conditionally receive */ +ELS_LIRR_SET_UNCOND =0x02 ,/* set - unconditionally receive */ +ELS_LIRR_CLEAR =0xff /* clear registration */ +}; + +/* +* ELS SRL - Scan Remote Loop request. +*/ +struct fc_els_srl { +__u8 srl_cmd;/* command */ +__u8 srl_resv[3 ];/* reserved - must be zero */ +__u8 srl_flag;/* flag - see below */ +__u8 srl_flag_param[3 ];/* flag parameter */ +}; + +enum fc_els_srl_flag { +FC_ELS_SRL_ALL =0x00 ,/* scan all FL ports */ +FC_ELS_SRL_ONE =0x01 ,/* scan specified loop */ +FC_ELS_SRL_EN_PER =0x02 ,/* enable periodic scanning (param) */ +FC_ELS_SRL_DIS_PER =0x03 ,/* disable periodic scanning */ +}; + +/* +* ELS RLS - Read Link Error Status Block request. +*/ +struct fc_els_rls { +__u8 rls_cmd;/* command */ +__u8 rls_resv[4 ];/* reserved - must be zero */ +__u8 rls_port_id[3 ];/* port ID */ +}; + +/* +* ELS RLS LS_ACC Response. +*/ +struct fc_els_rls_resp { +__u8 rls_cmd;/* ELS_LS_ACC */ +__u8 rls_resv[3 ];/* reserved - must be zero */ +struct fc_els_lesb rls_lesb;/* link error status block */ +}; + +/* +* ELS RLIR - Registered Link Incident Report. +* This is followed by the CLIR and the CLID, described below. +*/ +struct fc_els_rlir { +__u8 rlir_cmd;/* command */ +__u8 rlir_resv[3 ];/* reserved - must be zero */ +__u8 rlir_fmt;/* format (FC4-type if type specific) */ +__u8 rlir_clr_len;/* common link incident record length */ +__u8 rlir_cld_len;/* common link incident desc. length */ +__u8 rlir_slr_len;/* spec. link incident record length */ +}; + +/* +* CLIR - Common Link Incident Record Data. - Sent via RLIR. +*/ +struct fc_els_clir { +__be64 clir_wwpn;/* incident port name */ +__be64 clir_wwnn;/* incident port node name */ +__u8 clir_port_type;/* incident port type */ +__u8 clir_port_id[3 ];/* incident port ID */ + +__be64 clir_conn_wwpn;/* connected port name */ +__be64 clir_conn_wwnn;/* connected node name */ +__be64 clir_fab_name;/* fabric name */ +__be32 clir_phys_port;/* physical port number */ +__be32 clir_trans_id;/* transaction ID */ +__u8 clir_resv[3 ];/* reserved */ +__u8 clir_ts_fmt;/* time stamp format */ +__be64 clir_timestamp;/* time stamp */ +}; + +/* +* CLIR clir_ts_fmt - time stamp format values. +*/ +enum fc_els_clir_ts_fmt { +ELS_CLIR_TS_UNKNOWN =0 ,/* time stamp field unknown */ +ELS_CLIR_TS_SEC_FRAC =1 ,/* time in seconds and fractions */ +ELS_CLIR_TS_CSU =2 ,/* time in clock synch update format */ +}; + +/* +* Common Link Incident Descriptor - sent via RLIR. +*/ +struct fc_els_clid { +__u8 clid_iq;/* incident qualifier flags */ +__u8 clid_ic;/* incident code */ +__be16 clid_epai;/* domain/area of ISL */ +}; + +/* +* CLID incident qualifier flags. +*/ +enum fc_els_clid_iq { +ELS_CLID_SWITCH =0x20 ,/* incident port is a switch node */ +ELS_CLID_E_PORT =0x10 ,/* incident is an ISL (E) port */ +ELS_CLID_SEV_MASK =0x0c ,/* severity 2-bit field mask */ +ELS_CLID_SEV_INFO =0x00 ,/* report is informational */ +ELS_CLID_SEV_INOP =0x08 ,/* link not operational */ +ELS_CLID_SEV_DEG =0x04 ,/* link degraded but operational */ +ELS_CLID_LASER =0x02 ,/* subassembly is a laser */ +ELS_CLID_FRU =0x01 ,/* format can identify a FRU */ +}; + +/* +* CLID incident code. +*/ +enum fc_els_clid_ic { +ELS_CLID_IC_IMPL =1 ,/* implicit incident */ +ELS_CLID_IC_BER =2 ,/* bit-error-rate threshold exceeded */ +ELS_CLID_IC_LOS =3 ,/* loss of synch or signal */ +ELS_CLID_IC_NOS =4 ,/* non-operational primitive sequence */ +ELS_CLID_IC_PST =5 ,/* primitive sequence timeout */ +ELS_CLID_IC_INVAL =6 ,/* invalid primitive sequence */ +ELS_CLID_IC_LOOP_TO =7 ,/* loop initialization time out */ +ELS_CLID_IC_LIP =8 ,/* receiving LIP */ +}; + +#endif /* _FC_ELS_H_ */
include/scsi/fc/fc_encaps.h new file +138 -0
138 lines not on this page · raw message
include/scsi/fc/fc_fc2.h new file +124 -0
124 lines not on this page · raw message
include/scsi/fc/fc_fcoe.h new file +114 -0
114 lines not on this page · raw message
include/scsi/fc/fc_fcp.h new file +199 -0
199 lines not on this page · raw message
include/scsi/fc/fc_fs.h new file +340 -0
340 lines not on this page · raw message
include/scsi/fc/fc_gs.h new file +93 -0
93 lines not on this page · raw message
include/scsi/fc/fc_ns.h new file +159 -0
159 lines not on this page · raw message
PATCH 2/3 libfc: A modular Fibre Channel library
In reply to Robert LovelibFC is composed of 4 blocks supported by an exchange manager and a framing library. The upper 4 layers are fc_lport, fc_disc, fc_rport and fc_fcp. A LLD that uses libfc could choose to either use libfc's block, or using the transport template defined in libfc.h, override one or more blocks with its own implementation.
The EM (Exchange Manager) manages exhcanges/sequences for all commands- ELS, CT and FCP.
The framing library frames ELS and CT commands.
The fc_lport block manages the library's representation of the host's FC enabled ports.
The fc_disc block manages discovery of targets as well as handling changes that occur in the FC fabric (via. RSCN events).
The fc_rport block manages the library's representation of other entities in the FC fabric. Currently the library uses this block for targets, its peer when in point-to-point mode and the directory server, but can be extended for other entities if needed.
The fc_fcp block interacts with the scsi-ml and handles all I/O.
---
drivers/scsi/Kconfig | 6 drivers/scsi/Makefile | 1 drivers/scsi/libfc/Makefile | 12 drivers/scsi/libfc/fc_disc.c | 845 ++++++++++++++++ drivers/scsi/libfc/fc_elsct.c | 71 + drivers/scsi/libfc/fc_exch.c | 1970 ++++++++++++++++++++++++++++++++++++++ drivers/scsi/libfc/fc_fcp.c | 2130 +++++++++++++++++++++++++++++++++++++++++ drivers/scsi/libfc/fc_frame.c | 89 ++ drivers/scsi/libfc/fc_lport.c | 1604 +++++++++++++++++++++++++++++++ drivers/scsi/libfc/fc_rport.c | 1291 +++++++++++++++++++++++++ include/scsi/fc_encode.h | 309 ++++++ include/scsi/fc_frame.h | 242 +++++ include/scsi/libfc.h | 938 ++++++++++++++++++ 13 files changed, 9508 insertions(+), 0 deletions(-) create mode 100644 drivers/scsi/libfc/Makefile create mode 100644 drivers/scsi/libfc/fc_disc.c create mode 100644 drivers/scsi/libfc/fc_elsct.c create mode 100644 drivers/scsi/libfc/fc_exch.c create mode 100644 drivers/scsi/libfc/fc_fcp.c create mode 100644 drivers/scsi/libfc/fc_frame.c create mode 100644 drivers/scsi/libfc/fc_lport.c create mode 100644 drivers/scsi/libfc/fc_rport.c create mode 100644 include/scsi/fc_encode.h create mode 100644 include/scsi/fc_frame.h create mode 100644 include/scsi/libfc.h
drivers/scsi/Kconfig +6 -0
@@ -603,6 +603,12 @@ config SCSI_FLASHPOINT substantial, so users of MultiMaster Host Adapters may not wish to include it. +config LIBFC + tristate "LibFC module" + depends on SCSI && SCSI_FC_ATTRS + ---help--- + Fibre Channel library module + config SCSI_DMX3191D tristate "DMX3191D SCSI support" depends on PCI && SCSI
drivers/scsi/Makefile +1 -0
@@ -36,6 +36,7 @@ obj-$(CONFIG_SCSI_SAS_LIBSAS) += libsas/ obj-$(CONFIG_SCSI_SRP_ATTRS) += scsi_transport_srp.o obj-$(CONFIG_SCSI_DH) += device_handler/ +obj-$(CONFIG_LIBFC) += libfc/ obj-$(CONFIG_ISCSI_TCP) += libiscsi.o libiscsi_tcp.o iscsi_tcp.o obj-$(CONFIG_INFINIBAND_ISER) += libiscsi.o obj-$(CONFIG_SCSI_A4000T) += 53c700.o a4000t.o
drivers/scsi/libfc/Makefile new file +12 -0
@@ -0,0 +1,12 @@ +# $Id: Makefile + +obj-$(CONFIG_LIBFC) += libfc.o + +libfc-objs := \ + fc_disc.o \ + fc_exch.o \ + fc_elsct.o \ + fc_frame.o \ + fc_lport.o \ + fc_rport.o \ + fc_fcp.o
drivers/scsi/libfc/fc_disc.c new file +845 -0
845 lines not on this page · raw message
drivers/scsi/libfc/fc_elsct.c new file +71 -0
71 lines not on this page · raw message
drivers/scsi/libfc/fc_exch.c new file +1970 -0
1,970 lines not on this page · raw message
drivers/scsi/libfc/fc_fcp.c new file +2130 -0
83 lines not on this page · raw message
drivers/scsi/libfc/fc_frame.c new file +89 -0
89 lines not on this page · raw message
drivers/scsi/libfc/fc_lport.c new file +1604 -0
1,604 lines not on this page · raw message
drivers/scsi/libfc/fc_rport.c new file +1291 -0
1,291 lines not on this page · raw message
include/scsi/fc_encode.h new file +309 -0
309 lines not on this page · raw message
include/scsi/fc_frame.h new file +242 -0
242 lines not on this page · raw message
include/scsi/libfc.h new file +938 -0
938 lines not on this page · raw message
The rest of this diff is not shown. View the raw message.
Part of the diff is not shown here. View the raw message.
Re: [PATCH 2/3] libfc: A modular Fibre Channel library
Quoted 6 lines · Robert Love
libFC is composed of 4 blocks supported by an exchange manager and a framing library. The upper 4 layers are
fc_lport,fc_disc,fc_rportandfc_fcp. A LLD that uses libfc could choose to either use libfc's block, or using the transport template defined inlibfc.h, override one or more blocks with its own implementation.
Just some more stuff I noticed while looking briefly at libfcoe.c Not a complete review again.
230 #ifdef CONFIG_SMP
231 /*
232 * The exchange ID are ANDed with num of online CPUs, 233 * so that will have the least lock contention in 234 * handling the exchange. if there is no thread 235 * for a given idx then use first online cpu. 236 */
237 cpu_idx = oxid & (num_online_cpus() >> 1);
238 if (fcoe_percpu[cpu_idx] == NULL)
239 cpu_idx = first_cpu(cpu_online_map);
240 #endif
What is this supposed to do? It looks quite dubious. Shouldn't this use the current CPU number?
include/scsi/libfc.h: struct fcoe_dev_stats *dev_stats[NR_CPUS];
NR_CPUS arrays are strongly discouraged because they lead to waste on kernels compiled with large NR_CPUS, ideally use per CPU data or the per cpu allocator or at least a dynamic allocation with num_possible_cpus() In general every use of NR_CPUS is dubious. e.g. you have some loops over that, these should be all over possible cpus. fcoe_percpu[] should be also per cpu data, not an array.
It would probably simplify the code a lot if you just went to global per cpu counters instead of per cpu device counters. I don't know if that is feasible or not.
static int __init fcoe_init(void)
There are CPU hotplug races between the notifier and the for_each_online_cpu() loop (probably not a serious problem, such races are common all over the code base, just wanted to mention it)
static int fcoe_device_notification(struct notifier_block *notifier,
ulong event, void *ptr)
the link_status field seems to be only protected by the rtnl semaphore here which is normally hold in this notifier chain. But if any other users in the fcoe stack change that too there might be races. Also normally it's better to check "event" first before doing anything expensive like walking lists. There are definitely more events you're not handling, but right now you would slow them down.
fcoe_ethdrv_get/put
It's not quite clear to me why you go to the trouble to lock the underlying NIC module. Why would it be a problem if it goes away? Normally one would rather keep a reference to the device, but even that might be not needed.
static int fcoe_get_paged_crc_eof(struct sk_buff *skb, int tlen)
get_page(page);
skb_fill_page_desc(skb, skb_shinfo(skb)->nr_frags, page,
fps->crc_eof_offset, tlen);
skb->len += tlen;
skb->data_len += tlen;
skb->truesize += tlen;
fps->crc_eof_offset += sizeof(struct fcoe_crc_eof);
if (fps->crc_eof_offset >= PAGE_SIZE) {
fps->crc_eof_page = NULL;
fps->crc_eof_offset = 0;
put_page(page);
}Is the put_page() here really correct? The original reference in the skb will be still around, won't it?
static void fcoe_insert_wait_queue(struct fc_lport *lp,
struct sk_buff *skb)
{
struct fcoe_softc *fc;
fc = fcoe_softc(lp);
spin_lock_bh(&fc->fcoe_pending_queue.lock);
__skb_queue_tail(&fc->fcoe_pending_queue, skb);
spin_unlock_bh(&fc->fcoe_pending_queue.lock);
}That's just skb_queue_tail() (except that the lock is hardirq safe there, but that should be fine)
/* adjust skb netowrk/transport offsets to match mac/fcoe/fc */
typo
Quoted 12 lines
The EM (Exchange Manager) manages exhcanges/sequences for all commands- ELS, CT and FCP.
The framing library frames ELS and CT commands.
The
fc_lportblock manages the library's representation of the host's FC enabled ports.The
fc_discblock manages discovery of targets as well as handling changes that occur in the FC fabric (via. RSCN events).
Quoted 8 lines
The
fc_rportblock manages the library's representation of other entities in the FC fabric. Currently the library uses this block for targets, its peer when in point-to-point mode and the directory server, but can be extended for other entities if needed.The
fc_fcpblock interacts with the scsi-ml and handles all I/O.
A cheatsheet for all the acronyms would be nice.
-Andi
Re: [PATCH 2/3] libfc: A modular Fibre Channel library
Quoted 20 lines · Andi Kleen
Quoted 7 lines · Robert Love
libFC is composed of 4 blocks supported by an exchange manager and a framing library. The upper 4 layers are
fc_lport,fc_disc,fc_rportandfc_fcp. A LLD that uses libfc could choose to either use libfc's block, or using the transport template defined inlibfc.h, override one or more blocks with its own implementation.Just some more stuff I noticed while looking briefly at
libfcoe.cNot a complete review again.230 #ifdef
CONFIG_SMP
231 /*232 * The exchange ID are ANDed with num of online CPUs, 233 * so that will have the least lock contention in 234 * handling the exchange. if there is no thread 235 * for a given idx then use first online cpu. 236 */
Quoted 7 lines
237
cpu_idx= oxid & (num_online_cpus()>> 1);
238 if (fcoe_percpu[cpu_idx] == NULL)
239cpu_idx=first_cpu(cpu_online_map);
240 #endifWhat is this supposed to do? It looks quite dubious.
It had load balancing issue but now it is fixed, related latest submitted code with its updated comment is:-
/* * The incoming frame exchange id(oxid) is ANDed with num of online * cpu bits to get cpu_idx and then this cpu_idx is used for selecting * a per cpu kernel thread from fcoe_percpu. In case the cpu is * offline or no kernel thread for derived cpu_idx then cpu_idx is * initialize to first online cpu index. */ cpu_idx = oxid & (num_online_cpus() - 1); if (!fcoe_percpu[cpu_idx] || !cpu_online(cpu_idx)) cpu_idx = first_cpu(cpu_online_map);
The incoming FC frame is distributed across all fcoe_percpu_receive_thread for better load balancing, these per CPU threads are created by fcoe module during fcoe_init. This should help in performance but not sure how much since performance tuning is yet to be done.
Quoted 3 lines
Shouldn't this use the current CPU number?
The current cpu could be used here with some other ingress FC frames distribution/hashing across all cpus instead this hash logic "oxid & (num_online_cpus() - 1)", possibly ingress FC frame distribution at soft irq level by oxid and then passing up to current cpu's kernel thread fcoe_percpu_receive_thread.
Re: [PATCH 2/3] libfc: A modular Fibre Channel library
Quoted 4 lines · Vasu Dev
It had load balancing issue but now it is fixed, related latest submitted code with its updated comment is:-
Quoted 9 lines
/*
* The incoming frame exchange id(oxid) is ANDed with num of online
* cpu bits to getcpu_idxand then thiscpu_idxis used for selecting
* a per cpu kernel thread fromfcoe_percpu. In case the cpu is
* offline or no kernel thread for derivedcpu_idxthencpu_idxis
* initialize to first online cpu index.
*/
cpu_idx= oxid & (num_online_cpus()- 1);
First note that num_online_cpus() is not guaranteed to be a power of two,
- 1 is not guaranteed to give a suitable mask. So you might actually lose random bits. Also your load balancing scheme is unusual to say at least. e.g. when you're just talking to a single frame exchange you would always transfer data between CPUs instead of keeping it all on the CPU that processes the interrupt. Normally the rule of thumb is to use local data as much as possible. Or when you distribute like this at least stay in the same socket. But probably the scheme is too clever anyways.
-Andi
Signature · 1 line
[email protected]
Re: [PATCH 2/3] libfc: A modular Fibre Channel library
Quoted 15 lines · Andi Kleen
Quoted 12 lines · Vasu Dev
It had load balancing issue but now it is fixed, related latest submitted code with its updated comment is:-
/*
* The incoming frame exchange id(oxid) is ANDed with num of online
* cpu bits to getcpu_idxand then thiscpu_idxis used for selecting
* a per cpu kernel thread fromfcoe_percpu. In case the cpu is
* offline or no kernel thread for derivedcpu_idxthencpu_idxis
* initialize to first online cpu index.
*/
cpu_idx= oxid & (num_online_cpus()- 1);
Quoted 3 lines
First note that
num_online_cpus()is not guaranteed to be a power of two,
- 1 is not guaranteed to give a suitable mask. So you might actually lose random bits.
Correct, this will work best for only power of 2 online cpus and that would be the most common typical use case. I agree it won't load balance better in non power of 2 cpus case.
Quoted 7 lines
Also your load balancing scheme is unusual to say at least.
e.g. when you're just talking to a single frame exchange you would always transfer data between CPUs instead of keeping it all on the CPU that processes the interrupt.Normally the rule of thumb is to use local
data as much as possible. Or when you distribute like this at least stay in the same socket.
We cannot control what cpu to get interrupted for a FC frame in a typical generic NIC, so we may end up receiving mostly all FC frames on a single same cpu though system might have several other cpus available. In this scenario if frame are passed up to same cpu as suggested above then that won't do any load balancing, therefore some sort of load balancing is required based on some FC frame attributes here.
As I said in my last response that "performance tuning is yet to be done" but you bring up some good related points now of cross socket frame migration and balancing on non power of 2 cpus system. These should be considered during pending performance tuning but for now I can add additional check to select cpu within same socket but not sure how to do that, any kernel call for this ? This might cause more locking contentions on libfc structs so really we have to experiment these thing during performance tuning. Thanks Andi for these hints on performance consideration.
Vasu
Re: [PATCH 2/3] libfc: A modular Fibre Channel library
Quoted 49 lines · Vasu Dev
Quoted 16 lines · Andi Kleen
Quoted 13 lines · Vasu Dev
It had load balancing issue but now it is fixed, related latest submitted code with its updated comment is:-
/*
* The incoming frame exchange id(oxid) is ANDed with num of online
* cpu bits to getcpu_idxand then thiscpu_idxis used for
selecting
* a per cpu kernel thread fromfcoe_percpu. In case the cpu is
* offline or no kernel thread for derivedcpu_idxthencpu_idxis
* initialize to first online cpu index.
*/
cpu_idx= oxid & (num_online_cpus()- 1);Quoted 3 lines
First note that
num_online_cpus()is not guaranteed to be a power of two,
- 1 is not guaranteed to give a suitable mask. So you might actually lose random bits.Correct, this will work best for only power of 2 online cpus and that would be the most common typical use case. I agree it won't load balance better in non power of 2 cpus case.
Quoted 6 lines
Also your load balancing scheme is unusual to say at least. e.g. when
you're just talking to a single frame exchange you would always transfer data between CPUs instead of keeping it all on the CPU that
processes the interrupt. Normally the rule of thumb is to use local
data as much as possible. Or when you distribute like this at least stay in the same socket.We cannot control what cpu to get interrupted for a FC frame in a typical generic NIC, so we may end up receiving mostly all FC frames on a single same cpu though system might have several other cpus available. In this scenario if frame are passed up to same cpu as suggested above then that won't do any load balancing, therefore some sort of load balancing is required based on some FC frame attributes here.
As I said in my last response that "performance tuning is yet to be done" but you bring up some good related points now of cross socket frame migration and balancing on non power of 2 cpus system. These should be considered during pending performance tuning but for now I can add additional check to select cpu within same socket but not sure how to do that, any kernel call for this ? This might cause more locking contentions on libfc structs so really we have to experiment these thing during performance tuning. Thanks Andi for these hints on performance consideration.
Quoted 1 line
Vasu
Somehow the exchange IDs should be allocated so that responses can be directed back to the same CPU that issued the requests. At one time the mask was a computed mask such that it was always a power of 2.
If we made a separate exchange ID space per CPU and the LLD allocated the XIDs, then it could also be sure it was doing the right thing with the responses.
If we used a separate exchange manager for each CPU, there would be less
lock contention in the EM code. As we diffy up the exchange ID space
between the multiple EMs, we could use the high-order bits to be the CPU number. We would probably want to limit the number of EMs to something like 8 or 16 even on systems with more CPUs.
Like you said, Vasu, this is for performance tuning time, whenever that is. However, I think that might be a few releases away or a gradual evolution since there might be some significant changes involved.
Joe
Re: [PATCH 2/3] libfc: A modular Fibre Channel library
In reply to Andi KleenQuoted 2 lines · Andi Kleen
Is the
put_page()here really correct? The original reference in the skb will be still around, won't it?
But you didn't quote the part with the alloc_page call. Here's the whole function.
Quoted 27 lines
static int
fcoe_get_paged_crc_eof(struct sk_buff *skb, int tlen){ struct fcoe_percpu_s *fps; struct page *page; int cpu_idx; cpu_idx = get_cpu(); fps = fcoe_percpu[cpu_idx]; page = fps->crc_eof_page; if (!page) { page = alloc_page(GFP_ATOMIC); if (!page) { put_cpu(); return -ENOMEM; } fps->crc_eof_page = page; WARN_ON(fps->crc_eof_offset != 0); } get_page(page); skb_fill_page_desc(skb, skb_shinfo(skb)->nr_frags, page, fps->crc_eof_offset, tlen); skb->len += tlen; skb->data_len += tlen; skb->truesize += tlen; fps->crc_eof_offset += sizeof(struct fcoe_crc_eof);
Quoted 8 lines
if (fps->crc_eof_offset >= PAGE_SIZE) {
fps->crc_eof_page = NULL;
fps->crc_eof_offset = 0;
put_page(page);
}
put_cpu();
return 0;
}FCoE needs to attach an 8 byte trailer to all FC frames created by libfc before transmitting. If the skb is non-linear, say a SCSI write data frame, the only way to do that is to attach a fragment page to the skb.
In order to avoid repeated calls to alloc_page when only 8 bytes are needed, the fcoe module keeps a reference to the page using it repeatedly until it reaches the end. Then then call to put_page happens, and a new page is allocated on the next transmit. At this point the page may still have references from any skbs that are still outstanding (get_page before skb_fill_page_desc). With 4k pages 512 fragmented frames can be sent before calling alloc_page again.
- Chris
Re: [PATCH 2/3] libfc: A modular Fibre Channel library
In reply to Andi KleenQuoted 2 lines · Andi Kleen
Is the
put_page()here really correct? The original reference in the skb will be still around, won't it?
But you didn't quote the part with the alloc_page call. Here's the whole function.
Quoted 27 lines
static int
fcoe_get_paged_crc_eof(struct sk_buff *skb, int tlen){ struct fcoe_percpu_s *fps; struct page *page; int cpu_idx; cpu_idx = get_cpu(); fps = fcoe_percpu[cpu_idx]; page = fps->crc_eof_page; if (!page) { page = alloc_page(GFP_ATOMIC); if (!page) { put_cpu(); return -ENOMEM; } fps->crc_eof_page = page; WARN_ON(fps->crc_eof_offset != 0); } get_page(page); skb_fill_page_desc(skb, skb_shinfo(skb)->nr_frags, page, fps->crc_eof_offset, tlen); skb->len += tlen; skb->data_len += tlen; skb->truesize += tlen; fps->crc_eof_offset += sizeof(struct fcoe_crc_eof);
Quoted 8 lines
if (fps->crc_eof_offset >= PAGE_SIZE) {
fps->crc_eof_page = NULL;
fps->crc_eof_offset = 0;
put_page(page);
}
put_cpu();
return 0;
}FCoE needs to attach an 8 byte trailer to all FC frames created by libfc before transmitting. If the skb is non-linear, say a SCSI write data frame, the only way to do that is to attach a fragment page to the skb.
In order to avoid repeated calls to alloc_page when only 8 bytes are needed, the fcoe module keeps a reference to the page using it repeatedly until it reaches the end. Then then call to put_page happens, and a new page is allocated on the next transmit. At this point the page may still have references from any skbs that are still outstanding (get_page before skb_fill_page_desc). With 4k pages 512 fragmented frames can be sent before calling alloc_page again.
- Chris
RE: [PATCH 2/3] libfc: A modular Fibre Channel library
In reply to Andi KleenQuoted 16 lines
include/scsi/libfc.h: structfcoe_dev_stats*dev_stats[NR_CPUS];
NR_CPUSarrays are strongly discouraged because they lead to waste on kernels compiled with largeNR_CPUS, ideally use per CPU data or the per cpu allocator or at least a dynamic allocation withnum_possible_cpus()
In general every use ofNR_CPUSis dubious. e.g. you have some loops over that,
these should be all over possible cpus. fcoe_percpu[] should be also per cpu data, not an array.It would probably simplify the code a lot if you just went to global per cpu counters instead of per cpu device counters. I don't know if that is feasible or not.
Quoted 8 lines
static int
__initfcoe_init(void)There are CPU hotplug races between the notifier and the
for_each_online_cpu()
loop (probably not a serious problem, such races are common all over the
code base, just wanted to mention it)
Hi, Andi,
Thanks for the comments, I have submitted a patch to open-fcoe.org to convert the foce_percpu[] to be kernel percpu var as you suggested. We also have a dev_stats[NR_CPUS] as a member of the struct fc_lport. It stores an array of pointers to struct fcoe_dev_stats, which keeps track of all the statistics of fcoe traffic. The fc_lport is dynamically allocated upon creating fcoe instance from a given network interface name. The dev_stats are allocated initially in a for_each_online_cpu)() loop, and after that, corresponding dev_stats is allocated/freed upon CPU_ONLINE/CPU_DEAD event callback fcoe_cpu_callback(), do you think this is also a problem in using NR_CPUS?
Thanks.
yi
Re: [PATCH 2/3] libfc: A modular Fibre Channel library
Quoted 4 lines
name. The
dev_statsare allocated initially in a for_each_online_cpu)() loop, and after that, correspondingdev_statsis allocated/freed upon CPU_ONLINE/CPU_DEAD event callbackfcoe_cpu_callback(), do you think this is also a problem in usingNR_CPUS?
Yes, it's also a problem. Use num_possible_cpus()
-Andi
PATCH 3/3 fcoe: Fibre Channel over Ethernet
In reply to Robert LoveEncapsulation protocol for running Fibre Channel over Ethernet interfaces. Creates virtual Fibre Channel host adapters using libfc.
This layer is the LLD to the scsi-ml. It allocates the Scsi_Host, utilizes libfc for Fibre Channel protocol processing and interacts with netdev to send/receive Ethernet packets.
---
drivers/scsi/Kconfig | 7 drivers/scsi/Makefile | 1 drivers/scsi/fcoe/Makefile | 8 drivers/scsi/fcoe/fc_transport_fcoe.c | 446 ++++++++++ drivers/scsi/fcoe/fcoe_sw.c | 494 +++++++++++ drivers/scsi/fcoe/libfcoe.c | 1510 +++++++++++++++++++++++++++++++++ include/scsi/fc_transport_fcoe.h | 54 + include/scsi/libfcoe.h | 176 ++++ 8 files changed, 2696 insertions(+), 0 deletions(-) create mode 100644 drivers/scsi/fcoe/Makefile create mode 100644 drivers/scsi/fcoe/fc_transport_fcoe.c create mode 100644 drivers/scsi/fcoe/fcoe_sw.c create mode 100644 drivers/scsi/fcoe/libfcoe.c create mode 100644 include/scsi/fc_transport_fcoe.h create mode 100644 include/scsi/libfcoe.h
drivers/scsi/Kconfig +7 -0
@@ -609,6 +609,13 @@ config LIBFC ---help--- Fibre Channel library module +config FCOE + tristate "FCoE module" + depends on SCSI + select LIBFC + ---help--- + Fibre Channel over Ethernet module + config SCSI_DMX3191D tristate "DMX3191D SCSI support" depends on PCI && SCSI
drivers/scsi/Makefile +1 -0
@@ -37,6 +37,7 @@ obj-$(CONFIG_SCSI_SRP_ATTRS) += scsi_transport_srp.o obj-$(CONFIG_SCSI_DH) += device_handler/ obj-$(CONFIG_LIBFC) += libfc/ +obj-$(CONFIG_FCOE) += fcoe/ obj-$(CONFIG_ISCSI_TCP) += libiscsi.o libiscsi_tcp.o iscsi_tcp.o obj-$(CONFIG_INFINIBAND_ISER) += libiscsi.o obj-$(CONFIG_SCSI_A4000T) += 53c700.o a4000t.o
drivers/scsi/fcoe/Makefile new file +8 -0
@@ -0,0 +1,8 @@ +# $Id: Makefile + +obj-$(CONFIG_FCOE) += fcoe.o + +fcoe-y := \ + libfcoe.o \ + fcoe_sw.o \ + fc_transport_fcoe.o
drivers/scsi/fcoe/fc_transport_fcoe.c new file +446 -0
446 lines not on this page · raw message
drivers/scsi/fcoe/fcoe_sw.c new file +494 -0
494 lines not on this page · raw message
drivers/scsi/fcoe/libfcoe.c new file +1510 -0
1,510 lines not on this page · raw message
include/scsi/fc_transport_fcoe.h new file +54 -0
54 lines not on this page · raw message
include/scsi/libfcoe.h new file +176 -0
176 lines not on this page · raw message
Re: [PATCH 3/3] fcoe: Fibre Channel over Ethernet
Quoted 7 lines · On Tue, 09 Dec 2008 15:10:24 -0800 Robert Love <[email protected]> wrote:
Encapsulation protocol for running Fibre Channel over Ethernet interfaces. Creates virtual Fibre Channel host adapters using libfc.
This layer is the LLD to the scsi-ml. It allocates the
Scsi_Host, utilizes libfc for Fibre Channel protocol processing and interacts with netdev to send/receive Ethernet packets.
I stumbled across this while looking for new and weird kthread API usages..
Quoted 12 lines
...
+/**
+ *fcoe_transport_lookup- check if the transport is already registered
+ * @t: the transport to be looked up
+ *
+ * This compares the parent device (pci) vendor and device id
+ *
+ * Returns: NULL if not found
+ *
+ * TODO - return default sw transport if no other transport is found
+ **/
The kerneldoc comments consistently close with
**/
which is consistently unconventional. Not wrong, just odd.
Quoted 10 lines
...
drivers/scsi/fcoe/fc_transport_fcoe.c new file +7
-0+int fcoe_transport_register (struct fcoe_transport *t) +{ +struct fcoe_transport *tt; + +/* TODO - add fcoe_transport specific initialization here */ +mutex_lock (&fcoe_transports_lock); +list_for_each_entry (tt, &fcoe_transports, list) {
drivers/scsi/fcoe/fc_transport_fcoe.c line 263
Quoted 10 lines
drivers/scsi/fcoe/fc_transport_fcoe.c new file +10
-0+if (tt == t) { +mutex_unlock (&fcoe_transports_lock); +return -EEXIST ; + } + } +list_add_tail (&t->list, &fcoe_transports); +mutex_unlock (&fcoe_transports_lock); + +mutex_init (&t->devlock); +INIT_LIST_HEAD (&t->devlist);
Seems wrong to make this available for lookups pror to completing its initialisation.
Quoted 14 lines
drivers/scsi/fcoe/fc_transport_fcoe.c new file +11
-0+printk (KERN_DEBUG "fcoe_transport_register:%s\n" , t->name); + +return 0 ; +} +EXPORT_SYMBOL_GPL (fcoe_transport_register); + +int fcoe_load_transport_driver (struct net_device *netdev) +{ +struct pci_dev *pci; +struct device *dev = netdev->dev.parent; +
drivers/scsi/fcoe/fc_transport_fcoe.c line 325
Quoted 10 lines
drivers/scsi/fcoe/fc_transport_fcoe.c new file +10
-0+if (fcoe_transport_lookup (netdev)) { +/* load default transport */ +printk (KERN_DEBUG "fcoe: already loaded transport for %s\n" , + netdev->name); +return -EEXIST ; + } + + pci =to_pci_dev (dev); +if (dev->bus != &pci_bus_type) { +printk (KERN_DEBUG "fcoe: support noly PCI device\n" );
typo
Quoted 24 lines
drivers/scsi/fcoe/fcoe_sw.c new file +21
-0+return -ENODEV ; + } +printk (KERN_DEBUG "fcoe: loading driver fcoe-pci-0x%04x-0x%04x\n" , + pci->vendor, pci->device); + +return request_module ("fcoe-pci-0x%04x-0x%04x" , + pci->vendor, pci->device); + +static int fcoe_sw_lport_config (struct fc_lport *lp) +{ +int i =0 ; + + lp->link_status =0 ; + lp->max_retry_count =3 ; + lp->e_d_tov =2 *1000 ;/* FC-FS default */ + lp->r_a_tov =2 *2 *1000 ; + lp->service_params = (FCP_SPPF_INIT_FCN |FCP_SPPF_RD_XRDY_DIS | +FCP_SPPF_RETRY |FCP_SPPF_CONF_COMPL ); + +/* +* allocate per cpu stats block
drivers/scsi/fcoe/fcoe_sw.c line 137
Quoted 10 lines
drivers/scsi/fcoe/fcoe_sw.c new file +10
-0+*/ +for_each_online_cpu (i) + lp->dev_stats[i] =kzalloc (sizeof (struct fcoe_dev_stats ), +GFP_KERNEL ); + +/* lport fc_lport related configuration */ +fc_lport_config (lp); + +return 0 ; +}
Has the CPU hotplugging code been tested? This seems to not do things which fcoe_create_percpu_data() does.
Quoted 21 lines
drivers/scsi/fcoe/fcoe_sw.c new file +18
-0+/* +* fcoe_sw_netdev_config - sets up fcoe_softc for lport and network +* related properties +* @lp : ptr to the fc_lport +* @netdev : ptr to the associated netdevice struct +* +* Must be called after fcoe_sw_lport_config() as it will use lport mutex +* +* Returns : 0 for success +* +*/ +static int fcoe_cpu_callback (struct notifier_block *nfb,unsigned long action, +void *hcpu) +{ +unsigned int cpu = (unsigned long )hcpu; + +switch (action) { +case CPU_ONLINE :
drivers/scsi/fcoe/fcoe_sw.c line 225
Quoted 10 lines
drivers/scsi/fcoe/fcoe_sw.c new file +10
-0+fcoe_create_percpu_data (cpu); +break ; +case CPU_DEAD : +fcoe_destroy_percpu_data (cpu); +break ; +default : +break ; + } +return NOTIFY_OK ; +}
This function forgets to create and destroy the per-cpu kernel threads.
Quoted 8 lines
...
+/*
+ *fcoe_percpu_clean- frees skb of the corresponding lport from the per
+ * cpu queue.
+ * @lp: thefc_lport
+ */
This purports to bea kerneldoc comment but mieese the /** token.
The first line of the kernedoc comment should not have a linbreak - kerneldoc doesn't understand that.
Quoted 8 lines
...
drivers/scsi/fcoe/libfcoe.c new file +5
-0+static struct fcoe_softc *fcoe_hostlist_lookup_softc ( +const struct net_device *dev) +{ +struct fcoe_softc *fc; +
drivers/scsi/fcoe/libfcoe.c line 1309
Quoted 10 lines
drivers/scsi/fcoe/libfcoe.c new file +10
-0+read_lock (&fcoe_hostlist_lock); +list_for_each_entry (fc, &fcoe_hostlist, list) { +if (fc->real_dev == dev) { +read_unlock (&fcoe_hostlist_lock); +return fc; + } + } +read_unlock (&fcoe_hostlist_lock); +return NULL ; +}
It's strange to take a lock, look something up, drop the lock and then return the looked-up object without having taken any steps to prevent some other thread of control from destroying that object while the caller is playing with it. (ie: take a reference on it).
Quoted 5 lines
...
drivers/scsi/fcoe/libfcoe.c new file +2
-0+static int __init fcoe_init (void ) +{
drivers/scsi/fcoe/libfcoe.c line 1411
Quoted 10 lines
drivers/scsi/fcoe/libfcoe.c new file +10
-0+int cpu; +struct fcoe_percpu_s *p; + + +INIT_LIST_HEAD (&fcoe_hostlist); +rwlock_init (&fcoe_hostlist_lock); + +#ifdef CONFIG_HOTPLUG_CPU +register_cpu_notifier (&fcoe_cpu_notifier); +#endif /* CONFIG_HOTPLUG_CPU */
This could/should use hotcpu_notifier(), lose the ifdefs.
Quoted 18 lines
drivers/scsi/fcoe/libfcoe.c new file +18
-0+ +/* +* initialize per CPU interrupt thread +*/ +for_each_online_cpu (cpu) { + p =kzalloc (sizeof (struct fcoe_percpu_s ),GFP_KERNEL ); +if (p) { + p->thread =kthread_create (fcoe_percpu_receive_thread, + (void *)p, +"fcoethread/%d" , cpu); + +/* +* if there is no error then bind the thread to the cpu +* initialize the semaphore and skb queue head +*/ +if (likely (!IS_ERR (p->thread))) { + p->cpu = cpu; + fcoe_percpu[cpu] = p;
drivers/scsi/fcoe/libfcoe.c line 1439
Quoted 10 lines
drivers/scsi/fcoe/libfcoe.c new file +10
-0+skb_queue_head_init (&p->fcoe_rx_list); +kthread_bind (p->thread, cpu); +wake_up_process (p->thread); + }else { + fcoe_percpu[cpu] =NULL ; +kfree (p); + + } + } + }
afaict nothing is done here to avoid races against CPU hotplug.
What happens if the kzalloc() failed?
drivers/scsi/fcoe/libfcoe.c line 1448
Quoted 8 lines
drivers/scsi/fcoe/libfcoe.c new file +8
-0+/* +* setup link change notification +*/ +fcoe_dev_setup (); + +init_timer (&fcoe_timer); + fcoe_timer.data =0 ; + fcoe_timer.function = fcoe_watchdog;
Could use setup_timer() here.
drivers/scsi/fcoe/libfcoe.c line 1450
Quoted 2 lines
drivers/scsi/fcoe/libfcoe.c new file +2
-0+ fcoe_timer.expires = (jiffies + (10 *HZ )); +add_timer (&fcoe_timer);
Could actually use mod_timer() here.
Quoted 17 lines
drivers/scsi/fcoe/libfcoe.c new file +17
-0+/* initiatlize the fcoe transport */ +fcoe_transport_init (); + +fcoe_sw_init (); + +return 0 ; +} +module_init (fcoe_init); + +/** +* fcoe_exit - fcoe module unloading cleanup +* +* Returns 0 on success, negative on failure +**/ +static void __exit fcoe_exit(void) +{ +u32 idx;
drivers/scsi/fcoe/libfcoe.c line 1478
Quoted 10 lines
drivers/scsi/fcoe/libfcoe.c new file +10
-0+struct fcoe_softc *fc, *tmp; +struct fcoe_percpu_s *p; +struct sk_buff *skb; + +/* +* Stop all call back interfaces +*/ +#ifdef CONFIG_HOTPLUG_CPU +unregister_cpu_notifier (&fcoe_cpu_notifier); +#endif /* CONFIG_HOTPLUG_CPU */
Use unregister_hotcpu_notifier(), lose ifdef (I think).
drivers/scsi/fcoe/libfcoe.c line 1486
Quoted 8 lines
drivers/scsi/fcoe/libfcoe.c new file +8
-0+fcoe_dev_cleanup (); + +/* +* stop timer +*/ +del_timer_sync (&fcoe_timer); + +/* releases the assocaited fcoe transport for each lport */
typo
Quoted 6 lines
drivers/scsi/fcoe/libfcoe.c new file +6
-0+list_for_each_entry_safe (fc, tmp, &fcoe_hostlist, list) +fcoe_transport_release (fc->real_dev); + +for (idx =0 ; idx <NR_CPUS ; idx++) { +if (fcoe_percpu[idx]) { +kthread_stop (fcoe_percpu[idx]->thread);
drivers/scsi/fcoe/libfcoe.c line 1502
Quoted 10 lines
drivers/scsi/fcoe/libfcoe.c new file +10
-0+ p = fcoe_percpu[idx]; +spin_lock_bh (&p->fcoe_rx_list.lock); +while ((skb =__skb_dequeue (&p->fcoe_rx_list)) !=NULL ) +kfree_skb (skb); +spin_unlock_bh (&p->fcoe_rx_list.lock); +if (fcoe_percpu[idx]->crc_eof_page) +put_page (fcoe_percpu[idx]->crc_eof_page); +kfree (fcoe_percpu[idx]); + } + }
The iterating across NR_CPUS is rather, umm, old-fashioned.
It'd be better to lock against cpu hotplugging then use for_each_online_cpu().
drivers/scsi/fcoe/libfcoe.c
Quoted 11 lines
drivers/scsi/fcoe/libfcoe.c new file +8
-0+/* remove sw trasnport */ +fcoe_sw_exit (); + +/* detach the transport */ +fcoe_transport_exit (); +} +static inline struct fcoe_softc *fcoe_softc ( +const struct fc_lport *lp)
weird code layout, used everywhere.
include/scsi/libfcoe.h line 64
Quoted 2 lines
include/scsi/libfcoe.h new file +2
-0+{ +return (struct fcoe_softc *)lport_priv (lp);
unneeded/undesirable cast of void*. There are probably zillions of instances of this - there always are.
Quoted 5 lines
drivers/scsi/fcoe/fc_transport_fcoe.c new file +2
-0+} +
Re: [PATCH 3/3] fcoe: Fibre Channel over Ethernet
Quoted 5 lines · Andrew Morton
Quoted 3 lines · On Tue, 09 Dec 2008 15:10:24 -0800 Robert Love <[email protected]> wrote:
Encapsulation protocol for running Fibre Channel over Ethernet interfaces. Creates virtual Fibre Channel host adapters using libfc.
Quoted 7 lines
Quoted 4 lines
This layer is the LLD to the scsi-ml. It allocates the
Scsi_Host, utilizes libfc for Fibre Channel protocol processing and interacts with netdev to send/receive Ethernet packets.I stumbled across this while looking for new and weird kthread API usages..
I just sent a patchset of Open-FCoE updates to linux-scsi. Patches in that set should take care of the kernel-doc issues, the initialization ordering problem for fcoe transports, the typos, the timer comments, and the unnecessary casting problem.
Quoted 13 lines
Quoted 12 lines
...
+/**
+ *fcoe_transport_lookup- check if the transport is already registered
+ * @t: the transport to be looked up
+ *
+ * This compares the parent device (pci) vendor and device id
+ *
+ * Returns: NULL if not found
+ *
+ * TODO - return default sw transport if no other transport is found
+ **/
Quoted 6 lines
The kerneldoc comments consistently close with
**/
which is consistently unconventional. Not wrong, just odd.
I did change the kernel-doc function comment blocks to end with '*/', since you called it out. I looked at the other comments under drivers/scsi/ and most did end in '*/'. However, the Documentation/kernel-doc-nano-HOWTO.txt has an example for function headers that shows '**/' as the suggested style. Should the kernel-doc HOWTO be updated?
Example kernel-doc function comment:
/**
* foobar() - short function description of foobar
* @arg1: Describe the first argument to foobar. * @arg2: Describe the second argument to foobar. * One can provide multiple line descriptions * for arguments.
*
* A longer description, with more discussion of the function foobar()
* that might be useful to those using or modifying it. Begins with
* empty comment line, and may include additional embedded empty
* comment lines.
*
* The longer description can have multiple paragraphs.
**/
Re: [PATCH 3/3] fcoe: Fibre Channel over Ethernet
Quoted 7 lines · On Fri, 06 Feb 2009 11:05:17 -0800 Robert Love <[email protected]> wrote:
Quoted 6 lines
The kerneldoc comments consistently close with
**/
which is consistently unconventional. Not wrong, just odd.
Quoted 6 lines
I did change the kernel-doc function comment blocks to end with '*/', since you called it out. I looked at the other comments under
drivers/scsi/and most did end in '*/'. However, theDocumentation/kernel-doc-nano-HOWTO.txthas an example for function headers that shows '**/' as the suggested style. Should the kernel-doc HOWTO be updated?
I think so - we might as well be consistent about these things.
Quoted 8 lines
Example kernel-doc function comment:
/**
*foobar()- short function description of foobar* @arg1: Describe the first argument to foobar. * @arg2: Describe the second argument to foobar. * One can provide multiple line descriptions * for arguments.
Quoted 8 lines
*
* A longer description, with more discussion of the functionfoobar()
* that might be useful to those using or modifying it. Begins with
* empty comment line, and may include additional embedded empty
* comment lines.
*
* The longer description can have multiple paragraphs.
**/
That's RandyStuff.
PATCH 1/1 kernel-doc: preferred ending marker and examples
From: Randy Dunlap <[email protected]>
Fix kernel-doc-nano-HOWTO.txt to use */ as the ending marker in kernel-doc examples and state that */ is the preferred ending marker.
--- Documentation/kernel-doc-nano-HOWTO.txt | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
Documentation/kernel-doc-nano-HOWTO.txt +4 -3
@@ -43,7 +43,8 @@ Only comments so marked will be consider and any comment so marked must be in kernel-doc format. Do not use "/**" to be begin a comment block unless the comment block contains kernel-doc formatted comments. The closing comment marker for -kernel-doc comments can be either "*/" or "**/". +kernel-doc comments can be either "*/" or "**/", but "*/" is +preferred in the Linux kernel tree. Kernel-doc comments should be placed just before the function or data structure being described.
@@ -63,7 +64,7 @@ Example kernel-doc function comment: * comment lines. * * The longer description can have multiple paragraphs. - **/ + */ The first line, with the short description, must be on a single line.
@@ -85,7 +86,7 @@ Example kernel-doc data structure commen * perhaps with more lines and words. * * Longer description of this structure. - **/ + */ The kernel-doc function comments describe each parameter to the function, in order, with the @name lines.