[Open-FCoE] [PATCH] libfc, fcoe: fixed locking issues with lport->lp_mutex around lport->link_status

Vasu Dev vasu.dev at intel.com
Thu Jan 15 02:17:23 UTC 2009


The fcoe_xmit could call fc_pause in case the pending skb queue len is larger
than FCOE_MAX_QUEUE_DEPTH, the fc_pause was trying to grab lport->lp_muex to
change lport->link_status and that caused these issues :-

1. The fcoe_xmit was getting called with bh disabled, thus casing
"BUG: scheduling while atomic" when grabbing lport->lp_muex while bh disabled.

2. fc_linkup and fc_linkdown function calls lport_enter function with
lport->lp_mutex held and these enter function in turn calls fcoe_xmit to send
lport related FC frame, for instance fc_linkup calling fc_lport_enter_flogi
to send flogi req. In this call flow grabbing same lport->lp_mutex in
fc_puase from fcoe_xmit would cause deadlock.

The lport->lp_mutex required to set FC_PAUSE in fcoe_xmit path but
I don't see FC_PAUSE directly used anywhere beside just setting and clear this
bit in lport->link_status. Shouldn't we report SCSI_MLQUEUE_DEVICE_BUSY or
SCSI_MLQUEUE_HOST_BUSY for any new command while FC_PUASE set?

I'm not sure that lock is really required for lport->link_status but for now
changed this to atomic to avoid above listed two issues but I'll look into
eliminating this atomic since this is used from fc_queuecommand. If FC_PAUSE
is really required then I could add a separate field for this to eliminate
locking need for least FC_PAUSE bit setting.

Signed-off-by: Vasu Dev <vasu.dev at intel.com>
---

 drivers/scsi/fcoe/fcoe_sw.c   |    8 +++++---
 drivers/scsi/fcoe/libfcoe.c   |    6 +++---
 drivers/scsi/libfc/fc_fcp.c   |    5 +++--
 drivers/scsi/libfc/fc_lport.c |   40 ++++++++++++++++++++++++----------------
 include/scsi/libfc.h          |    2 +-
 5 files changed, 36 insertions(+), 25 deletions(-)


diff --git a/drivers/scsi/fcoe/fcoe_sw.c b/drivers/scsi/fcoe/fcoe_sw.c
index dc4cd5e..ac10f0f 100644
--- a/drivers/scsi/fcoe/fcoe_sw.c
+++ b/drivers/scsi/fcoe/fcoe_sw.c
@@ -116,7 +116,7 @@ static int fcoe_sw_lport_config(struct fc_lport *lp)
 {
 	int i = 0;
 
-	lp->link_status = 0;
+	atomic_set(&lp->link_status, 0);
 	lp->max_retry_count = 3;
 	lp->e_d_tov = 2 * 1000;	/* FC-FS default */
 	lp->r_a_tov = 2 * 2 * 1000;
@@ -153,6 +153,7 @@ static int fcoe_sw_netdev_config(struct fc_lport *lp, struct net_device *netdev)
 	u64 wwnn, wwpn;
 	struct fcoe_softc *fc;
 	u8 flogi_maddr[ETH_ALEN];
+	int status;
 
 	/* Setup lport private data to point to fcoe softc */
 	fc = lport_priv(lp);
@@ -181,9 +182,10 @@ static int fcoe_sw_netdev_config(struct fc_lport *lp, struct net_device *netdev)
 	if (fc_set_mfs(lp, mfs))
 		return -EINVAL;
 
-	lp->link_status = ~FC_PAUSE & ~FC_LINK_UP;
+	status =  ~FC_PAUSE & ~FC_LINK_UP;
 	if (!fcoe_link_ok(lp))
-		lp->link_status |= FC_LINK_UP;
+		status |= FC_LINK_UP;
+	atomic_set(&lp->link_status, status);
 
 	/* offload features support */
 	if (fc->real_dev->features & NETIF_F_SG)
diff --git a/drivers/scsi/fcoe/libfcoe.c b/drivers/scsi/fcoe/libfcoe.c
index e419f48..50cbf50 100644
--- a/drivers/scsi/fcoe/libfcoe.c
+++ b/drivers/scsi/fcoe/libfcoe.c
@@ -873,7 +873,7 @@ static int fcoe_device_notification(struct notifier_block *notifier,
 	struct net_device *real_dev = ptr;
 	struct fcoe_softc *fc;
 	struct fcoe_dev_stats *stats;
-	u16 new_status;
+	int new_status;
 	u32 mfs;
 	int rc = NOTIFY_OK;
 
@@ -890,7 +890,7 @@ static int fcoe_device_notification(struct notifier_block *notifier,
 		goto out;
 	}
 
-	new_status = lp->link_status;
+	new_status = atomic_read(&lp->link_status);
 	switch (event) {
 	case NETDEV_DOWN:
 	case NETDEV_GOING_DOWN:
@@ -917,7 +917,7 @@ static int fcoe_device_notification(struct notifier_block *notifier,
 	default:
 		FC_DBG("unknown event %ld call", event);
 	}
-	if (lp->link_status != new_status) {
+	if (atomic_read(&lp->link_status) != new_status) {
 		if ((new_status & FC_LINK_UP) == FC_LINK_UP)
 			fc_linkup(lp);
 		else {
diff --git a/drivers/scsi/libfc/fc_fcp.c b/drivers/scsi/libfc/fc_fcp.c
index 404e63f..fd2d642 100644
--- a/drivers/scsi/libfc/fc_fcp.c
+++ b/drivers/scsi/libfc/fc_fcp.c
@@ -1621,7 +1621,8 @@ out:
 static inline int fc_fcp_lport_queue_ready(struct fc_lport *lp)
 {
 	/* lock ? */
-	return (lp->state == LPORT_ST_READY) && (lp->link_status & FC_LINK_UP);
+	return (lp->state == LPORT_ST_READY)
+		&& (atomic_read(&lp->link_status) & FC_LINK_UP);
 }
 
 /**
@@ -1890,7 +1891,7 @@ int fc_eh_abort(struct scsi_cmnd *sc_cmd)
 	lp = shost_priv(sc_cmd->device->host);
 	if (lp->state != LPORT_ST_READY)
 		return rc;
-	else if (!(lp->link_status & FC_LINK_UP))
+	else if (!(atomic_read(&lp->link_status) & FC_LINK_UP))
 		return rc;
 
 	spin_lock_irqsave(lp->host->host_lock, flags);
diff --git a/drivers/scsi/libfc/fc_lport.c b/drivers/scsi/libfc/fc_lport.c
index 5db223c..51a5482 100644
--- a/drivers/scsi/libfc/fc_lport.c
+++ b/drivers/scsi/libfc/fc_lport.c
@@ -250,7 +250,7 @@ void fc_get_host_port_state(struct Scsi_Host *shost)
 {
 	struct fc_lport *lp = shost_priv(shost);
 
-	if ((lp->link_status & FC_LINK_UP) == FC_LINK_UP)
+	if ((atomic_read(&lp->link_status) & FC_LINK_UP) == FC_LINK_UP)
 		fc_host_port_state(shost) = FC_PORTSTATE_ONLINE;
 	else
 		fc_host_port_state(shost) = FC_PORTSTATE_OFFLINE;
@@ -573,17 +573,21 @@ EXPORT_SYMBOL(fc_fabric_login);
  */
 void fc_linkup(struct fc_lport *lport)
 {
+	int status;
+
 	FC_DEBUG_LPORT("Link is up for port (%6x)\n",
 		       fc_host_port_id(lport->host));
 
-	mutex_lock(&lport->lp_mutex);
-	if ((lport->link_status & FC_LINK_UP) != FC_LINK_UP) {
-		lport->link_status |= FC_LINK_UP;
+	status = atomic_read(&lport->link_status);
+	if ((status & FC_LINK_UP) != FC_LINK_UP) {
+		status |= FC_LINK_UP;
+		atomic_set(&lport->link_status, status);
 
+		mutex_lock(&lport->lp_mutex);
 		if (lport->state == LPORT_ST_RESET)
 			fc_lport_enter_flogi(lport);
+		mutex_unlock(&lport->lp_mutex);
 	}
-	mutex_unlock(&lport->lp_mutex);
 }
 EXPORT_SYMBOL(fc_linkup);
 
@@ -593,16 +597,20 @@ EXPORT_SYMBOL(fc_linkup);
  */
 void fc_linkdown(struct fc_lport *lport)
 {
-	mutex_lock(&lport->lp_mutex);
+	int status;
+
 	FC_DEBUG_LPORT("Link is down for port (%6x)\n",
 		       fc_host_port_id(lport->host));
 
-	if ((lport->link_status & FC_LINK_UP) == FC_LINK_UP) {
-		lport->link_status &= ~(FC_LINK_UP);
+	status = atomic_read(&lport->link_status);
+	if ((status & FC_LINK_UP) == FC_LINK_UP) {
+		status &= ~FC_LINK_UP;
+		atomic_set(&lport->link_status, status);
+		mutex_lock(&lport->lp_mutex);
 		fc_lport_enter_reset(lport);
 		lport->tt.fcp_cleanup(lport);
+		mutex_unlock(&lport->lp_mutex);
 	}
-	mutex_unlock(&lport->lp_mutex);
 }
 EXPORT_SYMBOL(fc_linkdown);
 
@@ -612,9 +620,9 @@ EXPORT_SYMBOL(fc_linkdown);
  */
 void fc_pause(struct fc_lport *lport)
 {
-	mutex_lock(&lport->lp_mutex);
-	lport->link_status |= FC_PAUSE;
-	mutex_unlock(&lport->lp_mutex);
+	int status;
+	status = atomic_read(&lport->link_status) | FC_PAUSE;
+	atomic_set(&lport->link_status, status);
 }
 EXPORT_SYMBOL(fc_pause);
 
@@ -624,9 +632,9 @@ EXPORT_SYMBOL(fc_pause);
  */
 void fc_unpause(struct fc_lport *lport)
 {
-	mutex_lock(&lport->lp_mutex);
-	lport->link_status &= ~(FC_PAUSE);
-	mutex_unlock(&lport->lp_mutex);
+	int status;
+	status = atomic_read(&lport->link_status) & ~FC_PAUSE;
+	atomic_set(&lport->link_status, status);
 }
 EXPORT_SYMBOL(fc_unpause);
 
@@ -977,7 +985,7 @@ static void fc_lport_enter_reset(struct fc_lport *lport)
 	fc_host_fabric_name(lport->host) = 0;
 	fc_host_port_id(lport->host) = 0;
 
-	if ((lport->link_status & FC_LINK_UP) == FC_LINK_UP)
+	if ((atomic_read(&lport->link_status) & FC_LINK_UP) == FC_LINK_UP)
 		fc_lport_enter_flogi(lport);
 }
 
diff --git a/include/scsi/libfc.h b/include/scsi/libfc.h
index 042f4ad..f7dca26 100644
--- a/include/scsi/libfc.h
+++ b/include/scsi/libfc.h
@@ -603,7 +603,7 @@ struct fc_lport {
 
 	/* Operational Information */
 	struct libfc_function_template tt;
-	u16			link_status;
+	atomic_t	link_status;
 	enum fc_lport_state	state;
 	unsigned long		boot_time;
 




More information about the devel mailing list