[02/14] scsi: fnic: Add headers and definitions for FDLS

Fabric Discovery and Login Services (FDLS) provide
functionality for fnic to discover the fabric and
target ports. It logs in to the target ports from
the initiator and creates target ports (tports).
This functionality is essential for port channel
register state change notification (PC-RSCN)
handling and FC-NVME initiator role.

This functionality is essential for eCPU hang or panic
handling. In cases where the eCPU in the UCS VIC (Unified
Computing Services Virtual Interface Card) hangs,
a fabric log out is sent to the fabric. Upon successful log
out from the fabric, the IO path is failed over to a new path.

Add headers and definitions for FDLS.

Reviewed-by: Sesidhar Baddela <sebaddel@cisco.com>
Reviewed-by: Arulprabhu Ponnusamy <arulponn@cisco.com>
Reviewed-by: Gian Carlo Boffa <gcboffa@cisco.com>
Signed-off-by: Karan Tilak Kumar <kartilak@cisco.com>
---
 drivers/scsi/fnic/fdls_fc.h   | 548 ++++++++++++++++++++++++++++++++++
 drivers/scsi/fnic/fnic_fdls.h | 362 ++++++++++++++++++++++
 2 files changed, 910 insertions(+)
 create mode 100644 drivers/scsi/fnic/fdls_fc.h
 create mode 100644 drivers/scsi/fnic/fnic_fdls.h

On 6/10/24 23:50, Karan Tilak Kumar wrote:
> Fabric Discovery and Login Services (FDLS) provide
> functionality for fnic to discover the fabric and
> target ports. It logs in to the target ports from
> the initiator and creates target ports (tports).
> This functionality is essential for port channel
> register state change notification (PC-RSCN)
> handling and FC-NVME initiator role.
> 
> This functionality is essential for eCPU hang or panic
> handling. In cases where the eCPU in the UCS VIC (Unified
> Computing Services Virtual Interface Card) hangs,
> a fabric log out is sent to the fabric. Upon successful log
> out from the fabric, the IO path is failed over to a new path.
> 
Didn't you have the paragraph in the patch series description?
Please don't duplicate it.

> Add headers and definitions for FDLS.
> 
> Reviewed-by: Sesidhar Baddela <sebaddel@cisco.com>
> Reviewed-by: Arulprabhu Ponnusamy <arulponn@cisco.com>
> Reviewed-by: Gian Carlo Boffa <gcboffa@cisco.com>
> Signed-off-by: Karan Tilak Kumar <kartilak@cisco.com>
> ---
>   drivers/scsi/fnic/fdls_fc.h   | 548 ++++++++++++++++++++++++++++++++++
>   drivers/scsi/fnic/fnic_fdls.h | 362 ++++++++++++++++++++++
>   2 files changed, 910 insertions(+)
>   create mode 100644 drivers/scsi/fnic/fdls_fc.h
>   create mode 100644 drivers/scsi/fnic/fnic_fdls.h
> 
> diff --git a/drivers/scsi/fnic/fdls_fc.h b/drivers/scsi/fnic/fdls_fc.h
> new file mode 100644
> index 000000000000..ad6a9eebc5d3
> --- /dev/null
> +++ b/drivers/scsi/fnic/fdls_fc.h
> @@ -0,0 +1,548 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright 2008 Cisco Systems, Inc.  All rights reserved.
> + * Copyright 2007 Nuova Systems, Inc.  All rights reserved.
> + */
> +
> +#ifndef _FDLS_FC_H_
> +#define _FDLS_FC_H_
> +
> +/* This file contains the declarations for FC fabric services
> + * and target discovery
> + *
> + * Request and Response for
> + * 1. FLOGI
> + * 2. PLOGI to Fabric Controller
> + * 3. GPN_ID, GPN_FT
> + * 4. RSCN
> + * 5. PLOGI to Target
> + * 6. PRLI to Target
> + */
> +
> +#include <scsi/scsi.h>
> +
> +#define MIN(x, y) (x < y ? x : y)
> +
> +#define FNIC_FCP_SP_RD_XRDY_DIS 0x00000002
> +#define FNIC_FCP_SP_TARGET      0x00000010
> +#define FNIC_FCP_SP_INITIATOR   0x00000020
> +#define FNIC_FCP_SP_CONF_CMPL   0x00000080
> +#define FNIC_FCP_SP_RETRY       0x00000100
> +
> +#ifdef _BIG_ENDIAN
> +
> +#define FNIC_FLOGI_OXID        (0x1001)
> +#define FNIC_PLOGI_FABRIC_OXID (0x1002)
> +#define FNIC_RPN_REQ_OXID      (0x1003)
> +#define FNIC_GPN_FT_OXID       (0x1004)
> +#define FNIC_SCR_REQ_OXID      (0x1005)
> +#define FNIC_RSCN_RESP_OXID    (0x1006)
> +#define FNIC_LOGO_REQ_OXID     (0x1007)
> +#define FNIC_LOGO_RESP_OXID    (0x1008)
> +#define FNIC_RFT_REQ_OXID      (0x100a)
> +#define FNIC_RFF_REQ_OXID      (0x100b)
> +#define FNIC_ECHO_RESP_OXID    (0x100c)
> +#define FNIC_ADISC_RESP_OXID   (0x100d)
> +#define FNIC_FDMI_PLOGI_OXID   (0x100e)
> +#define FNIC_FDMI_REG_HBA_OXID (0X100f)
> +#define FNIC_FDMI_RPA_OXID     (0X1010)

Please tell me that 'OXID' doesn't mean what I think it does ...
Hardcoded OXIDs for individual command types? Really?

> +#define FNIC_ELS_REQ_FCTL      (0x290000)
> +#define FNIC_ELS_REP_FCTL      (0x990000)
> +
> +#define FNIC_FC_PH_VER         (0x2020)
> +#define FNIC_FC_B2B_CREDIT     (0x000A)
> +#define FNIC_FC_B2B_RDF_SZ     (0x0800)
> +
> +#define FNIC_REQ_ABTS_FCTL     (0x090000)
> +
> +#define FNIC_FC_FEATURES       (0x8000)
> +
> +#define FNIC_FC_CONCUR_SEQS    (0x00FF)
> +
> +#define FNIC_FC_RO_INFO        (0x001F)
> +#define FNIC_E_D_TOV           (0x07D0)
> +
> +#define FC_CT_RPN_CMD          (0x0212)
> +#define FC_CT_GPN_FT_CMD       (0x0172)
> +#define FC_CT_ACC              (0x8002)
> +#define FC_CT_REJ              (0x8001)
> +#define FC_CT_RFT_CMD          (0x1702)
> +#define FC_CT_RFF_CMD          (0x1F02)
> +
> +#define FNIC_FCP_RSP_FCTL      (0x990000)
> +
> +#else /* _LITTLE_ENDIAN */
> +
> +#define FNIC_FLOGI_OXID        (0x0110)
> +#define FNIC_PLOGI_FABRIC_OXID (0x0210)
> +#define FNIC_RPN_REQ_OXID      (0x0310)
> +#define FNIC_GPN_FT_OXID       (0x0410)
> +#define FNIC_SCR_REQ_OXID      (0x0510)
> +#define FNIC_RSCN_RESP_OXID    (0x0610)
> +#define FNIC_TLOGO_REQ_OXID    (0x0710)
> +#define FNIC_FLOGO_REQ_OXID    (0x0711)
> +#define FNIC_LOGO_RESP_OXID    (0x0810)
> +#define FNIC_PLOGI_RESP_OXID   (0x0910)
> +#define FNIC_RFT_REQ_OXID      (0x0a10)
> +#define FNIC_RFF_REQ_OXID      (0x0b10)
> +#define FNIC_ECHO_RESP_OXID    (0x0c10)
> +#define FNIC_UNSUPPORTED_RESP_OXID   (0xffff)
> +#define FNIC_ADISC_RESP_OXID    (0x0d10)
> +#define FNIC_PLOGI_FDMI_OXID    (0x0e10)
> +#define FNIC_FDMI_REG_HBA_OXID  (0X0f10)
> +#define FNIC_FDMI_RPA_OXID      (0X1010)
> +#define FNIC_ELS_REQ_FCTL      (0x000029)
> +#define FNIC_ELS_REP_FCTL      (0x000099)
> +
> +#define FNIC_FCP_RSP_FCTL      (0x000099)
> +#define FNIC_REQ_ABTS_FCTL     (0x000009)
> +
> +#define FNIC_FC_PH_VER         (0x2020)
> +#define FNIC_FC_B2B_CREDIT     (0x0A00)
> +#define FNIC_FC_B2B_RDF_SZ     (0x0008)
> +
> +#define FNIC_FC_FEATURES       (0x0080)
> +
> +#define FNIC_FC_CONCUR_SEQS    (0xFF00)
> +#define FNIC_FC_RO_INFO        (0x1F00)
> +#define FNIC_E_D_TOV           (0xD0070000)
> +
> +#define FC_CT_RPN_CMD          (0x1202)
> +#define FC_CT_GPN_FT_CMD       (0x7201)
> +#define FC_CT_ACC              (0x0280)
> +#define FC_CT_REJ              (0x0180)
> +#define FC_CT_RFT_CMD          (0x1702)
> +#define FC_CT_RFF_CMD          (0x1F02)
> +
> +#endif

Duplicate macros for endian differences is quite error-prone,
as you have to remember to always change both sides.
I would prefer to have just one copy and use macros
to access the values (eg always use get_unaligned_le16).

> +
> +#define ETH_TYPE_FCOE			0x8906
> +#define ETH_TYPE_FIP			0x8914
> +
> +#define FC_DIR_SERVER          0xFFFFFC
> +#define FC_FABRIC_CONTROLLER   0xFFFFFD
> +#define FC_DOMAIN_CONTR        0xFFFFFE
> +
> +#define FNIC_FC_GPN_LAST_ENTRY (0x80)
> +
> +#define FC_ELS_FLOGI_REQ        0x04
> +#define FC_LS_REJ               0x01
> +#define FC_LS_ACC               0x02
> +#define FC_ELS_PLOGI_REQ        0x03
> +#define FC_ELS_ECHO_REQ         0x10
> +#define FC_ELS_PRLI_REQ         0x20
> +#define FC_ELS_SCR              0x62
> +#define FC_ELS_RLS_REQ          0x0F
> +#define FC_ELS_RRQ_REQ          0x12
> +#define FC_ELS_LOGO             0x05
> +#define FC_ELS_RSCN             0x61
> +#define FNIC_BA_ACC_RCTL        0x84
> +#define FNIC_BA_RJT_RCTL        0x85
> +#define FC_ABTS_RCTL            0x81
> +#define FNIC_ELS_ADISC_REQ      0x52
> +#define FC_ELS_RJT_LOGICAL_BUSY 0x05
> +#define FC_ELS_RJT_BUSY         0x09
> +#define FC_ELS_RTV_REQ          0x0E
> +

There is already a copy of FC-LS definitions in
include/uapi/scsi/fc/fc_els.h.

Please ensure that you don't introduce duplicates.

> +/* FNIC FDMI Register HBA Macros */
> +#define FNIC_FDMI_TYPE_NODE_NAME	0X100
> +#define FNIC_FDMI_TYPE_MANUFACTURER	0X200
> +#define FNIC_FDMI_MANUFACTURER		"Cisco Systems"
> +#define FNIC_FDMI_TYPE_SERIAL_NUMBER	0X300
> +#define FNIC_FDMI_TYPE_MODEL		0X400
> +#define FNIC_FDMI_TYPE_MODEL_DES	0X500
> +#define FNIC_FDMI_MODEL_DESCRIPTION	"Cisco Virtual Interface Card"
> +#define FNIC_FDMI_TYPE_HARDWARE_VERSION	0X600
> +#define FNIC_FDMI_TYPE_DRIVER_VERSION	0X700
> +#define FNIC_FDMI_TYPE_ROM_VERSION	0X800
> +#define FNIC_FDMI_TYPE_FIRMWARE_VERSION	0X900
> +
> +/* FNIC FDMI Register PA Macros */
> +#define FNIC_FDMI_TYPE_FC4_TYPES	0X100
> +#define FNIC_FDMI_TYPE_SUPPORTED_SPEEDS 0X200
> +#define FNIC_FDMI_TYPE_CURRENT_SPEED	0X300
> +#define FNIC_FDMI_TYPE_MAX_FRAME_SIZE	0X400
> +#define FNIC_FDMI_TYPE_OS_NAME		0X500
> +#define FNIC_FDMI_TYPE_HOST_NAME	0X600
> +
> +#define FNIC_SET_S_ID(_fchdr, _sid)        memcpy(_fchdr->sid, _sid, 3)
> +#define FNIC_SET_NPORT_NAME(_req, _pName)  (_req.nport_name = htonll(_pName))
> +#define FNIC_SET_NODE_NAME(_req, _pName)   (_req.node_name = htonll(_pName))
> +#define FNIC_SET_RDF_SIZE(_req, _rdf_size)	\
> +	(_req.b2b_rdf_size = htons(_rdf_size))
> +#define FNIC_SET_R_A_TOV(_req, _r_a_tov)   (_req.r_a_tov = htonl(_r_a_tov))
> +#define FNIC_SET_E_D_TOV(_req, _e_d_tov)   (_req.e_d_tov = htonl(_e_d_tov))
> +#define FNIC_SET_D_ID(_fchdr, _did)        memcpy(_fchdr->did, _did, 3)
> +#define FNIC_SET_OX_ID(_fchdr, _oxid)      (_fchdr->ox_id = _oxid)
> +#define FNIC_SET_RX_ID(_fchdr, _rxid)      (_fchdr->rx_id = _rxid)
> +
htonll() macros are typically used in the network stack; please use
get_unaligned_be64().

> +#define FNIC_SET_PORT_ID(__req, __portid) \
> +	memcpy(__req->port_id, __portid, 3)
> +#define FNIC_SET_RPN_PORT_ID(__req, __portid) \
> +	memcpy(__req->port_id, __portid, 3)
> +#define FNIC_SET_RPN_PORT_NAME(_req, _pName) \
> +	(_req->port_name = htonll(_pName))
> +
> +#define FNIC_GET_S_ID(_fchdr)        (_fchdr->sid)
> +#define FNIC_GET_D_ID(_fchdr)        (_fchdr->did)
> +#define FNIC_GET_OX_ID(_fchdr)       (_fchdr->ox_id)
> +
> +#define FNIC_GET_FC_CT_CMD(__fcct_hdr)  (__fcct_hdr->command)
> +
> +#define FNIC_FCOE_SOF         (0x2E)
> +#define FNIC_FCOE_EOF         (0x42)
> +
> +#define FNIC_GET_FC_TYPE(_fchdr)        (_fchdr->type)
> +#define FNIC_GET_FC_RCTL(_fchdr)        (_fchdr->r_ctl)
> +
> +#define FNIC_FC_TYPE_ELS        (0x01)
> +#define FNIC_FC_R_ELS_REQ       (0x22)
> +#define FNIC_FC_R_ELS_RSP       (0x23)
> +
> +#define FNIC_FCOE_MAX_FRAME_SZ  (2048)
> +#define FNIC_FCOE_MIN_FRAME_SZ  (280)
> +#define FNIC_FC_MAX_PAYLOAD_LEN (2048)
> +#define FNIC_MIN_DATA_FIELD_SIZE  (256)
> +#define FNIC_R_A_TOV_DEF        (10 * 1000) /* msec */
> +#define FNIC_E_D_TOV_DEF        (2 * 1000)  /* msec */
> +
> +#define FNIC_FC_EDTOV_NSEC    (0x400)
> +#define FNIC_NSEC_TO_MSEC     (0x1000000)
> +#define FCP_PRLI_FUNC_TARGET	(0x0010)
> +#define FC_CT_RJT_LOGICAL_BUSY 0x5
> +#define FC_CT_RJT_BUSY         0x9
> +
> +#define FNIC_FC_FRAME_UNSOLICITED(_fchdr)  (_fchdr->r_ctl == 0x22)
> +#define FNIC_FC_FRAME_SOLICITED_DATA(_fchdr)    (_fchdr->r_ctl == 0x21)
> +#define FNIC_FC_FRAME_SOLICITED_CTRL_REPLY(_fchdr)    (_fchdr->r_ctl == 0x23)
> +#define FNIC_FC_FRAME_FCTL_LAST_END_SEQ(_fchdr)    (_fchdr->f_ctl == 0x98)
> +#define FNIC_FC_FRAME_FCTL_LAST_END_SEQ_INT(_fchdr)    (_fchdr->f_ctl == 0x99)
> +#define FNIC_FC_FRAME_FCTL_FIRST_LAST_SEQINIT(_fchdr)  (_fchdr->f_ctl == 0x29)
> +#define FNIC_FC_FRAME_FC4_SCTL(_fchdr)    (_fchdr->r_ctl == 0x03)
> +#define FNIC_FC_FRAME_TYPE_BLS(_fchdr) (_fchdr->type == 0x00)
> +#define FNIC_FC_FRAME_TYPE_ELS(_fchdr) (_fchdr->type == 0x01)
> +#define FNIC_FC_FRAME_TYPE_FC_GS(_fchdr) (_fchdr->type == 0x20)
> +#define FNIC_FC_FRAME_CS_CTL(_fchdr) (_fchdr->cs_ctl == 0x00)

Please use macro names instead of raw values here.

> +
> +#define FNIC_FC_C3_RDF         (0xfff)
> +#define FNIC_FC_PLOGI_RSP_RDF(_plogi_rsp) \
> +	(MIN(_plogi_rsp->u.csp_plogi.b2b_rdf_size, \
> +	(_plogi_rsp->spc3[4] & FNIC_FC_C3_RDF)))
> +#define FNIC_FC_PLOGI_RSP_CONCUR_SEQ(_plogi_rsp) \
> +	(MIN(_plogi_rsp->u.csp_plogi.total_concur_seqs, \
> +	(uint8_t)(_plogi_rsp->spc3[10] & 0xff)))
> +
> +/* Frame header */
> +
> +struct fnic_eth_hdr_s {
> +	uint8_t		dst_mac[6];
> +	uint8_t		src_mac[6];
> +	uint16_t	ether_type;
> +}  __packed;
> +
> +struct	fnic_fcoe_hdr_s	{
> +	uint8_t		ver;
> +	uint8_t		rsvd[12];
> +	uint8_t		sof;
> +} __packed;
> +
> +/*	Big	endian	*/
> +struct	fc_hdr_s	{
> +	uint8_t		r_ctl;
> +	uint8_t		did[3];
> +	uint8_t		cs_ctl:8;
> +	uint8_t		sid[3];
> +	uint32_t	type:8;
> +	uint32_t	f_ctl:24;
> +	uint8_t		seq_id;
> +	uint8_t		df_ctl;
> +	uint16_t	seq_cnt;
> +	uint16_t	ox_id;
> +	uint16_t	rx_id;
> +	uint32_t	param;
> +} __packed;
> +
> +struct	fc_csp_flogi_s	{
> +	uint16_t	fc_ph_ver;
> +	uint16_t	b2b_credits;
> +	uint16_t	features;
> +	uint16_t	b2b_rdf_size;
> +	uint32_t	r_a_tov;
> +	uint32_t	e_d_tov;
> +} __packed;
> +
> +struct	fc_csp_plogi_s	{
> +	uint16_t	fc_ph_ver;
> +	uint16_t	b2b_credits;
> +	uint16_t	features;
> +	uint16_t	b2b_rdf_size;
> +	uint16_t	total_concur_seqs;
> +	uint16_t	ro_info;
> +	uint32_t	e_d_tov;
> +} __packed;
> +
> +struct	fc_els_s	{
> +	struct	fc_hdr_s	fchdr;
> +	uint8_t		command;
> +	uint8_t		rsvd[3];
> +	union	{
> +	struct	fc_csp_flogi_s	csp_flogi;
> +	struct	fc_csp_plogi_s	csp_plogi;
> +	}	u;
> +	uint64_t	nport_name;
> +	uint64_t	node_name;
> +	uint8_t		spc1[16];
> +	uint8_t		spc2[16];
> +	uint8_t		spc3[16];
> +	uint8_t		spc4[16];
> +	uint8_t		vendor_ver_level[16];
> +}	 __packed;
> +
> +struct	fc_els_acc_s	{
> +	struct	fc_hdr_s	fchdr;
> +	uint8_t		command;
> +	uint8_t		rsvd[3];
> +}	 __packed;
> +
> +struct	fc_els_reject_s	{
> +	struct	fc_hdr_s	fchdr;
> +	uint32_t	command;
> +	uint8_t		reserved;
> +	uint8_t		reason_code;
> +	uint8_t		reason_expl;
> +	uint8_t		vendor_specific;
> +}	 __packed;
> +
> +struct	fc_els_adisc_s	{
> +	struct	fc_hdr_s	fchdr;
> +	uint8_t		command;
> +	uint8_t		zeros[3];
> +	uint32_t	unused;
> +	uint64_t	nport_name;
> +	uint64_t	node_name;
> +	uint8_t		reserved;
> +	uint8_t		fcid[3];
> +} __packed;
> +
> +struct	fc_els_rls_ls_acc_s	{
> +	struct	fc_hdr_s	fchdr;
> +	uint8_t		command;
> +	uint8_t		reserved[3];
> +	uint32_t	link_fail_count;	/*	link	failure	count	*/
> +	uint32_t	sync_loss_count;	/*	loss	of	synchronization	count	*/
> +	uint32_t	sig_loss_count;	/*	loss	of	signal	count	*/
> +	uint32_t	prim_err_count;	/*	primitive	sequence	error	count	*/
> +	uint32_t	inv_word_count;	/*	invalid	transmission	word	count	*/
> +	uint32_t	inv_crc_count;	/*	invalid	CRC	count	*/
> +} __packed;
> +
> +struct	fc_els_adisc_ls_acc_s	{
> +	struct	fc_hdr_s	fchdr;
> +	uint8_t		command;
> +	uint8_t		zeros[3];
> +	uint32_t	unused;
> +	uint64_t	nport_name;
> +	uint64_t	node_name;
> +	uint8_t		reserved;
> +	uint8_t		fcid[3];
> +} __packed;
> +
> +struct	fc_abts_ba_acc_s	{
> +	struct	fc_hdr_s	fchdr;
> +	uint8_t		seq_id_validity;
> +	uint8_t		seq_id;
> +	uint16_t	reserved;
> +	uint16_t	ox_id;
> +	uint16_t	rx_id;
> +	uint16_t	low_seq_cnt;
> +	uint16_t	high_seq_cnt;
> +} __packed;
> +
> +struct	fc_abts_ba_rjt_s	{
> +	struct	fc_hdr_s	fchdr;
> +	uint8_t		vend_uniq;
> +	uint8_t		reason_explanation;
> +	uint8_t		reason_code;
> +	uint8_t		reserved;
> +} __packed;
> +
> +struct	fc_prli_sp_s	{
> +	uint8_t		type;
> +	uint8_t		type_ext;
> +	uint16_t	flags;
> +
> +	uint32_t	ox_proc_assoc;
> +	uint32_t	rx_proc_assoc;
> +	uint32_t	csp;
> +}	 __packed;
> +
> +struct	fc_els_prli_s	{
> +	struct	fc_hdr_s	fchdr;
> +	uint8_t		command;
> +	uint8_t		page_len;
> +	uint16_t	payload_len;
> +	struct	fc_prli_sp_s	sp;
> +}	 __packed;
> +
> +struct	fc_ct_hdr_s	{
> +	uint32_t	rev:	8;
> +	uint32_t	in_id:	24;
> +	uint8_t		fs_type;
> +	uint8_t		fs_subtype;
> +	uint8_t		options;
> +	uint8_t		rsvd;
> +	uint16_t	command;
> +	uint16_t	max_res_size;
> +	uint8_t		rsvd1;
> +	uint8_t		reason_code;
> +	uint8_t		reason_expl;
> +	uint8_t		vendor_specific;
> +}	 __packed;
> +
> +struct	fc_rpn_id_s	{
> +	struct	fc_hdr_s	fchdr;
> +	struct	fc_ct_hdr_s	fc_ct_hdr;
> +	uint8_t		rsvd;
> +	uint8_t		port_id[3];
> +	uint64_t	port_name;
> +}	 __packed;
> +
> +struct	fc_fdmi_rhba_s	{
> +	struct	fc_hdr_s	fchdr;
> +	struct	fc_ct_hdr_s	fc_ct_hdr;
> +	uint64_t	hba_identifier;
> +	uint32_t	num_ports;
> +	uint64_t	port_name;
> +	uint32_t	num_hba_attributes;
> +	uint16_t	type_nn;
> +	uint16_t	length_nn;
> +	uint64_t	node_name;
> +	uint16_t	type_manu;
> +	uint16_t	length_manu;
> +	uint8_t		manufacturer[20];
> +	uint16_t	type_serial;
> +	uint16_t	length_serial;
> +	uint8_t		serial_num[16];
> +	uint16_t	type_model;
> +	uint16_t	length_model;
> +	uint8_t		model[12];
> +	uint16_t	type_model_des;
> +	uint16_t	length_model_des;
> +	uint8_t		model_description[56];
> +	uint16_t	type_hw_ver;
> +	uint16_t	length_hw_ver;
> +	uint8_t		hardware_ver[16];
> +	uint16_t	type_dr_ver;
> +	uint16_t	length_dr_ver;
> +	uint8_t		driver_ver[28];
> +	uint16_t	type_rom_ver;
> +	uint16_t	length_rom_ver;
> +	uint8_t		rom_ver[8];
> +	uint16_t	type_fw_ver;
> +	uint16_t	length_fw_ver;
> +	uint8_t		firmware_ver[16];
> +} __packed;
> +
> +struct	fc_fdmi_rpa_s	{
> +	struct	fc_hdr_s	fchdr;
> +	struct	fc_ct_hdr_s	fc_ct_hdr;
> +	uint64_t	port_name;
> +	uint32_t	num_port_attributes;
> +	uint16_t	type_fc4;
> +	uint16_t	length_fc4;
> +	uint8_t		fc4_type[32];
> +	uint16_t	type_supp_speed;
> +	uint16_t	length_supp_speed;
> +	uint32_t	supported_speed;
> +	uint16_t	type_cur_speed;
> +	uint16_t	length_cur_speed;
> +	uint32_t	current_speed;
> +	uint16_t	type_max_frame_size;
> +	uint16_t	length_max_frame_size;
> +	uint32_t	max_frame_size;
> +	uint16_t	type_os_name;
> +	uint16_t	length_os_name;
> +	uint8_t		os_name[16];
> +	uint16_t	type_host_name;
> +	uint16_t	length_host_name;
> +	uint8_t		host_name[12];
> +}	 __packed;
> +
> +struct	fc_rft_id	{
> +	struct	fc_hdr_s fchdr;
> +	struct	fc_ct_hdr_s	fc_ct_hdr;
> +	uint8_t		rsvd;
> +	uint8_t		port_id[3];
> +	uint8_t		fc4_types[32];
> +} __packed;
> +
> +struct	fc_rff_id	{
> +	struct	fc_hdr_s fchdr;
> +	struct	fc_ct_hdr_s	fc_ct_hdr;
> +	uint8_t		rsvd;
> +	uint8_t		port_id[3];
> +	uint8_t		rsvd1;
> +	uint8_t		rsvd2;
> +	uint8_t		tgt;
> +	uint8_t		fc4_type;
> +} __packed;
> +
> +/*
> + *	Variables:
> + *	sid
> + */
> +struct	fc_gpn_ft_s	{
> +	struct	fc_hdr_s	fchdr;
> +	struct	fc_ct_hdr_s	fc_ct_hdr;
> +	uint8_t		rsvd[3];
> +	uint8_t		fc4_type;
> +} __packed;
> +
> +/* Accept CT_IU	for	GPN_FT	*/
> +struct	fc_gpn_ft_rsp_iu_s	{
> +	uint8_t		ctrl;
> +	uint8_t		fcid[3];
> +	uint32_t	rsvd;
> +	uint64_t	wwpn;
> +} __packed;
> +
> +/*
> + *	Variables:
> + *	sid
> + */
> +struct	fc_scr_s	{
> +	struct	fc_hdr_s	fchdr;
> +	uint8_t		command;
> +	uint8_t		rsvd[3];
> +	uint8_t		rsvd1[3];
> +	uint8_t		reg_func;
> +} __packed;
> +
> +struct	fc_rscn_hdr_s	{
> +	struct	fc_hdr_s	fchdr;
> +	uint8_t		command;
> +	uint8_t		page_len;
> +	uint16_t	payload_len;
> +} __packed;
> +
> +
> +struct	fc_rscn_port_s	{
> +	uint8_t		addr_format:2;
> +	uint8_t		rscn_evt_q:4;
> +	uint8_t		reserved:2;
> +	uint8_t		port_id[3];
> +} __packed;
> +
> +struct	fc_logo_req_s	{
> +	struct fc_hdr_s	fchdr;
> +	uint8_t		command;
> +	uint8_t		rsvd[3];
> +	uint8_t		rsvd1;
> +	uint8_t		fcid[3];
> +	uint64_t	wwpn;
> +} __packed;
> +
These seem to be standard FCoE frame definitions, for which we
already have a copy in include/uapi/scsi/fc.
Please use the definitions from there instead of introducing your own.

> +#define	FNIC_FCOE_FCHDR_OFFSET	\
> +	(sizeof(struct	fnic_eth_hdr_s)	+	sizeof(struct	fnic_fcoe_hdr_s))
> +
> +#endif	/*	_FDLS_FC_H	*/
> diff --git a/drivers/scsi/fnic/fnic_fdls.h b/drivers/scsi/fnic/fnic_fdls.h
> new file mode 100644
> index 000000000000..fddb9390d022
> --- /dev/null
> +++ b/drivers/scsi/fnic/fnic_fdls.h
> @@ -0,0 +1,362 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright 2008 Cisco Systems, Inc.  All rights reserved.
> + * Copyright 2007 Nuova Systems, Inc.  All rights reserved.
> + */
> +
> +#ifndef _FNIC_FDLS_H_
> +#define _FNIC_FDLS_H_
> +
> +#include "fnic_stats.h"
> +#include "fdls_fc.h"
> +
> +/* FDLS - Fabric discovery and login services
> + * -> VLAN discovery
> + *   -> retry every retry delay seconds until it succeeds.
> + *                        <- List of VLANs
> + *
> + * -> Solicitation
> + *                        <- Solicitation response (Advertisement)
> + *
> + * -> FCF selection & FLOGI ( FLOGI timeout - 2 * E_D_TOV)
> + *                        <- FLOGI response
> + *
> + * -> FCF keep alive
> + *                         <- FCF keep alive
> + *
> + * -> PLOGI to FFFFFC (DNS) (PLOGI timeout - 2 * R_A_TOV)
> + *    -> ABTS if timeout (ABTS tomeout - 2 * R_A_TOV)
> + *                        <- PLOGI response
> + *    -> Retry PLOGI to FFFFFC (DNS) - Number of retries from vnic.cfg
> + *
> + * -> SCR to FFFFFC (DNS) (SCR timeout - 2 * R_A_TOV)
> + *    -> ABTS if timeout (ABTS tomeout - 2 * R_A_TOV)
> + *                        <- SCR response
> + *    -> Retry SCR - Number of retries 2
> + *
> + * -> GPN_FT to FFFFFC (GPN_FT timeout - 2 * R_A_TOV)a
> + *    -> Retry on BUSY until it succeeds
> + *    -> Retry on BUSY until it succeeds
> + *    -> 2 retries on timeout
> + *
> + * -> RFT_ID to FFFFFC (DNS)        (RFT_ID timeout - 3 * R_A_TOV)
> + *    -> ABTS if timeout (ABTS tomeout - 2 * R_A_TOV)
> + *    -> Retry RFT_ID to FFFFFC (DNS) (Number of retries 2 )
> + *    -> Ignore if both retires fail.
> + *
> + *        Session establishment with targets
> + * For each PWWN
> + *   -> PLOGI to FCID of that PWWN (PLOGI timeout 2 * R_A_TOV)
> + *    -> ABTS if timeout (ABTS tomeout - 2 * R_A_TOV)
> + *                        <- PLOGI response
> + *    -> Retry PLOGI. Num retries using vnic.cfg
> + *
> + *   -> PRLI to FCID of that PWWN (PRLI timeout 2 * R_A_TOV)
> + *    -> ABTS if timeout (ABTS tomeout - 2 * R_A_TOV)
> + *                        <- PRLI response
> + *    -> Retry PRLI. Num retries using vnic.cfg
> + *
> + */
> +
> +#define FDLS_RETRY_COUNT 2
> +
> +#define FDLS_TGT_OXID_BLOCK_SZ  (0x200)
> +#define FDLS_PLOGI_OXID_BASE    (0x2000)
> +#define FDLS_PRLI_OXID_BASE     (0x2200)
> +#define FDLS_ADISC_OXID_BASE    (0x2400)
> +#define FDLS_TGT_OXID_POOL_END  (FDLS_PLOGI_OXID_BASE + FDLS_TGT_OXID_POOL_SZ)
> +#define FDLS_TGT_OXID_POOL_SZ   (0x800)
> +
> +#define FNIC_FDLS_FABRIC_ABORT_ISSUED     0x1
> +#define FNIC_FDLS_FPMA_LEARNT             0x2
> +
> +/* tport flags */
> +#define FNIC_FDLS_TPORT_IN_GPN_FT_LIST 0x1
> +#define FNIC_FDLS_TGT_ABORT_ISSUED     0x2
> +#define FNIC_FDLS_TPORT_SEND_ADISC     0x4
> +#define FNIC_FDLS_RETRY_FRAME          0x8
> +#define FNIC_FDLS_TPORT_BUSY	       0x10
> +#define FNIC_FDLS_TPORT_TERMINATING      0x20
> +#define FNIC_FDLS_TPORT_DELETED        0x40
> +#define FNIC_FDLS_SCSI_REGISTERED      0x200
> +#define FNIC_TPORT_CAN_BE_FREED        0x400
> +
> +/* Retry supported by rport(returned by prli service parameters) */
> +#define FDLS_FC_RP_FLAGS_RETRY 0x1
> +
> +#define fdls_set_state(_fdls_fabric, _state)  ((_fdls_fabric)->state = _state)
> +#define fdls_get_state(_fdls_fabric)          ((_fdls_fabric)->state)
> +
> +#define FNIC_FDMI_ACTIVE    0x8
> +#define FNIC_FIRST_LINK_UP    0x2
> +
> +#define fdls_set_tport_state(_tport, _state)    (_tport->state = _state)
> +#define fdls_get_tport_state(_tport)            (_tport->state)
> +
> +#define fnic_del_fabric_timer_sync() {							\
> +	iport->fabric.del_timer_inprogress = 1;						\
> +	spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);	\
> +	del_timer_sync(&iport->fabric.retry_timer);					\
> +	spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);		\
> +	iport->fabric.del_timer_inprogress = 0;						\
> +}
> +
Please, make this an inline function.

> +#define fnic_del_tport_timer_sync() {							\
> +	tport->del_timer_inprogress = 1;							\
> +	spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);	\
> +	del_timer_sync(&tport->retry_timer);						\
> +	spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);		\
> +	tport->del_timer_inprogress = 0;							\
> +}
> +
Same here.

> +#define FNIC_PORTSPEED_10GBIT   1
> +#define FNIC_FRAME_HT_ROOM     (2148)
> +#define FNIC_FCOE_FRAME_MAXSZ   (2112)
> +
> +struct fnic_fip_fcf_s {
> +	uint16_t vlan_id;
> +	uint8_t fcf_mac[6];
> +	uint8_t fcf_priority;
> +	uint32_t fka_adv_period;
> +	uint8_t ka_disabled;
> +};
> +
> +enum fnic_fdls_state_e {
> +	FDLS_STATE_INIT = 0,
> +	FDLS_STATE_LINKDOWN,
> +	FDLS_STATE_FABRIC_LOGO,
> +	FDLS_STATE_FLOGO_DONE,
> +	FDLS_STATE_FABRIC_FLOGI,
> +	FDLS_STATE_FABRIC_PLOGI,
> +	FDLS_STATE_RPN_ID,
> +	FDLS_STATE_REGISTER_FC4_TYPES,
> +	FDLS_STATE_REGISTER_FC4_FEATURES,
> +	FDLS_STATE_SCR,
> +	FDLS_STATE_GPN_FT,
> +	FDLS_STATE_TGT_DISCOVERY,
> +	FDLS_STATE_RSCN_GPN_FT,
> +	FDLS_STATE_SEND_GPNFT
> +};
> +
> +struct fnic_fdls_fabric_s {
> +	enum fnic_fdls_state_e state;
> +	uint32_t flags;
> +	struct list_head tport_list; /* List of discovered tports */
> +	struct timer_list retry_timer;
> +	int del_timer_inprogress;
> +	int del_fdmi_timer_inprogress;
> +	int retry_counter;
> +	int timer_pending;
> +	int fdmi_retry;
> +	struct timer_list fdmi_timer;
> +	int fdmi_pending;
> +};
> +
> +struct fnic_fdls_fip_s {
> +	uint32_t state;
> +	uint32_t flogi_retry;
> +};
> +
> +/* Message to tport_event_handler */
> +enum fnic_tgt_msg_id {
> +	TGT_EV_NONE = 0,
> +	TGT_EV_RPORT_ADD,
> +	TGT_EV_RPORT_DEL,
> +	TGT_EV_TPORT_DELETE,
> +	TGT_EV_REMOVE
> +};
> +
> +struct fnic_tport_event_s {
> +	struct list_head links;
> +	enum fnic_tgt_msg_id event;
> +	void *arg1;
> +};
> +
> +enum fdls_tgt_state_e {
> +	FDLS_TGT_STATE_INIT = 0,
> +	FDLS_TGT_STATE_PLOGI,
> +	FDLS_TGT_STATE_PRLI,
> +	FDLS_TGT_STATE_READY,
> +	FDLS_TGT_STATE_LOGO_RECEIVED,
> +	FDLS_TGT_STATE_ADISC,
> +	FDL_TGT_STATE_PLOGO,
> +	FDLS_TGT_STATE_OFFLINING,
> +	FDLS_TGT_STATE_OFFLINE
> +};
> +
> +struct fnic_tport_s {
> +	struct list_head links; /* To link the tports */
> +	enum fdls_tgt_state_e state;
> +	uint32_t flags;
> +	uint32_t fcid;
> +	uint64_t wwpn;
> +	uint64_t wwnn;
> +	uint16_t oxid_used;
> +	uint16_t tgt_flags;
> +	atomic_t in_flight; /* io counter */
> +	uint16_t max_payload_size;
> +	uint16_t r_a_tov;
> +	uint16_t e_d_tov;
> +	uint16_t lun0_delay;
> +	int max_concur_seqs;
> +	uint32_t fcp_csp;
> +	struct timer_list retry_timer;
> +	int del_timer_inprogress;
> +	int retry_counter;
> +	int timer_pending;
> +	unsigned int num_pending_cmds;
> +	int nexus_restart_count;
> +	int exch_reset_in_progress;
> +	void *iport;
> +	struct work_struct tport_del_work;
> +	struct completion *tport_del_done;
> +	struct delayed_work tport_scan_work;
> +	struct fc_rport *rport;
> +	char str_wwpn[20];
> +	char str_wwnn[20];
> +};
> +
> +/* iport */
> +enum fnic_iport_state_e {
> +	FNIC_IPORT_STATE_INIT = 0,
> +	FNIC_IPORT_STATE_LINK_WAIT,
> +	FNIC_IPORT_STATE_FIP,
> +	FNIC_IPORT_STATE_FABRIC_DISC,
> +	FNIC_IPORT_STATE_READY
> +};
> +
> +struct fnic_iport_s {
> +	enum fnic_iport_state_e state;
> +	struct fnic *fnic;
> +	uint64_t boot_time;
> +	uint32_t flags;
> +	int usefip;
> +	uint8_t hwmac[6]; /* HW MAC Addr */
> +	uint8_t fpma[6]; /* Fabric Provided MA */
> +	uint8_t fcfmac[6]; /* MAC addr of Fabric */
> +	uint16_t vlan_id;
> +	uint32_t fcid;
> +	uint8_t tgt_oxid_pool[FDLS_TGT_OXID_POOL_SZ];
> +	struct fnic_fip_fcf_s selected_fcf;
> +	struct fnic_fdls_fip_s fip;
> +	struct fnic_fdls_fabric_s fabric;
> +	struct list_head tport_list;
> +	struct list_head tport_list_pending_del;
> +	/* list of tports for which we are yet to send PLOGO */
> +	struct list_head inprocess_tport_list;
> +	struct list_head deleted_tport_list;
> +	struct work_struct tport_event_work;
> +	uint32_t e_d_tov; /* msec */
> +	uint32_t r_a_tov; /* msec */
> +	uint32_t link_supported_speeds;
> +	uint32_t max_flogi_retries;
> +	uint32_t max_plogi_retries;
> +	uint32_t plogi_timeout;
> +	uint32_t service_params;
> +	uint64_t wwpn;
> +	uint64_t wwnn;
> +	uint16_t max_payload_size;
> +	spinlock_t deleted_tport_lst_lock;
> +	struct completion *flogi_reg_done;
> +	char str_wwpn[20];
> +	char str_wwnn[20];
> +	};
> +	struct rport_dd_data_s {
> +	struct fnic_tport_s *tport;
> +	struct fnic_iport_s *iport;
> +};
> +
> +enum fnic_recv_frame_type_e {
> +	FNIC_FABRIC_FLOGI_RSP = 0,
> +	FNIC_FABRIC_PLOGI_RSP,
> +	FNIC_FDMI_PLOGI_RSP,
> +	FNIC_FABRIC_RPN_RSP,
> +	FNIC_FABRIC_RFT_RSP,
> +	FNIC_FABRIC_RFF_RSP,
> +	FNIC_FABRIC_SCR_RSP,
> +	FNIC_FABRIC_GPN_FT_RSP,
> +	FNIC_TPORT_PLOGI_RSP,
> +	FNIC_TPORT_PRLI_RSP,
> +	FNIC_TPORT_ADISC_RSP,
> +	FNIC_TPORT_LOGO_RSP,
> +	FNIC_BLS_ABTS_REQ,
> +	FNIC_FABRIC_LOGO_RSP,
> +	FNIC_BLS_ABTS_RSP,
> +	FNIC_ELS_PLOGI_REQ,
> +	FNIC_ELS_RSCN_REQ,
> +	FNIC_ELS_LOGO_REQ,
> +	FNIC_ELS_ECHO_REQ,
> +	FNIC_ELS_ADISC,
> +	FNIC_ELS_RLS,
> +	FNIC_ELS_UNSUPPORTED_REQ,
> +	FNIC_ELS_RRQ,
> +	FNIC_FDMI_RSP,
> +};
> +
> +enum fnic_port_speeds {
> +	DCEM_PORTSPEED_NONE = 0,
> +	DCEM_PORTSPEED_1G = 1000,
> +	DCEM_PORTSPEED_2G = 2000,
> +	DCEM_PORTSPEED_4G = 4000,
> +	DCEM_PORTSPEED_8G = 8000,
> +	DCEM_PORTSPEED_10G = 10000,
> +	DCEM_PORTSPEED_16G = 16000,
> +	DCEM_PORTSPEED_20G = 20000,
> +	DCEM_PORTSPEED_25G = 25000,
> +	DCEM_PORTSPEED_32G = 32000,
> +	DCEM_PORTSPEED_40G = 40000,
> +	DCEM_PORTSPEED_4x10G = 41000,
> +	DCEM_PORTSPEED_50G = 50000,
> +	DCEM_PORTSPEED_64G = 64000,
> +	DCEM_PORTSPEED_100G = 100000,
> +	DCEM_PORTSPEED_128G = 128000,
> +};
> +
> +/* Function Declarations */
> +/* fdls_disc.c */
> +void fnic_fdls_disc_init(struct fnic_iport_s *iport);
> +void fnic_fdls_disc_start(struct fnic_iport_s *iport);
> +void fnic_fdls_recv_frame(struct fnic_iport_s *iport, void *rx_frame, int len,
> +	int fchdr_offset);
> +void fnic_fdls_link_down(struct fnic_iport_s *iport);
> +void fdls_init_tgt_oxid_pool(struct fnic_iport_s *iport);
> +void fdls_tgt_logout(struct fnic_iport_s *iport, struct fnic_tport_s *tport);
> +
> +/* fnic_fcs.c */
> +void fnic_fdls_init(struct fnic *fnic, int usefip);
> +int fnic_send_fcoe_frame(struct fnic_iport_s *iport, void *payload,
> +	int payload_sz);
> +
> +int fnic_send_fip_frame(struct fnic_iport_s *iport,
> +	void *payload, int payload_sz);
> +void fnic_fdls_learn_fcoe_macs(struct fnic_iport_s *iport, void *rx_frame,
> +	uint8_t *fcid);
> +
> +void fnic_fdls_add_tport(struct fnic_iport_s *iport,
> +		struct fnic_tport_s *tport, unsigned long flags);
> +void fnic_fdls_remove_tport(struct fnic_iport_s *iport,
> +		struct fnic_tport_s *tport, unsigned long flags);
> +
> +/* fip.c */
> +void fnic_fcoe_send_vlan_req(struct fnic *fnic);
> +void fnic_common_fip_cleanup(struct fnic *fnic);
> +int fdls_fip_recv_frame(struct fnic *fnic, void *frame);
> +void fnic_handle_fcs_ka_timer(struct timer_list *t);
> +void fnic_handle_enode_ka_timer(struct timer_list *t);
> +void fnic_handle_vn_ka_timer(struct timer_list *t);
> +void fnic_handle_fip_timer(struct timer_list *t);
> +extern void fdls_fabric_timer_callback(struct timer_list *t);
> +
> +/* fnic_scsi.c */
> +void fnic_scsi_fcpio_reset(struct fnic *fnic);
> +extern void fdls_fabric_timer_callback(struct timer_list *t);
> +void fnic_rport_exch_reset(struct fnic *fnic, u32 fcid);
> +int fnic_fdls_register_portid(struct fnic_iport_s *iport, u32 port_id,
> +		void *fp);
> +struct fnic_tport_s *fnic_find_tport_by_fcid(struct fnic_iport_s *iport,
> +		uint32_t fcid);
> +struct fnic_tport_s *fnic_find_tport_by_wwpn(struct fnic_iport_s *iport,
> +		uint64_t  wwpn);
> +
> +#endif /* _FNIC_FDLS_H_ */
> +

Cheers,

Hannes

On Monday, June 10, 2024 11:54 PM, Hannes Reinecke <hare@suse.de> wrote:
>
> On 6/10/24 23:50, Karan Tilak Kumar wrote:
> > Fabric Discovery and Login Services (FDLS) provide functionality for
> > fnic to discover the fabric and target ports. It logs in to the target
> > ports from the initiator and creates target ports (tports).
> > This functionality is essential for port channel register state change
> > notification (PC-RSCN) handling and FC-NVME initiator role.
> >
> > This functionality is essential for eCPU hang or panic handling. In
> > cases where the eCPU in the UCS VIC (Unified Computing Services
> > Virtual Interface Card) hangs, a fabric log out is sent to the fabric.
> > Upon successful log out from the fabric, the IO path is failed over to
> > a new path.
> >
> Didn't you have the paragraph in the patch series description?
> Please don't duplicate it.

Yes, that's correct. Sure. I'll modify the patch description in the 
next version.

> Please tell me that 'OXID' doesn't mean what I think it does ...
> Hardcoded OXIDs for individual command types? Really?

The thinking during the design process was that each fabric related 
command needs only one OXID at a time. Since the calls to the fabric
are serial in nature, there may not be a need for a pool. In addition, 
debugging would be a little bit easier since we could just look at the 
OXID and immediately know what the command was, without the 
need for an FC trace to debug the issue.

However, based on your review feedback, we are evaluating a 
design for a pool-based approach.

> > +#define FC_CT_RFF_CMD          (0x1F02)
> > +
> > +#endif
>
> Duplicate macros for endian differences is quite error-prone, as you have to remember to always change both sides.
> I would prefer to have just one copy and use macros to access the values (eg always use get_unaligned_le16).

Sure. I'll fix this in the next version of changes.

> > +#define FC_ABTS_RCTL            0x81
> > +#define FNIC_ELS_ADISC_REQ      0x52
> > +#define FC_ELS_RJT_LOGICAL_BUSY 0x05
> > +#define FC_ELS_RJT_BUSY         0x09
> > +#define FC_ELS_RTV_REQ          0x0E
> > +
>
> There is already a copy of FC-LS definitions in include/uapi/scsi/fc/fc_els.h.
>
> Please ensure that you don't introduce duplicates.

Sure, makes sense. Thanks for pointing it out. I'll fix this in the next version of changes.

> htonll() macros are typically used in the network stack; please use get_unaligned_be64().

Sure. I'll fix this in the next version of changes.

> > +#define FNIC_FC_FRAME_UNSOLICITED(_fchdr)  (_fchdr->r_ctl == 0x22)
> > +#define FNIC_FC_FRAME_SOLICITED_DATA(_fchdr)    (_fchdr->r_ctl == 0x21)
> > +#define FNIC_FC_FRAME_SOLICITED_CTRL_REPLY(_fchdr)    (_fchdr->r_ctl == 0x23)
> > +#define FNIC_FC_FRAME_FCTL_LAST_END_SEQ(_fchdr)    (_fchdr->f_ctl == 0x98)
> > +#define FNIC_FC_FRAME_FCTL_LAST_END_SEQ_INT(_fchdr)    (_fchdr->f_ctl == 0x99)
> > +#define FNIC_FC_FRAME_FCTL_FIRST_LAST_SEQINIT(_fchdr)  (_fchdr->f_ctl == 0x29)
> > +#define FNIC_FC_FRAME_FC4_SCTL(_fchdr)    (_fchdr->r_ctl == 0x03)
> > +#define FNIC_FC_FRAME_TYPE_BLS(_fchdr) (_fchdr->type == 0x00) #define
> > +FNIC_FC_FRAME_TYPE_ELS(_fchdr) (_fchdr->type == 0x01) #define
> > +FNIC_FC_FRAME_TYPE_FC_GS(_fchdr) (_fchdr->type == 0x20) #define
> > +FNIC_FC_FRAME_CS_CTL(_fchdr) (_fchdr->cs_ctl == 0x00)
>
> Please use macro names instead of raw values here.

Sure. I'll fix this in the next version of changes.

> These seem to be standard FCoE frame definitions, for which we already have a copy in include/uapi/scsi/fc.
> Please use the definitions from there instead of introducing your own.

Sure. I'll use the standard definitions from the header files.

> > +#define fnic_del_fabric_timer_sync() {                                                     \
> > +   iport->fabric.del_timer_inprogress = 1;                                         \
> > +   spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);     \
> > +   del_timer_sync(&iport->fabric.retry_timer);                                     \
> > +   spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);          \
> > +   iport->fabric.del_timer_inprogress = 0;                                         \
> > +}
> > +
> Please, make this an inline function.
> > +#define fnic_del_tport_timer_sync() {                                                      \
> > +   tport->del_timer_inprogress = 1;                                                        \
> > +   spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);     \
> > +   del_timer_sync(&tport->retry_timer);                                            \
> > +   spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);          \
> > +   tport->del_timer_inprogress = 0;                                                        \
> > +}
> > +
> Same here.

Makes sense. I'll make these inline functions.

Regards,
Karan