diff mbox

[1/3] qla2xxx: Fixup locking for session deletion

Message ID 20180222084937.59664-2-hare@suse.de (mailing list archive)
State Accepted
Headers show

Commit Message

Hannes Reinecke Feb. 22, 2018, 8:49 a.m. UTC 
Commit d8630bb95f46 ('Serialize session deletion by using work_lock') tries
to fixup a deadlock when deleting sessions, but fails to take
into account the locking rules. This patch resolves the situation
by introducing a separate lock for processing the GNLIST response, and
ensures that sess_lock is released before calling
qlt_schedule_sess_delete().

Cc: Himanshu Madhani <himanshu.madhani@cavium.com>
Cc: Quinn Tran <quinn.tran@cavium.com>
Fixes: d8630bb95f46 ("scsi: qla2xxx: Serialize session deletion by using work_lock")
Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/qla2xxx/qla_def.h    |  4 ++--
 drivers/scsi/qla2xxx/qla_init.c   | 24 +++++++++++++++---------
 drivers/scsi/qla2xxx/qla_os.c     |  7 ++++++-
 drivers/scsi/qla2xxx/qla_target.c | 17 ++++++-----------
 4 files changed, 29 insertions(+), 23 deletions(-)

Comments

Madhani, Himanshu Feb. 22, 2018, 6:01 p.m. UTC | #1 
On Thu, 22 Feb 2018, 12:49am, Hannes Reinecke wrote:

> Commit d8630bb95f46 ('Serialize session deletion by using work_lock') tries
> to fixup a deadlock when deleting sessions, but fails to take
> into account the locking rules. This patch resolves the situation
> by introducing a separate lock for processing the GNLIST response, and
> ensures that sess_lock is released before calling
> qlt_schedule_sess_delete().
> 
> Cc: Himanshu Madhani <himanshu.madhani@cavium.com>
> Cc: Quinn Tran <quinn.tran@cavium.com>
> Fixes: d8630bb95f46 ("scsi: qla2xxx: Serialize session deletion by using work_lock")
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/qla2xxx/qla_def.h    |  4 ++--
>  drivers/scsi/qla2xxx/qla_init.c   | 24 +++++++++++++++---------
>  drivers/scsi/qla2xxx/qla_os.c     |  7 ++++++-
>  drivers/scsi/qla2xxx/qla_target.c | 17 ++++++-----------
>  4 files changed, 29 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index be7d6824581a..3ca4b6a5eddd 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
> @@ -261,9 +261,9 @@
>  struct name_list_extended {
>  	struct get_name_list_extended *l;
>  	dma_addr_t		ldma;
> -	struct list_head 	fcports;	/* protect by sess_list */
> +	struct list_head	fcports;
> +	spinlock_t		fcports_lock;
>  	u32			size;
> -	u8			sent;
>  };
>  /*
>   * Timeout timer counts in seconds
> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index 590aa904fdef..4dd897c2c2b1 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
> @@ -660,8 +660,7 @@ qla24xx_async_gnl_sp_done(void *s, int res)
>  		    (loop_id & 0x7fff));
>  	}
>  
> -	spin_lock_irqsave(&vha->hw->tgt.sess_lock, flags);
> -	vha->gnl.sent = 0;
> +	spin_lock_irqsave(&vha->gnl.fcports_lock, flags);
>  
>  	INIT_LIST_HEAD(&h);
>  	fcport = tf = NULL;
> @@ -670,12 +669,16 @@ qla24xx_async_gnl_sp_done(void *s, int res)
>  
>  	list_for_each_entry_safe(fcport, tf, &h, gnl_entry) {
>  		list_del_init(&fcport->gnl_entry);
> +		spin_lock(&vha->hw->tgt.sess_lock);
>  		fcport->flags &= ~(FCF_ASYNC_SENT | FCF_ASYNC_ACTIVE);
> +		spin_unlock(&vha->hw->tgt.sess_lock);
>  		ea.fcport = fcport;
>  
>  		qla2x00_fcport_event_handler(vha, &ea);
>  	}
> +	spin_unlock_irqrestore(&vha->gnl.fcports_lock, flags);
>  
> +	spin_lock_irqsave(&vha->hw->tgt.sess_lock, flags);
>  	/* create new fcport if fw has knowledge of new sessions */
>  	for (i = 0; i < n; i++) {
>  		port_id_t id;
> @@ -727,18 +730,21 @@ int qla24xx_async_gnl(struct scsi_qla_host *vha, fc_port_t *fcport)
>  	ql_dbg(ql_dbg_disc, vha, 0x20d9,
>  	    "Async-gnlist WWPN %8phC \n", fcport->port_name);
>  
> -	spin_lock_irqsave(&vha->hw->tgt.sess_lock, flags);
> +	spin_lock_irqsave(&vha->gnl.fcports_lock, flags);
> +	if (!list_empty(&fcport->gnl_entry)) {
> +		spin_unlock_irqrestore(&vha->gnl.fcports_lock, flags);
> +		rval = QLA_SUCCESS;
> +		goto done;
> +	}
> +
> +	spin_lock(&vha->hw->tgt.sess_lock);
>  	fcport->disc_state = DSC_GNL;
>  	fcport->last_rscn_gen = fcport->rscn_gen;
>  	fcport->last_login_gen = fcport->login_gen;
> +	spin_unlock(&vha->hw->tgt.sess_lock);
>  
>  	list_add_tail(&fcport->gnl_entry, &vha->gnl.fcports);
> -	if (vha->gnl.sent) {
> -		spin_unlock_irqrestore(&vha->hw->tgt.sess_lock, flags);
> -		return QLA_SUCCESS;
> -	}
> -	vha->gnl.sent = 1;
> -	spin_unlock_irqrestore(&vha->hw->tgt.sess_lock, flags);
> +	spin_unlock_irqrestore(&vha->gnl.fcports_lock, flags);
>  
>  	sp = qla2x00_get_sp(vha, fcport, GFP_KERNEL);
>  	if (!sp)
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index 12ee6e02d146..dc2b188fdce1 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -4575,6 +4575,7 @@ struct scsi_qla_host *qla2x00_create_host(struct scsi_host_template *sht,
>  
>  	spin_lock_init(&vha->work_lock);
>  	spin_lock_init(&vha->cmd_list_lock);
> +	spin_lock_init(&vha->gnl.fcports_lock);
>  	init_waitqueue_head(&vha->fcport_waitQ);
>  	init_waitqueue_head(&vha->vref_waitq);
>  
> @@ -4875,6 +4876,8 @@ void qla24xx_create_new_sess(struct scsi_qla_host *vha, struct qla_work_evt *e)
>  			}
>  			qlt_plogi_ack_unref(vha, pla);
>  		} else {
> +			fc_port_t *dfcp = NULL;
> +
>  			spin_lock_irqsave(&vha->hw->tgt.sess_lock, flags);
>  			tfcp = qla2x00_find_fcport_by_nportid(vha,
>  			    &e->u.new_sess.id, 1);
> @@ -4897,11 +4900,13 @@ void qla24xx_create_new_sess(struct scsi_qla_host *vha, struct qla_work_evt *e)
>  				default:
>  					fcport->login_pause = 1;
>  					tfcp->conflict = fcport;
> -					qlt_schedule_sess_for_deletion(tfcp);
> +					dfcp = tfcp;
>  					break;
>  				}
>  			}
>  			spin_unlock_irqrestore(&vha->hw->tgt.sess_lock, flags);
> +			if (dfcp)
> +				qlt_schedule_sess_for_deletion(tfcp);
>  
>  			wwn = wwn_to_u64(fcport->node_name);
>  
> diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> index 3735ebd83012..990ef1183f3a 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
> @@ -1224,10 +1224,10 @@ static void qla24xx_chk_fcp_state(struct fc_port *sess)
>  	}
>  }
>  
> -/* ha->tgt.sess_lock supposed to be held on entry */
>  void qlt_schedule_sess_for_deletion(struct fc_port *sess)
>  {
>  	struct qla_tgt *tgt = sess->tgt;
> +	struct qla_hw_data *ha = sess->vha->hw;
>  	unsigned long flags;
>  
>  	if (sess->disc_state == DSC_DELETE_PEND)
> @@ -1244,16 +1244,16 @@ void qlt_schedule_sess_for_deletion(struct fc_port *sess)
>  			return;
>  	}
>  
> +	spin_lock_irqsave(&ha->tgt.sess_lock, flags);
>  	if (sess->deleted == QLA_SESS_DELETED)
>  		sess->logout_on_delete = 0;
>  
> -	spin_lock_irqsave(&sess->vha->work_lock, flags);
>  	if (sess->deleted == QLA_SESS_DELETION_IN_PROGRESS) {
> -		spin_unlock_irqrestore(&sess->vha->work_lock, flags);
> +		spin_unlock_irqrestore(&ha->tgt.sess_lock, flags);
>  		return;
>  	}
>  	sess->deleted = QLA_SESS_DELETION_IN_PROGRESS;
> -	spin_unlock_irqrestore(&sess->vha->work_lock, flags);
> +	spin_unlock_irqrestore(&ha->tgt.sess_lock, flags);
>  
>  	sess->disc_state = DSC_DELETE_PEND;
>  
> @@ -1262,13 +1262,10 @@ void qlt_schedule_sess_for_deletion(struct fc_port *sess)
>  	ql_dbg(ql_dbg_tgt, sess->vha, 0xe001,
>  	    "Scheduling sess %p for deletion\n", sess);
>  
> -	/* use cancel to push work element through before re-queue */
> -	cancel_work_sync(&sess->del_work);
>  	INIT_WORK(&sess->del_work, qla24xx_delete_sess_fn);
> -	queue_work(sess->vha->hw->wq, &sess->del_work);
> +	WARN_ON(!queue_work(sess->vha->hw->wq, &sess->del_work));
>  }
>  
> -/* ha->tgt.sess_lock supposed to be held on entry */
>  static void qlt_clear_tgt_db(struct qla_tgt *tgt)
>  {
>  	struct fc_port *sess;
> @@ -1451,8 +1448,8 @@ qlt_fc_port_deleted(struct scsi_qla_host *vha, fc_port_t *fcport, int max_gen)
>  	ql_dbg(ql_dbg_tgt_mgt, vha, 0xf008, "qla_tgt_fc_port_deleted %p", sess);
>  
>  	sess->local = 1;
> -	qlt_schedule_sess_for_deletion(sess);
>  	spin_unlock_irqrestore(&vha->hw->tgt.sess_lock, flags);
> +	qlt_schedule_sess_for_deletion(sess);
>  }
>  
>  static inline int test_tgt_sess_count(struct qla_tgt *tgt)
> @@ -1512,10 +1509,8 @@ int qlt_stop_phase1(struct qla_tgt *tgt)
>  	 * Lock is needed, because we still can get an incoming packet.
>  	 */
>  	mutex_lock(&vha->vha_tgt.tgt_mutex);
> -	spin_lock_irqsave(&ha->tgt.sess_lock, flags);
>  	tgt->tgt_stop = 1;
>  	qlt_clear_tgt_db(tgt);
> -	spin_unlock_irqrestore(&ha->tgt.sess_lock, flags);
>  	mutex_unlock(&vha->vha_tgt.tgt_mutex);
>  	mutex_unlock(&qla_tgt_mutex);
>  
> 

Thanks for the patch. Looks good.

Acked-by: Himanshu Madhani <himanshu.madhani@cavium.com>
diff mbox

Patch

diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
index be7d6824581a..3ca4b6a5eddd 100644
--- a/drivers/scsi/qla2xxx/qla_def.h
+++ b/drivers/scsi/qla2xxx/qla_def.h
@@ -261,9 +261,9 @@ 
 struct name_list_extended {
 	struct get_name_list_extended *l;
 	dma_addr_t		ldma;
-	struct list_head 	fcports;	/* protect by sess_list */
+	struct list_head	fcports;
+	spinlock_t		fcports_lock;
 	u32			size;
-	u8			sent;
 };
 /*
  * Timeout timer counts in seconds
diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index 590aa904fdef..4dd897c2c2b1 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -660,8 +660,7 @@  qla24xx_async_gnl_sp_done(void *s, int res)
 		    (loop_id & 0x7fff));
 	}
 
-	spin_lock_irqsave(&vha->hw->tgt.sess_lock, flags);
-	vha->gnl.sent = 0;
+	spin_lock_irqsave(&vha->gnl.fcports_lock, flags);
 
 	INIT_LIST_HEAD(&h);
 	fcport = tf = NULL;
@@ -670,12 +669,16 @@  qla24xx_async_gnl_sp_done(void *s, int res)
 
 	list_for_each_entry_safe(fcport, tf, &h, gnl_entry) {
 		list_del_init(&fcport->gnl_entry);
+		spin_lock(&vha->hw->tgt.sess_lock);
 		fcport->flags &= ~(FCF_ASYNC_SENT | FCF_ASYNC_ACTIVE);
+		spin_unlock(&vha->hw->tgt.sess_lock);
 		ea.fcport = fcport;
 
 		qla2x00_fcport_event_handler(vha, &ea);
 	}
+	spin_unlock_irqrestore(&vha->gnl.fcports_lock, flags);
 
+	spin_lock_irqsave(&vha->hw->tgt.sess_lock, flags);
 	/* create new fcport if fw has knowledge of new sessions */
 	for (i = 0; i < n; i++) {
 		port_id_t id;
@@ -727,18 +730,21 @@  int qla24xx_async_gnl(struct scsi_qla_host *vha, fc_port_t *fcport)
 	ql_dbg(ql_dbg_disc, vha, 0x20d9,
 	    "Async-gnlist WWPN %8phC \n", fcport->port_name);
 
-	spin_lock_irqsave(&vha->hw->tgt.sess_lock, flags);
+	spin_lock_irqsave(&vha->gnl.fcports_lock, flags);
+	if (!list_empty(&fcport->gnl_entry)) {
+		spin_unlock_irqrestore(&vha->gnl.fcports_lock, flags);
+		rval = QLA_SUCCESS;
+		goto done;
+	}
+
+	spin_lock(&vha->hw->tgt.sess_lock);
 	fcport->disc_state = DSC_GNL;
 	fcport->last_rscn_gen = fcport->rscn_gen;
 	fcport->last_login_gen = fcport->login_gen;
+	spin_unlock(&vha->hw->tgt.sess_lock);
 
 	list_add_tail(&fcport->gnl_entry, &vha->gnl.fcports);
-	if (vha->gnl.sent) {
-		spin_unlock_irqrestore(&vha->hw->tgt.sess_lock, flags);
-		return QLA_SUCCESS;
-	}
-	vha->gnl.sent = 1;
-	spin_unlock_irqrestore(&vha->hw->tgt.sess_lock, flags);
+	spin_unlock_irqrestore(&vha->gnl.fcports_lock, flags);
 
 	sp = qla2x00_get_sp(vha, fcport, GFP_KERNEL);
 	if (!sp)
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 12ee6e02d146..dc2b188fdce1 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -4575,6 +4575,7 @@  struct scsi_qla_host *qla2x00_create_host(struct scsi_host_template *sht,
 
 	spin_lock_init(&vha->work_lock);
 	spin_lock_init(&vha->cmd_list_lock);
+	spin_lock_init(&vha->gnl.fcports_lock);
 	init_waitqueue_head(&vha->fcport_waitQ);
 	init_waitqueue_head(&vha->vref_waitq);
 
@@ -4875,6 +4876,8 @@  void qla24xx_create_new_sess(struct scsi_qla_host *vha, struct qla_work_evt *e)
 			}
 			qlt_plogi_ack_unref(vha, pla);
 		} else {
+			fc_port_t *dfcp = NULL;
+
 			spin_lock_irqsave(&vha->hw->tgt.sess_lock, flags);
 			tfcp = qla2x00_find_fcport_by_nportid(vha,
 			    &e->u.new_sess.id, 1);
@@ -4897,11 +4900,13 @@  void qla24xx_create_new_sess(struct scsi_qla_host *vha, struct qla_work_evt *e)
 				default:
 					fcport->login_pause = 1;
 					tfcp->conflict = fcport;
-					qlt_schedule_sess_for_deletion(tfcp);
+					dfcp = tfcp;
 					break;
 				}
 			}
 			spin_unlock_irqrestore(&vha->hw->tgt.sess_lock, flags);
+			if (dfcp)
+				qlt_schedule_sess_for_deletion(tfcp);
 
 			wwn = wwn_to_u64(fcport->node_name);
 
diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index 3735ebd83012..990ef1183f3a 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -1224,10 +1224,10 @@  static void qla24xx_chk_fcp_state(struct fc_port *sess)
 	}
 }
 
-/* ha->tgt.sess_lock supposed to be held on entry */
 void qlt_schedule_sess_for_deletion(struct fc_port *sess)
 {
 	struct qla_tgt *tgt = sess->tgt;
+	struct qla_hw_data *ha = sess->vha->hw;
 	unsigned long flags;
 
 	if (sess->disc_state == DSC_DELETE_PEND)
@@ -1244,16 +1244,16 @@  void qlt_schedule_sess_for_deletion(struct fc_port *sess)
 			return;
 	}
 
+	spin_lock_irqsave(&ha->tgt.sess_lock, flags);
 	if (sess->deleted == QLA_SESS_DELETED)
 		sess->logout_on_delete = 0;
 
-	spin_lock_irqsave(&sess->vha->work_lock, flags);
 	if (sess->deleted == QLA_SESS_DELETION_IN_PROGRESS) {
-		spin_unlock_irqrestore(&sess->vha->work_lock, flags);
+		spin_unlock_irqrestore(&ha->tgt.sess_lock, flags);
 		return;
 	}
 	sess->deleted = QLA_SESS_DELETION_IN_PROGRESS;
-	spin_unlock_irqrestore(&sess->vha->work_lock, flags);
+	spin_unlock_irqrestore(&ha->tgt.sess_lock, flags);
 
 	sess->disc_state = DSC_DELETE_PEND;
 
@@ -1262,13 +1262,10 @@  void qlt_schedule_sess_for_deletion(struct fc_port *sess)
 	ql_dbg(ql_dbg_tgt, sess->vha, 0xe001,
 	    "Scheduling sess %p for deletion\n", sess);
 
-	/* use cancel to push work element through before re-queue */
-	cancel_work_sync(&sess->del_work);
 	INIT_WORK(&sess->del_work, qla24xx_delete_sess_fn);
-	queue_work(sess->vha->hw->wq, &sess->del_work);
+	WARN_ON(!queue_work(sess->vha->hw->wq, &sess->del_work));
 }
 
-/* ha->tgt.sess_lock supposed to be held on entry */
 static void qlt_clear_tgt_db(struct qla_tgt *tgt)
 {
 	struct fc_port *sess;
@@ -1451,8 +1448,8 @@  qlt_fc_port_deleted(struct scsi_qla_host *vha, fc_port_t *fcport, int max_gen)
 	ql_dbg(ql_dbg_tgt_mgt, vha, 0xf008, "qla_tgt_fc_port_deleted %p", sess);
 
 	sess->local = 1;
-	qlt_schedule_sess_for_deletion(sess);
 	spin_unlock_irqrestore(&vha->hw->tgt.sess_lock, flags);
+	qlt_schedule_sess_for_deletion(sess);
 }
 
 static inline int test_tgt_sess_count(struct qla_tgt *tgt)
@@ -1512,10 +1509,8 @@  int qlt_stop_phase1(struct qla_tgt *tgt)
 	 * Lock is needed, because we still can get an incoming packet.
 	 */
 	mutex_lock(&vha->vha_tgt.tgt_mutex);
-	spin_lock_irqsave(&ha->tgt.sess_lock, flags);
 	tgt->tgt_stop = 1;
 	qlt_clear_tgt_db(tgt);
-	spin_unlock_irqrestore(&ha->tgt.sess_lock, flags);
 	mutex_unlock(&vha->vha_tgt.tgt_mutex);
 	mutex_unlock(&qla_tgt_mutex);