[PATCH 0/2] qla2xxx target fixes

[PATCH 0/2] qla2xxx target fixes

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

Hi everyone, a couple of qla2xxx target fixes that we've been shipping
for a while at Pure and that I've finally got around to cleaning up
and merging to the latest kernel tree...

Roland Dreier (2):
  tcm_qla2xxx: Format VPD page 83h SCSI name string according to SPC
  qla2xxx: Update target lookup session tables when a target session
    changes

 drivers/scsi/qla2xxx/qla_target.c  |   22 +++++------
 drivers/scsi/qla2xxx/qla_target.h  |    1 +
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |   77 +++++++++++++++++++++++++++++++++++-
 drivers/scsi/qla2xxx/tcm_qla2xxx.h |    2 +
 4 files changed, 89 insertions(+), 13 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] tcm_qla2xxx: Format VPD page 83h SCSI name string according to SPC

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

My draft of SPC-4 says the following about the SCSI name string in
inquiry VPD page 83h:

    The SCSI NAME STRING field starts with either:

    a) the four UTF-8 characters 'eui.' concatenated with 16, 24, or
       32 hexadecimal digits (i.e., the UTF-8 characters 0 through 9
       and A through F) for an EUI-64 based identifier (see
       7.8.6.5). The first hexadecimal digit shall be the most
       significant four bits of the first byte (i.e., most significant
       byte) of the EUI-64 based identifier;
    b) the four UTF-8 characters 'naa.' concatenated with 16 or 32
       hexadecimal digits for an NAA identifier (see 7.8.6.6). The
       first hexadecimal digit shall be the most significant four bits
       of the first byte (i.e., most significant byte) of the NAA
       identifier; or
    c) the four UTF-8 characters 'iqn.' concatenated with an iSCSI
       Name for an iSCSI-name based identifier (see iSCSI).

However, the .tpg_get_wwn method for tcm_qla2xxx formats the WWN so
the SCSI name string looks like "52:4a:93:7d:24:5f:b2:12,t,0x0001".
This patch corrects the code so that VPD 83h gives a SPC-compliant
SCSI name string like "naa.524a937d245fb212,t,0x0001" while leavig
other uses alone (so configfs will still work with ':' separated WWNs).

Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |    4 +++-
 drivers/scsi/qla2xxx/tcm_qla2xxx.h |    2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 2358c16..d8211cc 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -237,7 +237,7 @@ static char *tcm_qla2xxx_get_fabric_wwn(struct se_portal_group *se_tpg)
 				struct tcm_qla2xxx_tpg, se_tpg);
 	struct tcm_qla2xxx_lport *lport = tpg->lport;
 
-	return &lport->lport_name[0];
+	return lport->lport_naa_name;
 }
 
 static char *tcm_qla2xxx_npiv_get_fabric_wwn(struct se_portal_group *se_tpg)
@@ -1534,6 +1534,7 @@ static struct se_wwn *tcm_qla2xxx_make_lport(
 	lport->lport_wwpn = wwpn;
 	tcm_qla2xxx_format_wwn(&lport->lport_name[0], TCM_QLA2XXX_NAMELEN,
 				wwpn);
+	sprintf(lport->lport_naa_name, "naa.%016llx", (unsigned long long) wwpn);
 
 	ret = tcm_qla2xxx_init_lport(lport);
 	if (ret != 0)
@@ -1601,6 +1602,7 @@ static struct se_wwn *tcm_qla2xxx_npiv_make_lport(
 	lport->lport_npiv_wwnn = npiv_wwnn;
 	tcm_qla2xxx_npiv_format_wwn(&lport->lport_npiv_name[0],
 			TCM_QLA2XXX_NAMELEN, npiv_wwpn, npiv_wwnn);
+	sprintf(lport->lport_naa_name, "naa.%016llx", (unsigned long long) npiv_wwpn);
 
 /* FIXME: tcm_qla2xxx_npiv_make_lport */
 	ret = -ENOSYS;
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.h b/drivers/scsi/qla2xxx/tcm_qla2xxx.h
index 8254981..9ba075f 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.h
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.h
@@ -61,6 +61,8 @@ struct tcm_qla2xxx_lport {
 	u64 lport_npiv_wwnn;
 	/* ASCII formatted WWPN for FC Target Lport */
 	char lport_name[TCM_QLA2XXX_NAMELEN];
+	/* ASCII formatted naa WWPN for VPD page 83 etc */
+	char lport_naa_name[TCM_QLA2XXX_NAMELEN];
 	/* ASCII formatted WWPN+WWNN for NPIV FC Target Lport */
 	char lport_npiv_name[TCM_QLA2XXX_NPIV_NAMELEN];
 	/* map for fc_port pointers in 24-bit FC Port ID space */
-- 
1.7.10.4

[PATCH 2/2] qla2xxx: Update target lookup session tables when a target session changes

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

It is possible for the target code to change the loop_id or s_id of a
target session in reaction to an FC fabric change.  However, the
session structures are stored in tables that are indexed by these two
keys, and if we just change the session structure but leave the
pointers to it in the old places in the table, havoc can ensue.  For
example, a new session might come along that should go in the old slot
in the table and overwrite the old session pointer.

To handle this, add a new tgt_ops->update_sess() method that also
updates the "by loop_id" and "by s_id" lookup tables when a session
changes, so that the keys where a session pointer is stored in these
tables always matches the keys in the session structure itself.

Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/scsi/qla2xxx/qla_target.c  |   22 +++++------
 drivers/scsi/qla2xxx/qla_target.h  |    1 +
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |   73 ++++++++++++++++++++++++++++++++++++
 3 files changed, 84 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index 0e09d8f..556e941 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -557,6 +557,7 @@ static bool qlt_check_fcport_exist(struct scsi_qla_host *vha,
 	int pmap_len;
 	fc_port_t *fcport;
 	int global_resets;
+	unsigned long flags;
 
 retry:
 	global_resets = atomic_read(&ha->tgt.qla_tgt->tgt_global_resets_count);
@@ -625,10 +626,10 @@ retry:
 	    sess->s_id.b.area, sess->loop_id, fcport->d_id.b.domain,
 	    fcport->d_id.b.al_pa, fcport->d_id.b.area, fcport->loop_id);
 
-	sess->s_id = fcport->d_id;
-	sess->loop_id = fcport->loop_id;
-	sess->conf_compl_supported = !!(fcport->flags &
-	    FCF_CONF_COMP_SUPPORTED);
+	spin_lock_irqsave(&ha->hardware_lock, flags);
+	ha->tgt.tgt_ops->update_sess(sess, fcport->d_id, fcport->loop_id,
+				!!(fcport->flags & FCF_CONF_COMP_SUPPORTED));
+	spin_unlock_irqrestore(&ha->hardware_lock, flags);
 
 	res = true;
 
@@ -740,10 +741,9 @@ static struct qla_tgt_sess *qlt_create_sess(
 				qlt_undelete_sess(sess);
 
 			kref_get(&sess->se_sess->sess_kref);
-			sess->s_id = fcport->d_id;
-			sess->loop_id = fcport->loop_id;
-			sess->conf_compl_supported = !!(fcport->flags &
-			    FCF_CONF_COMP_SUPPORTED);
+			ha->tgt.tgt_ops->update_sess(sess, fcport->d_id, fcport->loop_id,
+						!!(fcport->flags & FCF_CONF_COMP_SUPPORTED));
+
 			if (sess->local && !local)
 				sess->local = 0;
 			spin_unlock_irqrestore(&ha->hardware_lock, flags);
@@ -869,10 +869,8 @@ void qlt_fc_port_added(struct scsi_qla_host *vha, fc_port_t *fcport)
 			ql_dbg(ql_dbg_tgt_mgt, vha, 0xf007,
 			    "Reappeared sess %p\n", sess);
 		}
-		sess->s_id = fcport->d_id;
-		sess->loop_id = fcport->loop_id;
-		sess->conf_compl_supported = !!(fcport->flags &
-		    FCF_CONF_COMP_SUPPORTED);
+		ha->tgt.tgt_ops->update_sess(sess, fcport->d_id, fcport->loop_id,
+					!!(fcport->flags & FCF_CONF_COMP_SUPPORTED));
 	}
 
 	if (sess && sess->local) {
diff --git a/drivers/scsi/qla2xxx/qla_target.h b/drivers/scsi/qla2xxx/qla_target.h
index 170af15..bad7495 100644
--- a/drivers/scsi/qla2xxx/qla_target.h
+++ b/drivers/scsi/qla2xxx/qla_target.h
@@ -648,6 +648,7 @@ struct qla_tgt_func_tmpl {
 
 	int (*check_initiator_node_acl)(struct scsi_qla_host *, unsigned char *,
 					void *, uint8_t *, uint16_t);
+	void (*update_sess)(struct qla_tgt_sess *, port_id_t, uint16_t, bool);
 	struct qla_tgt_sess *(*find_sess_by_loop_id)(struct scsi_qla_host *,
 						const uint16_t);
 	struct qla_tgt_sess *(*find_sess_by_s_id)(struct scsi_qla_host *,
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index d8211cc..3d74f2f 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -1457,6 +1457,78 @@ static int tcm_qla2xxx_check_initiator_node_acl(
 	return 0;
 }
 
+static void tcm_qla2xxx_update_sess(struct qla_tgt_sess *sess, port_id_t s_id,
+				    uint16_t loop_id, bool conf_compl_supported)
+{
+	struct qla_tgt *tgt = sess->tgt;
+	struct qla_hw_data *ha = tgt->ha;
+	struct tcm_qla2xxx_lport *lport = ha->tgt.target_lport_ptr;
+	struct se_node_acl *se_nacl = sess->se_sess->se_node_acl;
+	struct tcm_qla2xxx_nacl *nacl = container_of(se_nacl,
+			struct tcm_qla2xxx_nacl, se_node_acl);
+	u32 key;
+
+
+	if (sess->loop_id != loop_id || sess->s_id.b24 != s_id.b24)
+		pr_info("Updating session %p from port %02x:%02x:%02x:%02x:%02x:%02x:%02x:%02x loop_id %d -> %d s_id %x:%x:%x -> %x:%x:%x\n",
+			sess,
+			sess->port_name[0], sess->port_name[1],
+			sess->port_name[2], sess->port_name[3],
+			sess->port_name[4], sess->port_name[5],
+			sess->port_name[6], sess->port_name[7],
+			sess->loop_id, loop_id,
+			sess->s_id.b.domain, sess->s_id.b.area, sess->s_id.b.al_pa,
+			s_id.b.domain, s_id.b.area, s_id.b.al_pa);
+
+	if (sess->loop_id != loop_id) {
+		/*
+		 * Because we can shuffle loop IDs around and we
+		 * update different sessions non-atomically, we might
+		 * have overwritten this session's old loop ID
+		 * already, and we might end up overwriting some other
+		 * session that will be updated later.  So we have to
+		 * be extra careful and we can't warn about those things...
+		 */
+		if (lport->lport_loopid_map[sess->loop_id].se_nacl == se_nacl)
+			lport->lport_loopid_map[sess->loop_id].se_nacl = NULL;
+
+		lport->lport_loopid_map[loop_id].se_nacl = se_nacl;
+
+		sess->loop_id = loop_id;
+	}
+
+	if (sess->s_id.b24 != s_id.b24) {
+		key = (((u32) sess->s_id.b.domain << 16) |
+		       ((u32) sess->s_id.b.area   <<  8) |
+		       ((u32) sess->s_id.b.al_pa));
+
+		if (btree_lookup32(&lport->lport_fcport_map, key))
+			WARN(btree_remove32(&lport->lport_fcport_map, key) != se_nacl,
+			     "Found wrong se_nacl when updating s_id %x:%x:%x\n",
+			     sess->s_id.b.domain, sess->s_id.b.area, sess->s_id.b.al_pa);
+		else
+			WARN(1, "No lport_fcport_map entry for s_id %x:%x:%x\n",
+			     sess->s_id.b.domain, sess->s_id.b.area, sess->s_id.b.al_pa);
+
+		key = (((u32) s_id.b.domain << 16) |
+		       ((u32) s_id.b.area   <<  8) |
+		       ((u32) s_id.b.al_pa));
+
+		if (btree_lookup32(&lport->lport_fcport_map, key)) {
+			WARN(1, "Already have lport_fcport_map entry for s_id %x:%x:%x\n",
+			     s_id.b.domain, s_id.b.area, s_id.b.al_pa);
+			btree_update32(&lport->lport_fcport_map, key, se_nacl);
+		} else {
+			btree_insert32(&lport->lport_fcport_map, key, se_nacl, GFP_ATOMIC);
+		}
+
+		sess->s_id = s_id;
+		nacl->nport_id = key;
+	}
+
+	sess->conf_compl_supported = conf_compl_supported;
+}
+
 /*
  * Calls into tcm_qla2xxx used by qla2xxx LLD I/O path.
  */
@@ -1467,6 +1539,7 @@ static struct qla_tgt_func_tmpl tcm_qla2xxx_template = {
 	.free_cmd		= tcm_qla2xxx_free_cmd,
 	.free_mcmd		= tcm_qla2xxx_free_mcmd,
 	.free_session		= tcm_qla2xxx_free_session,
+	.update_sess		= tcm_qla2xxx_update_sess,
 	.check_initiator_node_acl = tcm_qla2xxx_check_initiator_node_acl,
 	.find_sess_by_s_id	= tcm_qla2xxx_find_sess_by_s_id,
 	.find_sess_by_loop_id	= tcm_qla2xxx_find_sess_by_loop_id,
-- 
1.7.10.4

Re: [PATCH 1/2] tcm_qla2xxx: Format VPD page 83h SCSI name string according to SPC

[Nicholas A. Bellinger]

On Thu, 2012-10-11 at 13:41 -0700, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> My draft of SPC-4 says the following about the SCSI name string in
> inquiry VPD page 83h:
> 
>     The SCSI NAME STRING field starts with either:
> 
>     a) the four UTF-8 characters 'eui.' concatenated with 16, 24, or
>        32 hexadecimal digits (i.e., the UTF-8 characters 0 through 9
>        and A through F) for an EUI-64 based identifier (see
>        7.8.6.5). The first hexadecimal digit shall be the most
>        significant four bits of the first byte (i.e., most significant
>        byte) of the EUI-64 based identifier;
>     b) the four UTF-8 characters 'naa.' concatenated with 16 or 32
>        hexadecimal digits for an NAA identifier (see 7.8.6.6). The
>        first hexadecimal digit shall be the most significant four bits
>        of the first byte (i.e., most significant byte) of the NAA
>        identifier; or
>     c) the four UTF-8 characters 'iqn.' concatenated with an iSCSI
>        Name for an iSCSI-name based identifier (see iSCSI).
> 
> However, the .tpg_get_wwn method for tcm_qla2xxx formats the WWN so
> the SCSI name string looks like "52:4a:93:7d:24:5f:b2:12,t,0x0001".
> This patch corrects the code so that VPD 83h gives a SPC-compliant
> SCSI name string like "naa.524a937d245fb212,t,0x0001" while leavig
> other uses alone (so configfs will still work with ':' separated WWNs).
> 
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---

Hi Roland,

Apologies for the delayed response on this patch.

Applied to target-pending/master and queued for v3.7-rc fixes

Thanks!

Re: [PATCH 2/2] qla2xxx: Update target lookup session tables when a target session changes

[Nicholas A. Bellinger]

On Thu, 2012-10-11 at 13:41 -0700, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> It is possible for the target code to change the loop_id or s_id of a
> target session in reaction to an FC fabric change.  However, the
> session structures are stored in tables that are indexed by these two
> keys, and if we just change the session structure but leave the
> pointers to it in the old places in the table, havoc can ensue.  For
> example, a new session might come along that should go in the old slot
> in the table and overwrite the old session pointer.
> 
> To handle this, add a new tgt_ops->update_sess() method that also
> updates the "by loop_id" and "by s_id" lookup tables when a session
> changes, so that the keys where a session pointer is stored in these
> tables always matches the keys in the session structure itself.
> 
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---

Nice work tracking down this long standing bug with fc_port_t handling !

Queued for v3.7-rc fixes with a CC' to stable.

One extra bit below..

>  drivers/scsi/qla2xxx/qla_target.c  |   22 +++++------
>  drivers/scsi/qla2xxx/qla_target.h  |    1 +
>  drivers/scsi/qla2xxx/tcm_qla2xxx.c |   73 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 84 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> index 0e09d8f..556e941 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
> @@ -557,6 +557,7 @@ static bool qlt_check_fcport_exist(struct scsi_qla_host *vha,
>  	int pmap_len;
>  	fc_port_t *fcport;
>  	int global_resets;
> +	unsigned long flags;
>  
>  retry:
>  	global_resets = atomic_read(&ha->tgt.qla_tgt->tgt_global_resets_count);
> @@ -625,10 +626,10 @@ retry:
>  	    sess->s_id.b.area, sess->loop_id, fcport->d_id.b.domain,
>  	    fcport->d_id.b.al_pa, fcport->d_id.b.area, fcport->loop_id);
>  
> -	sess->s_id = fcport->d_id;
> -	sess->loop_id = fcport->loop_id;
> -	sess->conf_compl_supported = !!(fcport->flags &
> -	    FCF_CONF_COMP_SUPPORTED);
> +	spin_lock_irqsave(&ha->hardware_lock, flags);
> +	ha->tgt.tgt_ops->update_sess(sess, fcport->d_id, fcport->loop_id,
> +				!!(fcport->flags & FCF_CONF_COMP_SUPPORTED));
> +	spin_unlock_irqrestore(&ha->hardware_lock, flags);
>  

Dropping the extra unnecessary '!!' double inversion check here around
FCF_CONF_COMP_SUPPORTED usage, and folding into the original patch.

>  	res = true;
>  
> @@ -740,10 +741,9 @@ static struct qla_tgt_sess *qlt_create_sess(
>  				qlt_undelete_sess(sess);
>  
>  			kref_get(&sess->se_sess->sess_kref);
> -			sess->s_id = fcport->d_id;
> -			sess->loop_id = fcport->loop_id;
> -			sess->conf_compl_supported = !!(fcport->flags &
> -			    FCF_CONF_COMP_SUPPORTED);
> +			ha->tgt.tgt_ops->update_sess(sess, fcport->d_id, fcport->loop_id,
> +						!!(fcport->flags & FCF_CONF_COMP_SUPPORTED));
> +

Ditto

>  			if (sess->local && !local)
>  				sess->local = 0;
>  			spin_unlock_irqrestore(&ha->hardware_lock, flags);
> @@ -869,10 +869,8 @@ void qlt_fc_port_added(struct scsi_qla_host *vha, fc_port_t *fcport)
>  			ql_dbg(ql_dbg_tgt_mgt, vha, 0xf007,
>  			    "Reappeared sess %p\n", sess);
>  		}
> -		sess->s_id = fcport->d_id;
> -		sess->loop_id = fcport->loop_id;
> -		sess->conf_compl_supported = !!(fcport->flags &
> -		    FCF_CONF_COMP_SUPPORTED);
> +		ha->tgt.tgt_ops->update_sess(sess, fcport->d_id, fcport->loop_id,
> +					!!(fcport->flags & FCF_CONF_COMP_SUPPORTED));
>  	}
>  

And here too..

Thanks Roland!