[PATCH v2 00/12] libfc, libfcoe and fcoe updates for scsi-misc

[PATCH v2 00/12] libfc, libfcoe and fcoe updates for scsi-misc

[Robert Love]

The following series contains target offload patches and
a few bug fixes. The main change that this series presents
is target mode offload support. These are the same offloads
that the initiator uses, so there is no new hardware support.
There is a new hook in the netdev_ops structure to setup
a DDP context for an offloadable exchange. If ndo_fcoe_ddp_target
is present on the netdevice the fcoe stack will make use of
it. This interfce was merged at the v3.0 merge window, so
all depedencies should already be complete and in-kernel.

Aside from this feature there are a few fixes to tcm_fc as
well as libfc and fcoe.

All patches were tested against scsi-misc on the day of
this posting.

---

Kiran Patil (8):
      tcm_fc: Fix warning in file tfc_io
      libfc: Enhancement to RPORT state machine applicable only for VN2VN mode
      fcoe: Unable to select the exchangeID from offload pool for storage targets
      fcoe: Round-robin based selection of CPU for post-processing of incoming commands
      fcoe: Amends previous patch, Round-robin based selection of CPU for post processing of incoming request for FCoE target
      libfc:Fix for exchange/seq loopup failure when FCoE stack is used as target and connected to windows initaitor
      tcm_fc: Fix ft_send_tm-bug and drop ft_get_lun_for_cmd usage
      tcm_fc: Fixing reference counting problem which was causing ft_sess to be deleted.

Neerav Parikh (1):
      fcoe: Rearrange fcoe port and NPIV port cleanup

Vasu Dev (1):
      libfc: post reset event on lport reset

Yi Zou (2):
      libfc, tcm_fc: add ddp_targ() to libfc function template to supprot FCoE DDP in target mode
      fcoe: support ndo_fcoe_ddp_target() for DDP in FCoE targe


 drivers/scsi/fcoe/fcoe.c        |  174 ++++++++++++++++++++++++++++++---------
 drivers/scsi/libfc/fc_exch.c    |   26 +++++-
 drivers/scsi/libfc/fc_lport.c   |    2 
 drivers/scsi/libfc/fc_rport.c   |   14 +++
 drivers/target/tcm_fc/tcm_fc.h  |    2 
 drivers/target/tcm_fc/tfc_cmd.c |   88 +++++++++++---------
 drivers/target/tcm_fc/tfc_io.c  |    2 
 include/scsi/libfc.h            |    8 ++
 8 files changed, 233 insertions(+), 83 deletions(-)

-- 
Thanks, //Rob

[PATCH v2 01/12] tcm_fc: Fix warning in file tfc_io

[Robert Love]

From: Kiran Patil <kiran.patil@intel.com>

Printk causing warning, trying to print size_t as %x.

Fix the printk statement, to print it as %zd instead of explictly casting to (unsigned int)

Signed-off-by: Kiran Patil <kiran.patil@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/target/tcm_fc/tfc_io.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/target/tcm_fc/tfc_io.c b/drivers/target/tcm_fc/tfc_io.c
index 4c3c0ef..702d4b8 100644
--- a/drivers/target/tcm_fc/tfc_io.c
+++ b/drivers/target/tcm_fc/tfc_io.c
@@ -203,7 +203,7 @@ int ft_queue_data_in(struct se_cmd *se_cmd)
 			/* XXX For now, initiator will retry */
 			if (printk_ratelimit())
 				printk(KERN_ERR "%s: Failed to send frame %p, "
-						"xid <0x%x>, remaining <0x%x>, "
+						"xid <0x%x>, remaining <%zd>, "
 						"lso_max <0x%x>\n",
 						__func__, fp, ep->xid,
 						remaining, lport->lso_max);

[PATCH v2 02/12] libfc: Enhancement to RPORT state machine applicable only for VN2VN mode

[Robert Love]

From: Kiran Patil <kiran.patil@intel.com>

Problem: Existing RPORT state machine continues witg FLOGI/PLOGI process
only after it receices beacon from other end. Once claiming stage is over
(either clain notify or clain repose), beacon is sent and state machine
enters into operational mode where it initiates the rlogin process (FLOGI/PLOGI)
to the peer but before this rlogin is initiated, exitsing implementation
checks if it received beacon from other end, it beacon is not received yet,
rlogin process is not initiated. Other end initiates FLOGI but peer end keeps
on rejecting FLOGI, hence after 3 retries other end deletes associated rport,
then sends a beacon. Once the beacon is received, peer end now initiates rlogin
to the peer end but since associated rport is deleted FLOGI is neither accepted
nor the reject response send out because rport is deleted. Hence unable to proceed
withg FLOGI/PLOGI process and fails to establish VN2VN connection.

Fix: VN2VN spec is not standard yet but based on exitsing collateral on T11, it appears
that, both end shall send beacon and enter into 'operational mode' without
explictly waiting for beacon from other end. Fix is to allow the RPORT login
process as long as respective RPORT is created (as part of claim notification /
claim response) even though state of RPORT is INIT. Means don't wait for beacon
from peer end, if peer end initiates FLOGI (means peer end exist and responding).

Notes: This patch is preparing the FCoE stack for target wrt offload. This is generic
patch and harmless even if applied on storage initiator because 'else if' condition
of function 'fcoe_oem_found' shall evaluate to TRUE only for targets.

Dependencies: None

Signed-off-by: Kiran Patil <kiran.patil@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/libfc/fc_rport.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
index 49e1ccc..3b66937 100644
--- a/drivers/scsi/libfc/fc_rport.c
+++ b/drivers/scsi/libfc/fc_rport.c
@@ -801,6 +801,20 @@ static void fc_rport_recv_flogi_req(struct fc_lport *lport,
 
 	switch (rdata->rp_state) {
 	case RPORT_ST_INIT:
+		/*
+		 * If received the FLOGI request on RPORT which is INIT state
+		 * (means not transition to FLOGI either fc_rport timeout
+		 * function didn;t trigger or this end hasn;t received
+		 * beacon yet from other end. In that case only, allow RPORT
+		 * state machine to continue, otherwise fall through which
+		 * causes the code to send reject response.
+		 * NOTE; Not checking for FIP->state such as VNMP_UP or
+		 * VNMP_CLAIM because if FIP state is not one of those,
+		 * RPORT wouldn;t have created and 'rport_lookup' would have
+		 * failed anyway in that case.
+		 */
+		if (lport->point_to_multipoint)
+			break;
 	case RPORT_ST_DELETE:
 		mutex_unlock(&rdata->rp_mutex);
 		rjt_data.reason = ELS_RJT_FIP;

[PATCH v2 03/12] libfc, tcm_fc: add ddp_targ() to libfc function template to supprot FCoE DDP in target mode

[Robert Love]

From: Yi Zou <yi.zou@intel.com>

The fcoe driver can implement ddp_targ() similarly to ddp_setup() when fcoe
stack works with existing target frame, e.g., tcm, where the ddp_targ() would
eventually point to the underlying hardware driver's implementation of
ndo_fcoe_ddp_targ() through net_device_ops. This new API sets up DDP context
for target appropriately by setting required bits for DDP context.

Signed-off-by: Yi Zou <yi.zou@intel.com>
Signed-off-by: Kiran Patil <kiran.patil@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/target/tcm_fc/tfc_cmd.c |    5 +++--
 include/scsi/libfc.h            |    8 ++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
index c056a11..1625b48 100644
--- a/drivers/target/tcm_fc/tfc_cmd.c
+++ b/drivers/target/tcm_fc/tfc_cmd.c
@@ -289,8 +289,9 @@ int ft_write_pending(struct se_cmd *se_cmd)
 				cmd->sg_cnt =
 					T_TASK(se_cmd)->t_tasks_sg_chained_no;
 			}
-			if (cmd->sg && lport->tt.ddp_setup(lport, ep->xid,
-						    cmd->sg, cmd->sg_cnt))
+			if (cmd->sg && lport->tt.ddp_target(lport, ep->xid,
+							    cmd->sg,
+							    cmd->sg_cnt))
 				cmd->was_ddp_setup = 1;
 		}
 	}
diff --git a/include/scsi/libfc.h b/include/scsi/libfc.h
index a3cbda4..7d96829 100644
--- a/include/scsi/libfc.h
+++ b/include/scsi/libfc.h
@@ -511,6 +511,14 @@ struct libfc_function_template {
 	 */
 	int (*ddp_done)(struct fc_lport *, u16);
 	/*
+	 * Sets up the DDP context for a given exchange id on the given
+	 * scatterlist if LLD supports DDP for FCoE target.
+	 *
+	 * STATUS: OPTIONAL
+	 */
+	int (*ddp_target)(struct fc_lport *, u16, struct scatterlist *,
+			  unsigned int);
+	/*
 	 * Allow LLD to fill its own Link Error Status Block
 	 *
 	 * STATUS: OPTIONAL

[PATCH v2 04/12] fcoe: support ndo_fcoe_ddp_target() for DDP in FCoE targe

[Robert Love]

From: Yi Zou <yi.zou@intel.com>

Add ddp_target() support to the Open-FCoE sw fcoe hba driver (fcoe.ko).

Signed-off-by: Yi Zou <yi.zou@intel.com>
Signed-off-by: Kiran Patil <kiran.patil@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/fcoe/fcoe.c |   26 +++++++++++++++++++++++++-
 1 files changed, 25 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 155d7b9..f76a321 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -99,7 +99,8 @@ static void fcoe_destroy_work(struct work_struct *);
 static int fcoe_ddp_setup(struct fc_lport *, u16, struct scatterlist *,
 			  unsigned int);
 static int fcoe_ddp_done(struct fc_lport *, u16);
-
+static int fcoe_ddp_target(struct fc_lport *, u16, struct scatterlist *,
+			   unsigned int);
 static int fcoe_cpu_callback(struct notifier_block *, unsigned long, void *);
 
 static bool fcoe_match(struct net_device *netdev);
@@ -143,6 +144,7 @@ static struct libfc_function_template fcoe_libfc_fcn_templ = {
 	.frame_send = fcoe_xmit,
 	.ddp_setup = fcoe_ddp_setup,
 	.ddp_done = fcoe_ddp_done,
+	.ddp_target = fcoe_ddp_target,
 	.elsct_send = fcoe_elsct_send,
 	.get_lesb = fcoe_get_lesb,
 	.lport_set_port_id = fcoe_set_port_id,
@@ -887,6 +889,28 @@ static int fcoe_ddp_setup(struct fc_lport *lport, u16 xid,
 }
 
 /**
+ * fcoe_ddp_target() - Call a LLD's ddp_target through the net device
+ * @lport: The local port to setup DDP for
+ * @xid:   The exchange ID for this DDP transfer
+ * @sgl:   The scatterlist describing this transfer
+ * @sgc:   The number of sg items
+ *
+ * Returns: 0 if the DDP context was not configured
+ */
+static int fcoe_ddp_target(struct fc_lport *lport, u16 xid,
+			   struct scatterlist *sgl, unsigned int sgc)
+{
+	struct net_device *netdev = fcoe_netdev(lport);
+
+	if (netdev->netdev_ops->ndo_fcoe_ddp_target)
+		return netdev->netdev_ops->ndo_fcoe_ddp_target(netdev, xid,
+							       sgl, sgc);
+
+	return 0;
+}
+
+
+/**
  * fcoe_ddp_done() - Call a LLD's ddp_done through the net device
  * @lport: The local port to complete DDP on
  * @xid:   The exchange ID for this DDP transfer

[PATCH v2 05/12] fcoe: Unable to select the exchangeID from offload pool for storage targets

[Robert Love]

From: Kiran Patil <kiran.patil@intel.com>

Problem: When initiator sends write command to target, target tries to assign new sequence. It
         allocates new exchangeID (RX_ID) always from non-offloaded pool (Non-offload EMA)

Fix:     Enhanced fcoe_oem_match routine to look at F_CTL flags and if it is exchange responder and
         command type is WRITEDATA, then function returns TRUE instead of FALSE. This function
         is used to determine which pool to use (offload pool of exchange is used only if this
         function returns TRUE).

Technical Notes: N/A

Signed-off-by: Kiran Patil <kiran.patil@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/fcoe/fcoe.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index f76a321..6378c58 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -751,12 +751,27 @@ static int fcoe_shost_config(struct fc_lport *lport, struct device *dev)
  * The offload EM that this routine is associated with will handle any
  * packets that are for SCSI read requests.
  *
+ * This has been enhanced to work when FCoE stack is operating in target
+ * mode.
+ *
  * Returns: True for read types I/O, otherwise returns false.
  */
 bool fcoe_oem_match(struct fc_frame *fp)
 {
-	return fc_fcp_is_read(fr_fsp(fp)) &&
-		(fr_fsp(fp)->data_len > fcoe_ddp_min);
+	struct fc_frame_header *fh = fc_frame_header_get(fp);
+	struct fcp_cmnd *fcp;
+
+	if (fc_fcp_is_read(fr_fsp(fp)) &&
+	    (fr_fsp(fp)->data_len > fcoe_ddp_min))
+		return true;
+	else if (!(ntoh24(fh->fh_f_ctl) & FC_FC_EX_CTX)) {
+		fcp = fc_frame_payload_get(fp, sizeof(*fcp));
+		if (ntohs(fh->fh_rx_id) == FC_XID_UNKNOWN &&
+		    fcp && (ntohl(fcp->fc_dl) > fcoe_ddp_min) &&
+		    (fcp->fc_flags & FCP_CFL_WRDATA))
+			return true;
+	}
+	return false;
 }
 
 /**

[PATCH v2 06/12] fcoe: Round-robin based selection of CPU for post-processing of incoming commands

[Robert Love]

From: Kiran Patil <kiran.patil@intel.com>

Problem: Earlier mechanism of selection of CPU was, to select the same CPU
which has received incoming request. Hence in case of rx_id = 0xFFFF,
request was always posted to same NetRx queue, hence only 1 CPU is utilized
for handling the command. It was also causing problem of "running out of
exchanges from per CPU pool of exchanges (in case of DDP offload)

Fix: Implemented new algo. to select CPU for post-processing of incoming commands
when rx_id is unknown. This is simple Round robin algo. for CPU selection.

Notes/Dependencies: N/A

Signed-off-by: Kiran Patil <kiran.patil@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/fcoe/fcoe.c |   39 ++++++++++++++++++++++++++++++++++++++-
 1 files changed, 38 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 6378c58..da73115 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1245,6 +1245,36 @@ static int fcoe_cpu_callback(struct notifier_block *nfb,
 }
 
 /**
+ * fcoe_select_cpu() - Selects CPU to handle post-processing of incoming
+ *			command.
+ * @curr_cpu:   CPU which received request
+ *
+ * This routine selects next CPU based on cpumask.
+ *
+ * Returns: int (CPU number). Caller to verify if returned CPU is online or not.
+ */
+static unsigned int fcoe_select_cpu(unsigned int curr_cpu)
+{
+	static unsigned int selected_cpu;
+
+	if (num_online_cpus() == 1)
+		return curr_cpu;
+	/*
+	 * Doing following check, to skip "curr_cpu (smp_processor_id)"
+	 * from selection of CPU is intentional. This is to avoid same CPU
+	 * doing post-processing of command. "curr_cpu" to just receive
+	 * incoming request in case where rx_id is UNKNOWN and all other
+	 * CPU to actually process the command(s)
+	 */
+	do {
+		selected_cpu = cpumask_next(selected_cpu, cpu_online_mask);
+		if (selected_cpu >= nr_cpu_ids)
+			selected_cpu = cpumask_first(cpu_online_mask);
+	} while (selected_cpu == curr_cpu);
+	return selected_cpu;
+}
+
+/**
  * fcoe_rcv() - Receive packets from a net device
  * @skb:    The received packet
  * @netdev: The net device that the packet was received on
@@ -1320,9 +1350,16 @@ int fcoe_rcv(struct sk_buff *skb, struct net_device *netdev,
 	 */
 	if (ntoh24(fh->fh_f_ctl) & FC_FC_EX_CTX)
 		cpu = ntohs(fh->fh_ox_id) & fc_cpu_mask;
-	else
+	else {
 		cpu = smp_processor_id();
 
+		if ((fh->fh_type == FC_TYPE_FCP) &&
+		    (ntohs(fh->fh_rx_id) == FC_XID_UNKNOWN)) {
+			do {
+				cpu = fcoe_select_cpu(cpu);
+			} while (!cpu_online(cpu));
+		}
+	}
 	fps = &per_cpu(fcoe_percpu, cpu);
 	spin_lock_bh(&fps->fcoe_rx_list.lock);
 	if (unlikely(!fps->thread)) {

[PATCH v2 07/12] fcoe: Amends previous patch, Round-robin based selection of CPU for post processing of incoming request for FCoE target

[Robert Love]

From: Kiran Patil <kiran.patil@intel.com>

Problem: Selection of RX queue on target is based on RX-ID. FCoE used 8 Net Rx queues.
HW post the packets based on rx_id % num_rx_queue. Due to this has based filtering,
only one CPU is busy servicing incoming request including post-processing of incoming
request. This is gating factor because
1. Only one CPU is utilized 100% while others CPUs are not used at all.
2. CPU which received request assign "sequence' by selecting exchange from
   per CPU pool (num_ddp_context / num_online_cpus, approxi.). Due to which if
   if rate of incoming request is higher than rate of servicing request, existing
   code path end of sending "BUSY" response (SAM_STAT_BUSY because unable to
   allocate exchange).

Fix: Fan-out incoming request to all other CPUs excluding the CPU which is receiving
all incoiming request. This path also addresses, selecting same CPU based on rx_id
from received frame for completion of the request such as "releasing exchange
to the per CPU Pool". This fix is applicable for FCoE target since initiator
code path already takes care of selecting CPU to complete post-processing of request
once OX_ID is assigned.

Notes: N/A

Dependencines: N/A

Signed-off-by: Kiran Patil <kiran.patil@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/fcoe/fcoe.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index da73115..522fbaa 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1358,7 +1358,11 @@ int fcoe_rcv(struct sk_buff *skb, struct net_device *netdev,
 			do {
 				cpu = fcoe_select_cpu(cpu);
 			} while (!cpu_online(cpu));
-		}
+		} else  if ((fh->fh_type == FC_TYPE_FCP) &&
+			    (ntohs(fh->fh_rx_id) != FC_XID_UNKNOWN)) {
+			cpu = ntohs(fh->fh_rx_id) & fc_cpu_mask;
+		} else
+			cpu = smp_processor_id();
 	}
 	fps = &per_cpu(fcoe_percpu, cpu);
 	spin_lock_bh(&fps->fcoe_rx_list.lock);

[PATCH v2 08/12] libfc:Fix for exchange/seq loopup failure when FCoE stack is used as target and connected to windows initaitor

[Robert Love]

From: Kiran Patil <kiran.patil@intel.com>

Problem: Linux based SW target (TCM) connected to windows initiator was unable to satisfy
write request of size > 2K.

Fix: Existing linux implememtation of FCoE stack is expecting sequence number to match w.r.t
incoming framme. When DDP is used on target in response to write request from initiator, SW
stack is notified only when last data frame arrives and only the pakcket header of last data
frame is posted to NetRx queue of storage. When that last packet was processed in libfc:Exchange
layer, implementation was expecting sequence number to match, but in this case sequence number
which is embedded in FC Header is assigned by windows initaitor, hence due to sequence number mismatch
post-processing which shall result into sending RSP is not done. Enhanced the code to
utilize the sequence number of incoming last frame and process the packet so that, it
will eventually complete the write request by sending write response (RSP) GOOD.

Notes/Dependencies: This patch is validated using windows and linux initiator to make
sure, it doesn't break anything.

Signed-off-by: Kiran Patil <kiran.patil@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/libfc/fc_exch.c |   26 ++++++++++++++++++++++++--
 1 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
index 3b8a645..f5a0665 100644
--- a/drivers/scsi/libfc/fc_exch.c
+++ b/drivers/scsi/libfc/fc_exch.c
@@ -965,8 +965,30 @@ static enum fc_pf_rjt_reason fc_seq_lookup_recip(struct fc_lport *lport,
 		sp = &ep->seq;
 		if (sp->id != fh->fh_seq_id) {
 			atomic_inc(&mp->stats.seq_not_found);
-			reject = FC_RJT_SEQ_ID;	/* sequence/exch should exist */
-			goto rel;
+			if (f_ctl & FC_FC_END_SEQ) {
+				/*
+				 * Update sequence_id based on incoming last
+				 * frame of sequence exchange. This is needed
+				 * for FCoE target where DDP has been used
+				 * on target where, stack is indicated only
+				 * about last frame's (payload _header) header.
+				 * Whereas "seq_id" which is part of
+				 * frame_header is allocated by initiator
+				 * which is totally different from "seq_id"
+				 * allocated when XFER_RDY was sent by target.
+				 * To avoid false -ve which results into not
+				 * sending RSP, hence write request on other
+				 * end never finishes.
+				 */
+				spin_lock_bh(&ep->ex_lock);
+				sp->ssb_stat |= SSB_ST_RESP;
+				sp->id = fh->fh_seq_id;
+				spin_unlock_bh(&ep->ex_lock);
+			} else {
+				/* sequence/exch should exist */
+				reject = FC_RJT_SEQ_ID;
+				goto rel;
+			}
 		}
 	}
 	WARN_ON(ep != fc_seq_exch(sp));

[PATCH v2 09/12] tcm_fc: Fix ft_send_tm-bug and drop ft_get_lun_for_cmd usage

[Robert Love]

From: Kiran Patil <kiran.patil@intel.com>

This patch fixes a bug in ft_send_tm() that was incorrectly calling
ft_get_lun_for_cmd() -> transport_get_lun_for_cmd(), instead of using
transport_get_lun_for_tmr() for the proper struct se_lun lookup.

It also drops the now unnecessary ft_get_lun_for_cmd() code, and uses
scsilun_to_int() directly ahead of direct transport_get_lun_for_cmd()
and transport_get_lun_for_tmr usage().

Patch was reworked due to NULL pointer access in function transport_get_lun_for_tmr(),
(NOTE: transport_get_lun_for_tmr access tmr_req pointer which caused
panic due to NULL pointer access) This patch fixes the issue by re-arranging
the codepath where function "transport_get_lun_for_tmr" is called
after tmr request is allocated and made it available as part of se_cmd.

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
Signed-off-by: Kiran Patil <kiran.patil@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/target/tcm_fc/tcm_fc.h  |    2 +
 drivers/target/tcm_fc/tfc_cmd.c |   62 ++++++++++++++++++++-------------------
 2 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/drivers/target/tcm_fc/tcm_fc.h b/drivers/target/tcm_fc/tcm_fc.h
index defff32..7b82f1b 100644
--- a/drivers/target/tcm_fc/tcm_fc.h
+++ b/drivers/target/tcm_fc/tcm_fc.h
@@ -144,7 +144,7 @@ enum ft_cmd_state {
  */
 struct ft_cmd {
 	enum ft_cmd_state state;
-	u16 lun;			/* LUN from request */
+	u32 lun;                        /* LUN from request */
 	struct ft_sess *sess;		/* session held for cmd */
 	struct fc_seq *seq;		/* sequence in exchange mgr */
 	struct se_cmd se_cmd;		/* Local TCM I/O descriptor */
diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
index 1625b48..cdbcb6c 100644
--- a/drivers/target/tcm_fc/tfc_cmd.c
+++ b/drivers/target/tcm_fc/tfc_cmd.c
@@ -94,29 +94,6 @@ void ft_dump_cmd(struct ft_cmd *cmd, const char *caller)
 		16, 4, cmd->cdb, MAX_COMMAND_SIZE, 0);
 }
 
-/*
- * Get LUN from CDB.
- */
-static int ft_get_lun_for_cmd(struct ft_cmd *cmd, u8 *lunp)
-{
-	u64 lun;
-
-	lun = lunp[1];
-	switch (lunp[0] >> 6) {
-	case 0:
-		break;
-	case 1:
-		lun |= (lunp[0] & 0x3f) << 8;
-		break;
-	default:
-		return -1;
-	}
-	if (lun >= TRANSPORT_MAX_LUNS_PER_TPG)
-		return -1;
-	cmd->lun = lun;
-	return transport_get_lun_for_cmd(&cmd->se_cmd, NULL, lun);
-}
-
 static void ft_queue_cmd(struct ft_sess *sess, struct ft_cmd *cmd)
 {
 	struct se_queue_obj *qobj;
@@ -426,13 +403,6 @@ static void ft_send_tm(struct ft_cmd *cmd)
 	switch (fcp->fc_tm_flags) {
 	case FCP_TMF_LUN_RESET:
 		tm_func = TMR_LUN_RESET;
-		if (ft_get_lun_for_cmd(cmd, fcp->fc_lun) < 0) {
-			ft_dump_cmd(cmd, __func__);
-			transport_send_check_condition_and_sense(&cmd->se_cmd,
-				cmd->se_cmd.scsi_sense_reason, 0);
-			ft_sess_put(cmd->sess);
-			return;
-		}
 		break;
 	case FCP_TMF_TGT_RESET:
 		tm_func = TMR_TARGET_WARM_RESET;
@@ -464,6 +434,35 @@ static void ft_send_tm(struct ft_cmd *cmd)
 		return;
 	}
 	cmd->se_cmd.se_tmr_req = tmr;
+
+	switch (fcp->fc_tm_flags) {
+	case FCP_TMF_LUN_RESET:
+		cmd->lun = scsilun_to_int((struct scsi_lun *)fcp->fc_lun);
+		if (transport_get_lun_for_tmr(&cmd->se_cmd, cmd->lun) < 0) {
+			/*
+			 * Make sure to clean up newly allocated TMR request
+			 * since "unable to  handle TMR request because failed
+			 * to get to LUN"
+			 */
+			FT_TM_DBG("Failed to get LUN for TMR func %d, "
+				  "se_cmd %p, unpacked_lun %d\n",
+				  tm_func, &cmd->se_cmd, cmd->lun);
+			transport_generic_free_cmd(&cmd->se_cmd, 0, 1, 0);
+			ft_dump_cmd(cmd, __func__);
+			transport_send_check_condition_and_sense(&cmd->se_cmd,
+				cmd->se_cmd.scsi_sense_reason, 0);
+			ft_sess_put(cmd->sess);
+			return;
+		}
+		break;
+	case FCP_TMF_TGT_RESET:
+	case FCP_TMF_CLR_TASK_SET:
+	case FCP_TMF_ABT_TASK_SET:
+	case FCP_TMF_CLR_ACA:
+		break;
+	default:
+		return;
+	}
 	transport_generic_handle_tmr(&cmd->se_cmd);
 }
 
@@ -636,7 +635,8 @@ static void ft_send_cmd(struct ft_cmd *cmd)
 
 	fc_seq_exch(cmd->seq)->lp->tt.seq_set_resp(cmd->seq, ft_recv_seq, cmd);
 
-	ret = ft_get_lun_for_cmd(cmd, fcp->fc_lun);
+	cmd->lun = scsilun_to_int((struct scsi_lun *)fcp->fc_lun);
+	ret = transport_get_lun_for_cmd(&cmd->se_cmd, NULL, cmd->lun);
 	if (ret < 0) {
 		ft_dump_cmd(cmd, __func__);
 		transport_send_check_condition_and_sense(&cmd->se_cmd,

[PATCH v2 10/12] tcm_fc: Fixing reference counting problem which was causing ft_sess to be deleted.

[Robert Love]

From: Kiran Patil <kiran.patil@intel.com>

Problem: After fixing the issue in TCM core w.r.t LUN Reset (Task Management request)
, ran into issue where during the completing of this LUN Reset command, reference
count of "ft_sess" drops to zero which caused "sess" to be deleted.

Fix: As part of handling task management request (e.g. LUN Reset), TCM core function
"transport_generic_do_tmr" ends up calling ft_free_cmd  which in turn calls "ft_sess_put"
(which drops session's reference count by 1) and then frees ft_cmd. Then function
"transport_generic_do_tmr" calls "transport_cmd_check_stop" which in turn also calls
ft_free_cmd (which calls ft_sess_put - which drops reference count of sess by 1, hence
reference count of sess becomes zero and session gets deleted). Fix is to just send
response in case of tmr from function "ft_queue_resp_code" and not delete "ft_cmd"
(means don't call ft_free_cmd). Earlier code was to send the response code and also
free ft_cmd. ft_free_cmd will be freed later after sending response code as a result of
"transport_cmd_check_stop" (which calls ft_release_cmd -> ft_free_cmd) being called
from "transport_generic_do_tmr" after sening TMR response code.

Notes/Dependencies: This bug was found after fixing NULL pointer access issue in TCM
core (in LUN Reset codepath)

Signed-off-by: Kiran Patil <kiran.patil@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/target/tcm_fc/tfc_cmd.c |   21 ++++++++++++++++-----
 1 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
index cdbcb6c..2b60eee 100644
--- a/drivers/target/tcm_fc/tfc_cmd.c
+++ b/drivers/target/tcm_fc/tfc_cmd.c
@@ -380,12 +380,23 @@ static void ft_send_resp_status(struct fc_lport *lport,
 
 /*
  * Send error or task management response.
- * Always frees the cmd and associated state.
  */
-static void ft_send_resp_code(struct ft_cmd *cmd, enum fcp_resp_rsp_codes code)
+static void ft_send_resp_code(struct ft_cmd *cmd,
+			      enum fcp_resp_rsp_codes code)
 {
 	ft_send_resp_status(cmd->sess->tport->lport,
 			    cmd->req_frame, SAM_STAT_GOOD, code);
+}
+
+
+/*
+ * Send error or task management response.
+ * Always frees the cmd and associated state.
+ */
+static void ft_send_resp_code_and_free(struct ft_cmd *cmd,
+				      enum fcp_resp_rsp_codes code)
+{
+	ft_send_resp_code(cmd, code);
 	ft_free_cmd(cmd);
 }
 
@@ -422,7 +433,7 @@ static void ft_send_tm(struct ft_cmd *cmd)
 		 * tm_flags set is invalid.
 		 */
 		FT_TM_DBG("invalid FCP tm_flags %x\n", fcp->fc_tm_flags);
-		ft_send_resp_code(cmd, FCP_CMND_FIELDS_INVALID);
+		ft_send_resp_code_and_free(cmd, FCP_CMND_FIELDS_INVALID);
 		return;
 	}
 
@@ -430,7 +441,7 @@ static void ft_send_tm(struct ft_cmd *cmd)
 	tmr = core_tmr_alloc_req(&cmd->se_cmd, cmd, tm_func);
 	if (!tmr) {
 		FT_TM_DBG("alloc failed\n");
-		ft_send_resp_code(cmd, FCP_TMF_FAILED);
+		ft_send_resp_code_and_free(cmd, FCP_TMF_FAILED);
 		return;
 	}
 	cmd->se_cmd.se_tmr_req = tmr;
@@ -668,7 +679,7 @@ static void ft_send_cmd(struct ft_cmd *cmd)
 	return;
 
 err:
-	ft_send_resp_code(cmd, FCP_CMND_FIELDS_INVALID);
+	ft_send_resp_code_and_free(cmd, FCP_CMND_FIELDS_INVALID);
 	return;
 }
 

[PATCH v2 11/12] libfc: post reset event on lport reset

[Robert Love]

From: Vasu Dev <vasu.dev@intel.com>

Post an FCH_EVT_LIPRESET event on lport reset as
as lport reset occurs on FIP cleat virtual link,
this could be due to change in fcoe vlan and this
event will allow user app fcoemon to switch to
new fcoe vlan.

Signed-off-by: Vasu Dev <vasu.dev@intel.com>
Tested-by: Ross Brattain <ross.b.brattain@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/libfc/fc_lport.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/libfc/fc_lport.c b/drivers/scsi/libfc/fc_lport.c
index 389ab80..e008b16 100644
--- a/drivers/scsi/libfc/fc_lport.c
+++ b/drivers/scsi/libfc/fc_lport.c
@@ -1025,6 +1025,8 @@ static void fc_lport_enter_reset(struct fc_lport *lport)
 			fc_vport_set_state(lport->vport, FC_VPORT_LINKDOWN);
 	}
 	fc_lport_state_enter(lport, LPORT_ST_RESET);
+	fc_host_post_event(lport->host, fc_get_event_number(),
+			   FCH_EVT_LIPRESET, 0);
 	fc_vports_linkchange(lport);
 	fc_lport_reset_locked(lport);
 	if (lport->link_up)

[PATCH v2 12/12] fcoe: Rearrange fcoe port and NPIV port cleanup

[Robert Love]

From: Neerav Parikh <neerav.parikh@intel.com>

When NPIV port destroy handler is called it does not do all the cleanup
required for the given NPIV port. This was happening as some of the
lport cleanup moved to fcoe_interface_cleanup() routine, which is not
called as part of the vport delete process.

This patch rearranges the sequence in which the fcoe_if_destory() and
fcoe_interface_cleanup() functions are being called from various places
in the code. It now matches the sequence they are constructed during the
create process for both N_Port as well as NPIV port.

Tested-by: Ross Brattain <ross.b.brattain@intel.com>
Signed-off-by: Neerav Parikh <Neerav.Parikh@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/fcoe/fcoe.c |   86 ++++++++++++++++++++++++++--------------------
 1 files changed, 49 insertions(+), 37 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 522fbaa..204fa8d 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -431,21 +431,6 @@ void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
 	struct fcoe_ctlr *fip = &fcoe->ctlr;
 	u8 flogi_maddr[ETH_ALEN];
 	const struct net_device_ops *ops;
-	struct fcoe_port *port = lport_priv(fcoe->ctlr.lp);
-
-	FCOE_NETDEV_DBG(netdev, "Destroying interface\n");
-
-	/* Logout of the fabric */
-	fc_fabric_logoff(fcoe->ctlr.lp);
-
-	/* Cleanup the fc_lport */
-	fc_lport_destroy(fcoe->ctlr.lp);
-
-	/* Stop the transmit retry timer */
-	del_timer_sync(&port->timer);
-
-	/* Free existing transmit skbs */
-	fcoe_clean_pending_queue(fcoe->ctlr.lp);
 
 	/*
 	 * Don't listen for Ethernet packets anymore.
@@ -468,9 +453,6 @@ void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
 	} else
 		dev_mc_del(netdev, FIP_ALL_ENODE_MACS);
 
-	if (!is_zero_ether_addr(port->data_src_addr))
-		dev_uc_del(netdev, port->data_src_addr);
-
 	/* Tell the LLD we are done w/ FCoE */
 	ops = netdev->netdev_ops;
 	if (ops->ndo_fcoe_disable) {
@@ -478,6 +460,8 @@ void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
 			FCOE_NETDEV_DBG(netdev, "Failed to disable FCoE"
 					" specific feature for LLD.\n");
 	}
+
+	/* Release the self-reference taken during fcoe_interface_create() */
 	fcoe_interface_put(fcoe);
 }
 
@@ -861,6 +845,32 @@ skip_oem:
  */
 static void fcoe_if_destroy(struct fc_lport *lport)
 {
+	struct fcoe_port *port = lport_priv(lport);
+	struct fcoe_interface *fcoe = port->priv;
+	struct net_device *netdev = fcoe->netdev;
+
+	FCOE_NETDEV_DBG(netdev, "Destroying interface\n");
+
+	/* Logout of the fabric */
+	fc_fabric_logoff(lport);
+
+	/* Cleanup the fc_lport */
+	fc_lport_destroy(lport);
+
+	/* Stop the transmit retry timer */
+	del_timer_sync(&port->timer);
+
+	/* Free existing transmit skbs */
+	fcoe_clean_pending_queue(lport);
+
+	rtnl_lock();
+	if (!is_zero_ether_addr(port->data_src_addr))
+		dev_uc_del(netdev, port->data_src_addr);
+	rtnl_unlock();
+
+	/* Release reference held in fcoe_if_create() */
+	fcoe_interface_put(fcoe);
+
 	/* Free queued packets for the per-CPU receive threads */
 	fcoe_percpu_clean(lport);
 
@@ -1813,7 +1823,6 @@ static int fcoe_device_notification(struct notifier_block *notifier,
 	case NETDEV_UNREGISTER:
 		list_del(&fcoe->list);
 		port = lport_priv(fcoe->ctlr.lp);
-		fcoe_interface_cleanup(fcoe);
 		queue_work(fcoe_wq, &port->destroy_work);
 		goto out;
 		break;
@@ -1907,22 +1916,22 @@ static int fcoe_destroy(struct net_device *netdev)
 {
 	struct fcoe_interface *fcoe;
 	struct fc_lport *lport;
+	struct fcoe_port *port;
 	int rc = 0;
 
 	mutex_lock(&fcoe_config_mutex);
 	rtnl_lock();
 	fcoe = fcoe_hostlist_lookup_port(netdev);
 	if (!fcoe) {
-		rtnl_unlock();
 		rc = -ENODEV;
 		goto out_nodev;
 	}
 	lport = fcoe->ctlr.lp;
+	port = lport_priv(lport);
 	list_del(&fcoe->list);
-	fcoe_interface_cleanup(fcoe);
-	rtnl_unlock();
-	fcoe_if_destroy(lport);
+	queue_work(fcoe_wq, &port->destroy_work);
 out_nodev:
+	rtnl_unlock();
 	mutex_unlock(&fcoe_config_mutex);
 	return rc;
 }
@@ -1934,10 +1943,25 @@ out_nodev:
 static void fcoe_destroy_work(struct work_struct *work)
 {
 	struct fcoe_port *port;
+	struct fcoe_interface *fcoe;
+	int npiv = 0;
 
 	port = container_of(work, struct fcoe_port, destroy_work);
 	mutex_lock(&fcoe_config_mutex);
+
+	/* set if this is an NPIV port */
+	npiv = port->lport->vport ? 1 : 0;
+
+	fcoe = port->priv;
 	fcoe_if_destroy(port->lport);
+
+	/* Do not tear down the fcoe interface for NPIV port */
+	if (!npiv) {
+		rtnl_lock();
+		fcoe_interface_cleanup(fcoe);
+		rtnl_unlock();
+	}
+
 	mutex_unlock(&fcoe_config_mutex);
 }
 
@@ -1966,7 +1990,7 @@ static bool fcoe_match(struct net_device *netdev)
  */
 static int fcoe_create(struct net_device *netdev, enum fip_state fip_mode)
 {
-	int rc;
+	int rc = 0;
 	struct fcoe_interface *fcoe;
 	struct fc_lport *lport;
 
@@ -1991,7 +2015,7 @@ static int fcoe_create(struct net_device *netdev, enum fip_state fip_mode)
 		       netdev->name);
 		rc = -EIO;
 		fcoe_interface_cleanup(fcoe);
-		goto out_free;
+		goto out_nodev;
 	}
 
 	/* Make this the "master" N_Port */
@@ -2006,17 +2030,6 @@ static int fcoe_create(struct net_device *netdev, enum fip_state fip_mode)
 	if (!fcoe_link_ok(lport))
 		fcoe_ctlr_link_up(&fcoe->ctlr);
 
-	/*
-	 * Release from init in fcoe_interface_create(), on success lport
-	 * should be holding a reference taken in fcoe_if_create().
-	 */
-	fcoe_interface_put(fcoe);
-	rtnl_unlock();
-	mutex_unlock(&fcoe_config_mutex);
-
-	return 0;
-out_free:
-	fcoe_interface_put(fcoe);
 out_nodev:
 	rtnl_unlock();
 	mutex_unlock(&fcoe_config_mutex);
@@ -2298,7 +2311,6 @@ static void __exit fcoe_exit(void)
 	list_for_each_entry_safe(fcoe, tmp, &fcoe_hostlist, list) {
 		list_del(&fcoe->list);
 		port = lport_priv(fcoe->ctlr.lp);
-		fcoe_interface_cleanup(fcoe);
 		queue_work(fcoe_wq, &port->destroy_work);
 	}
 	rtnl_unlock();

Re: [PATCH v2 09/12] tcm_fc: Fix ft_send_tm-bug and drop ft_get_lun_for_cmd usage

[Nicholas A. Bellinger]

On Mon, 2011-06-20 at 16:59 -0700, Robert Love wrote:
> From: Kiran Patil <kiran.patil@intel.com>
> 
> This patch fixes a bug in ft_send_tm() that was incorrectly calling
> ft_get_lun_for_cmd() -> transport_get_lun_for_cmd(), instead of using
> transport_get_lun_for_tmr() for the proper struct se_lun lookup.
> 
> It also drops the now unnecessary ft_get_lun_for_cmd() code, and uses
> scsilun_to_int() directly ahead of direct transport_get_lun_for_cmd()
> and transport_get_lun_for_tmr usage().
> 
> Patch was reworked due to NULL pointer access in function transport_get_lun_for_tmr(),
> (NOTE: transport_get_lun_for_tmr access tmr_req pointer which caused
> panic due to NULL pointer access) This patch fixes the issue by re-arranging
> the codepath where function "transport_get_lun_for_tmr" is called
> after tmr request is allocated and made it available as part of se_cmd.
> 
> Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> Signed-off-by: Kiran Patil <kiran.patil@intel.com>
> Signed-off-by: Robert Love <robert.w.love@intel.com>

Hi Robert,

Thanks for the follow up on this.  One comment below..

> ---
>  drivers/target/tcm_fc/tcm_fc.h  |    2 +
>  drivers/target/tcm_fc/tfc_cmd.c |   62 ++++++++++++++++++++-------------------
>  2 files changed, 32 insertions(+), 32 deletions(-)
> 

<SNIP>

>  static void ft_queue_cmd(struct ft_sess *sess, struct ft_cmd *cmd)
>  {
>  	struct se_queue_obj *qobj;
> @@ -426,13 +403,6 @@ static void ft_send_tm(struct ft_cmd *cmd)
>  	switch (fcp->fc_tm_flags) {
>  	case FCP_TMF_LUN_RESET:
>  		tm_func = TMR_LUN_RESET;
> -		if (ft_get_lun_for_cmd(cmd, fcp->fc_lun) < 0) {
> -			ft_dump_cmd(cmd, __func__);
> -			transport_send_check_condition_and_sense(&cmd->se_cmd,
> -				cmd->se_cmd.scsi_sense_reason, 0);
> -			ft_sess_put(cmd->sess);
> -			return;
> -		}
>  		break;
>  	case FCP_TMF_TGT_RESET:
>  		tm_func = TMR_TARGET_WARM_RESET;
> @@ -464,6 +434,35 @@ static void ft_send_tm(struct ft_cmd *cmd)
>  		return;
>  	}
>  	cmd->se_cmd.se_tmr_req = tmr;
> +
> +	switch (fcp->fc_tm_flags) {
> +	case FCP_TMF_LUN_RESET:
> +		cmd->lun = scsilun_to_int((struct scsi_lun *)fcp->fc_lun);
> +		if (transport_get_lun_for_tmr(&cmd->se_cmd, cmd->lun) < 0) {
> +			/*
> +			 * Make sure to clean up newly allocated TMR request
> +			 * since "unable to  handle TMR request because failed
> +			 * to get to LUN"
> +			 */
> +			FT_TM_DBG("Failed to get LUN for TMR func %d, "
> +				  "se_cmd %p, unpacked_lun %d\n",
> +				  tm_func, &cmd->se_cmd, cmd->lun);
> +			transport_generic_free_cmd(&cmd->se_cmd, 0, 1, 0);
> +			ft_dump_cmd(cmd, __func__);
> +			transport_send_check_condition_and_sense(&cmd->se_cmd,
> +				cmd->se_cmd.scsi_sense_reason, 0);
> +			ft_sess_put(cmd->sess);
> +			return;

This appears to be incorrect as transport_generic_free_cmd() will
release cmd->se_cmd before transport_send_check_condition_and_sense() is
called.  I think they need to be reversed along the lines of how
ft_send_cmd() failure cases for -ENOMEM and -EINVAL are handled.

Here is a patch for lio-core-2.6.git with TCM v4.1 code to address the
issue, please review and I will plan to respin against mainline and send
off to Linus for -rc5.

Thanks!

--nab

------------------------------------------------------------------------------

diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
index 6333170..1f2e9f5 100644
--- a/drivers/target/tcm_fc/tfc_cmd.c
+++ b/drivers/target/tcm_fc/tfc_cmd.c
@@ -390,6 +390,7 @@ static void ft_send_tm(struct ft_cmd *cmd)
 {
        struct se_tmr_req *tmr;
        struct fcp_cmnd *fcp;
+       struct ft_sess *sess;
        u8 tm_func;
 
        fcp = fc_frame_payload_get(cmd->req_frame, sizeof(*fcp));
@@ -399,10 +400,20 @@ static void ft_send_tm(struct ft_cmd *cmd)
                tm_func = TMR_LUN_RESET;
                cmd->lun = scsilun_to_int((struct scsi_lun *)fcp->fc_lun);
                if (transport_lookup_tmr_lun(&cmd->se_cmd, cmd->lun) < 0) {
+                       /*
+                        * Make sure to clean up newly allocated TMR request
+                        * since "unable to  handle TMR request because failed
+                        * to get to LUN"
+                        */
+                       FT_TM_DBG("Failed to get LUN for TMR func %d, "
+                               "se_cmd %p, unpacked_lun %d\n",
+                               tm_func, &cmd->se_cmd, cmd->lun);
                        ft_dump_cmd(cmd, __func__);
+                       sess = cmd->sess;
                        transport_send_check_condition_and_sense(&cmd->se_cmd,
                                cmd->se_cmd.scsi_sense_reason, 0);
-                       ft_sess_put(cmd->sess);
+                       transport_generic_free_cmd(&cmd->se_cmd, 0, 0);
+                       ft_sess_put(sess);
                        return;
                }
                break;

Re: [PATCH v2 09/12] tcm_fc: Fix ft_send_tm-bug and drop ft_get_lun_for_cmd usage

[Kiran Patil]

Hi Nick,

Good catch and thanks for carefully reviewing this patch. Comments in-line.

On 6/22/2011 11:37 AM, Nicholas A. Bellinger wrote:
> On Mon, 2011-06-20 at 16:59 -0700, Robert Love wrote:
>> From: Kiran Patil<kiran.patil@intel.com>
>>
>> This patch fixes a bug in ft_send_tm() that was incorrectly calling
>> ft_get_lun_for_cmd() ->  transport_get_lun_for_cmd(), instead of using
>> transport_get_lun_for_tmr() for the proper struct se_lun lookup.
>>
>> It also drops the now unnecessary ft_get_lun_for_cmd() code, and uses
>> scsilun_to_int() directly ahead of direct transport_get_lun_for_cmd()
>> and transport_get_lun_for_tmr usage().
>>
>> Patch was reworked due to NULL pointer access in function transport_get_lun_for_tmr(),
>> (NOTE: transport_get_lun_for_tmr access tmr_req pointer which caused
>> panic due to NULL pointer access) This patch fixes the issue by re-arranging
>> the codepath where function "transport_get_lun_for_tmr" is called
>> after tmr request is allocated and made it available as part of se_cmd.
>>
>> Signed-off-by: Nicholas A. Bellinger<nab@linux-iscsi.org>
>> Signed-off-by: Kiran Patil<kiran.patil@intel.com>
>> Signed-off-by: Robert Love<robert.w.love@intel.com>
> Hi Robert,
>
> Thanks for the follow up on this.  One comment below..
>
>> ---
>>   drivers/target/tcm_fc/tcm_fc.h  |    2 +
>>   drivers/target/tcm_fc/tfc_cmd.c |   62 ++++++++++++++++++++-------------------
>>   2 files changed, 32 insertions(+), 32 deletions(-)
>>
> <SNIP>
>
>>   static void ft_queue_cmd(struct ft_sess *sess, struct ft_cmd *cmd)
>>   {
>>   	struct se_queue_obj *qobj;
>> @@ -426,13 +403,6 @@ static void ft_send_tm(struct ft_cmd *cmd)
>>   	switch (fcp->fc_tm_flags) {
>>   	case FCP_TMF_LUN_RESET:
>>   		tm_func = TMR_LUN_RESET;
>> -		if (ft_get_lun_for_cmd(cmd, fcp->fc_lun)<  0) {
>> -			ft_dump_cmd(cmd, __func__);
>> -			transport_send_check_condition_and_sense(&cmd->se_cmd,
>> -				cmd->se_cmd.scsi_sense_reason, 0);
>> -			ft_sess_put(cmd->sess);
>> -			return;
>> -		}
>>   		break;
>>   	case FCP_TMF_TGT_RESET:
>>   		tm_func = TMR_TARGET_WARM_RESET;
>> @@ -464,6 +434,35 @@ static void ft_send_tm(struct ft_cmd *cmd)
>>   		return;
>>   	}
>>   	cmd->se_cmd.se_tmr_req = tmr;
>> +
>> +	switch (fcp->fc_tm_flags) {
>> +	case FCP_TMF_LUN_RESET:
>> +		cmd->lun = scsilun_to_int((struct scsi_lun *)fcp->fc_lun);
>> +		if (transport_get_lun_for_tmr(&cmd->se_cmd, cmd->lun)<  0) {
>> +			/*
>> +			 * Make sure to clean up newly allocated TMR request
>> +			 * since "unable to  handle TMR request because failed
>> +			 * to get to LUN"
>> +			 */
>> +			FT_TM_DBG("Failed to get LUN for TMR func %d, "
>> +				  "se_cmd %p, unpacked_lun %d\n",
>> +				  tm_func,&cmd->se_cmd, cmd->lun);
>> +			transport_generic_free_cmd(&cmd->se_cmd, 0, 1, 0);
>> +			ft_dump_cmd(cmd, __func__);
>> +			transport_send_check_condition_and_sense(&cmd->se_cmd,
>> +				cmd->se_cmd.scsi_sense_reason, 0);
>> +			ft_sess_put(cmd->sess);
>> +			return;
> This appears to be incorrect as transport_generic_free_cmd() will
> release cmd->se_cmd before transport_send_check_condition_and_sense() is
> called.  I think they need to be reversed along the lines of how
> ft_send_cmd() failure cases for -ENOMEM and -EINVAL are handled.

Agree. Fix in progress and shall submit fixed patch soon to address 
first comment.
> Here is a patch for lio-core-2.6.git with TCM v4.1 code to address the
> issue, please review and I will plan to respin against mainline and send
> off to Linus for -rc5.
>
> Thanks!
>
> --nab
>
Thanks,
-- Kiran P.

[PATCH] tcm_fc: Fix ft_send_tm-bug and drop ft_get_lun_for_cmd usage

[Kiran Patil]

From: Kiran Patil <kiran.patil@intel.com>

This patch fixes a bug in ft_send_tm() that was incorrectly calling
ft_get_lun_for_cmd() -> transport_get_lun_for_cmd(), instead of using
transport_get_lun_for_tmr() for the proper struct se_lun lookup.

It also drops the now unnecessary ft_get_lun_for_cmd() code, and uses
scsilun_to_int() directly ahead of direct transport_get_lun_for_cmd()
and transport_get_lun_for_tmr usage().

Patch was reworked due to NULL pointer access in function transport_get_lun_for_tmr(),
(NOTE: transport_get_lun_for_tmr access tmr_req pointer which caused
panic due to NULL pointer access) This patch fixes the issue by re-arranging
the codepath where function "transport_get_lun_for_tmr" is called
after tmr request is allocated and made it available as part of se_cmd.

Patch was reworked based on Nick's comment, where order of transport_generic_free_cmd and
transport_send_check_condition_and_sense needs to be reversed. transport_generic_free_cmd
releases cmd->se_cmd. Address of cmd->se_cmd is passed to transport_send_check_condition_and_sense
hence transport_send_check_condition_and_sense shall be called before transport_generic_free_cmd.

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
Signed-off-by: Patil, Kiran <kiran.patil@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---

 drivers/target/tcm_fc/tcm_fc.h  |    2 +
 drivers/target/tcm_fc/tfc_cmd.c |   64 ++++++++++++++++++++-------------------
 2 files changed, 34 insertions(+), 32 deletions(-)

diff --git a/drivers/target/tcm_fc/tcm_fc.h b/drivers/target/tcm_fc/tcm_fc.h
index defff32..7b82f1b 100644
--- a/drivers/target/tcm_fc/tcm_fc.h
+++ b/drivers/target/tcm_fc/tcm_fc.h
@@ -144,7 +144,7 @@ enum ft_cmd_state {
  */
 struct ft_cmd {
 	enum ft_cmd_state state;
-	u16 lun;			/* LUN from request */
+	u32 lun;                        /* LUN from request */
 	struct ft_sess *sess;		/* session held for cmd */
 	struct fc_seq *seq;		/* sequence in exchange mgr */
 	struct se_cmd se_cmd;		/* Local TCM I/O descriptor */
diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
index 1625b48..b9cea59 100644
--- a/drivers/target/tcm_fc/tfc_cmd.c
+++ b/drivers/target/tcm_fc/tfc_cmd.c
@@ -94,29 +94,6 @@ void ft_dump_cmd(struct ft_cmd *cmd, const char *caller)
 		16, 4, cmd->cdb, MAX_COMMAND_SIZE, 0);
 }
 
-/*
- * Get LUN from CDB.
- */
-static int ft_get_lun_for_cmd(struct ft_cmd *cmd, u8 *lunp)
-{
-	u64 lun;
-
-	lun = lunp[1];
-	switch (lunp[0] >> 6) {
-	case 0:
-		break;
-	case 1:
-		lun |= (lunp[0] & 0x3f) << 8;
-		break;
-	default:
-		return -1;
-	}
-	if (lun >= TRANSPORT_MAX_LUNS_PER_TPG)
-		return -1;
-	cmd->lun = lun;
-	return transport_get_lun_for_cmd(&cmd->se_cmd, NULL, lun);
-}
-
 static void ft_queue_cmd(struct ft_sess *sess, struct ft_cmd *cmd)
 {
 	struct se_queue_obj *qobj;
@@ -419,6 +396,7 @@ static void ft_send_tm(struct ft_cmd *cmd)
 {
 	struct se_tmr_req *tmr;
 	struct fcp_cmnd *fcp;
+	struct ft_sess *sess;
 	u8 tm_func;
 
 	fcp = fc_frame_payload_get(cmd->req_frame, sizeof(*fcp));
@@ -426,13 +404,6 @@ static void ft_send_tm(struct ft_cmd *cmd)
 	switch (fcp->fc_tm_flags) {
 	case FCP_TMF_LUN_RESET:
 		tm_func = TMR_LUN_RESET;
-		if (ft_get_lun_for_cmd(cmd, fcp->fc_lun) < 0) {
-			ft_dump_cmd(cmd, __func__);
-			transport_send_check_condition_and_sense(&cmd->se_cmd,
-				cmd->se_cmd.scsi_sense_reason, 0);
-			ft_sess_put(cmd->sess);
-			return;
-		}
 		break;
 	case FCP_TMF_TGT_RESET:
 		tm_func = TMR_TARGET_WARM_RESET;
@@ -464,6 +435,36 @@ static void ft_send_tm(struct ft_cmd *cmd)
 		return;
 	}
 	cmd->se_cmd.se_tmr_req = tmr;
+
+	switch (fcp->fc_tm_flags) {
+	case FCP_TMF_LUN_RESET:
+		cmd->lun = scsilun_to_int((struct scsi_lun *)fcp->fc_lun);
+		if (transport_get_lun_for_tmr(&cmd->se_cmd, cmd->lun) < 0) {
+			/*
+			 * Make sure to clean up newly allocated TMR request
+			 * since "unable to  handle TMR request because failed
+			 * to get to LUN"
+			 */
+			FT_TM_DBG("Failed to get LUN for TMR func %d, "
+				  "se_cmd %p, unpacked_lun %d\n",
+				  tm_func, &cmd->se_cmd, cmd->lun);
+			ft_dump_cmd(cmd, __func__);
+			sess = cmd->sess;
+			transport_send_check_condition_and_sense(&cmd->se_cmd,
+				cmd->se_cmd.scsi_sense_reason, 0);
+			transport_generic_free_cmd(&cmd->se_cmd, 0, 1, 0);
+			ft_sess_put(sess);
+			return;
+		}
+		break;
+	case FCP_TMF_TGT_RESET:
+	case FCP_TMF_CLR_TASK_SET:
+	case FCP_TMF_ABT_TASK_SET:
+	case FCP_TMF_CLR_ACA:
+		break;
+	default:
+		return;
+	}
 	transport_generic_handle_tmr(&cmd->se_cmd);
 }
 
@@ -636,7 +637,8 @@ static void ft_send_cmd(struct ft_cmd *cmd)
 
 	fc_seq_exch(cmd->seq)->lp->tt.seq_set_resp(cmd->seq, ft_recv_seq, cmd);
 
-	ret = ft_get_lun_for_cmd(cmd, fcp->fc_lun);
+	cmd->lun = scsilun_to_int((struct scsi_lun *)fcp->fc_lun);
+	ret = transport_get_lun_for_cmd(&cmd->se_cmd, NULL, cmd->lun);
 	if (ret < 0) {
 		ft_dump_cmd(cmd, __func__);
 		transport_send_check_condition_and_sense(&cmd->se_cmd,

Re: [PATCH] tcm_fc: Fix ft_send_tm-bug and drop ft_get_lun_for_cmd usage

[Nicholas A. Bellinger]

On Wed, 2011-06-22 at 16:30 -0700, Kiran Patil wrote:
> From: Kiran Patil <kiran.patil@intel.com>
> 
> This patch fixes a bug in ft_send_tm() that was incorrectly calling
> ft_get_lun_for_cmd() -> transport_get_lun_for_cmd(), instead of using
> transport_get_lun_for_tmr() for the proper struct se_lun lookup.
> 
> It also drops the now unnecessary ft_get_lun_for_cmd() code, and uses
> scsilun_to_int() directly ahead of direct transport_get_lun_for_cmd()
> and transport_get_lun_for_tmr usage().
> 
> Patch was reworked due to NULL pointer access in function transport_get_lun_for_tmr(),
> (NOTE: transport_get_lun_for_tmr access tmr_req pointer which caused
> panic due to NULL pointer access) This patch fixes the issue by re-arranging
> the codepath where function "transport_get_lun_for_tmr" is called
> after tmr request is allocated and made it available as part of se_cmd.
> 
> Patch was reworked based on Nick's comment, where order of transport_generic_free_cmd and
> transport_send_check_condition_and_sense needs to be reversed. transport_generic_free_cmd
> releases cmd->se_cmd. Address of cmd->se_cmd is passed to transport_send_check_condition_and_sense
> hence transport_send_check_condition_and_sense shall be called before transport_generic_free_cmd.
> 
> Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> Signed-off-by: Patil, Kiran <kiran.patil@intel.com>
> Signed-off-by: Robert Love <robert.w.love@intel.com>
> ---

Hi Kiran,

Thanks for fixing and reposting.  The equivilent change for handling
transport_get_lun_for_tmr exceptions in lio-core-2.6.git/master has been
fixed following this patch, and i'll send this patch out as a 'GIT PULL'
to Linus for -rc5 tomorrow.

Thanks!

--nab

> 
>  drivers/target/tcm_fc/tcm_fc.h  |    2 +
>  drivers/target/tcm_fc/tfc_cmd.c |   64 ++++++++++++++++++++-------------------
>  2 files changed, 34 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/target/tcm_fc/tcm_fc.h b/drivers/target/tcm_fc/tcm_fc.h
> index defff32..7b82f1b 100644
> --- a/drivers/target/tcm_fc/tcm_fc.h
> +++ b/drivers/target/tcm_fc/tcm_fc.h
> @@ -144,7 +144,7 @@ enum ft_cmd_state {
>   */
>  struct ft_cmd {
>  	enum ft_cmd_state state;
> -	u16 lun;			/* LUN from request */
> +	u32 lun;                        /* LUN from request */
>  	struct ft_sess *sess;		/* session held for cmd */
>  	struct fc_seq *seq;		/* sequence in exchange mgr */
>  	struct se_cmd se_cmd;		/* Local TCM I/O descriptor */
> diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
> index 1625b48..b9cea59 100644
> --- a/drivers/target/tcm_fc/tfc_cmd.c
> +++ b/drivers/target/tcm_fc/tfc_cmd.c
> @@ -94,29 +94,6 @@ void ft_dump_cmd(struct ft_cmd *cmd, const char *caller)
>  		16, 4, cmd->cdb, MAX_COMMAND_SIZE, 0);
>  }
>  
> -/*
> - * Get LUN from CDB.
> - */
> -static int ft_get_lun_for_cmd(struct ft_cmd *cmd, u8 *lunp)
> -{
> -	u64 lun;
> -
> -	lun = lunp[1];
> -	switch (lunp[0] >> 6) {
> -	case 0:
> -		break;
> -	case 1:
> -		lun |= (lunp[0] & 0x3f) << 8;
> -		break;
> -	default:
> -		return -1;
> -	}
> -	if (lun >= TRANSPORT_MAX_LUNS_PER_TPG)
> -		return -1;
> -	cmd->lun = lun;
> -	return transport_get_lun_for_cmd(&cmd->se_cmd, NULL, lun);
> -}
> -
>  static void ft_queue_cmd(struct ft_sess *sess, struct ft_cmd *cmd)
>  {
>  	struct se_queue_obj *qobj;
> @@ -419,6 +396,7 @@ static void ft_send_tm(struct ft_cmd *cmd)
>  {
>  	struct se_tmr_req *tmr;
>  	struct fcp_cmnd *fcp;
> +	struct ft_sess *sess;
>  	u8 tm_func;
>  
>  	fcp = fc_frame_payload_get(cmd->req_frame, sizeof(*fcp));
> @@ -426,13 +404,6 @@ static void ft_send_tm(struct ft_cmd *cmd)
>  	switch (fcp->fc_tm_flags) {
>  	case FCP_TMF_LUN_RESET:
>  		tm_func = TMR_LUN_RESET;
> -		if (ft_get_lun_for_cmd(cmd, fcp->fc_lun) < 0) {
> -			ft_dump_cmd(cmd, __func__);
> -			transport_send_check_condition_and_sense(&cmd->se_cmd,
> -				cmd->se_cmd.scsi_sense_reason, 0);
> -			ft_sess_put(cmd->sess);
> -			return;
> -		}
>  		break;
>  	case FCP_TMF_TGT_RESET:
>  		tm_func = TMR_TARGET_WARM_RESET;
> @@ -464,6 +435,36 @@ static void ft_send_tm(struct ft_cmd *cmd)
>  		return;
>  	}
>  	cmd->se_cmd.se_tmr_req = tmr;
> +
> +	switch (fcp->fc_tm_flags) {
> +	case FCP_TMF_LUN_RESET:
> +		cmd->lun = scsilun_to_int((struct scsi_lun *)fcp->fc_lun);
> +		if (transport_get_lun_for_tmr(&cmd->se_cmd, cmd->lun) < 0) {
> +			/*
> +			 * Make sure to clean up newly allocated TMR request
> +			 * since "unable to  handle TMR request because failed
> +			 * to get to LUN"
> +			 */
> +			FT_TM_DBG("Failed to get LUN for TMR func %d, "
> +				  "se_cmd %p, unpacked_lun %d\n",
> +				  tm_func, &cmd->se_cmd, cmd->lun);
> +			ft_dump_cmd(cmd, __func__);
> +			sess = cmd->sess;
> +			transport_send_check_condition_and_sense(&cmd->se_cmd,
> +				cmd->se_cmd.scsi_sense_reason, 0);
> +			transport_generic_free_cmd(&cmd->se_cmd, 0, 1, 0);
> +			ft_sess_put(sess);
> +			return;
> +		}
> +		break;
> +	case FCP_TMF_TGT_RESET:
> +	case FCP_TMF_CLR_TASK_SET:
> +	case FCP_TMF_ABT_TASK_SET:
> +	case FCP_TMF_CLR_ACA:
> +		break;
> +	default:
> +		return;
> +	}
>  	transport_generic_handle_tmr(&cmd->se_cmd);
>  }
>  
> @@ -636,7 +637,8 @@ static void ft_send_cmd(struct ft_cmd *cmd)
>  
>  	fc_seq_exch(cmd->seq)->lp->tt.seq_set_resp(cmd->seq, ft_recv_seq, cmd);
>  
> -	ret = ft_get_lun_for_cmd(cmd, fcp->fc_lun);
> +	cmd->lun = scsilun_to_int((struct scsi_lun *)fcp->fc_lun);
> +	ret = transport_get_lun_for_cmd(&cmd->se_cmd, NULL, cmd->lun);
>  	if (ret < 0) {
>  		ft_dump_cmd(cmd, __func__);
>  		transport_send_check_condition_and_sense(&cmd->se_cmd,
>