Skip to contentSkip to conversation

libfc, libfcoe, fcoe updates for 3.11(+)

PATCH In review Robert Love 2 participants 12 messages Jul 9, 2013

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover libfc, libfcoe, fcoe updates for 3.11(+)

The following series implements a few random fixes. I know this series may be a bit late for the merge window, but they're fairly simple fixes and I think they should be fine for RC if I've missed the merge window.

---

Bart Van Assche (1):
fcoe: Reduce number of sparse warnings

Mark Rustad (2):
libfc: Reject PLOGI from nodes with incompatible role
fcoe: Stop fc_rport_priv structure leak

Neerav Parikh (1):
fcoe: Fix smatch warning in fcoe_fdmi_info function

Robert Love (3):
libfc: Remove extra space in fc_exch_timer_cancel definition
libfc: Differentiate echange timer cancellation debug statements
libfcoe: Fix meaningless log statement

Yi Zou (1):
fcoe: fix the link error status block sparse warnings

 drivers/scsi/fcoe/fcoe.c           |   26 +++++++++++++++-----------
 drivers/scsi/fcoe/fcoe_ctlr.c      |    4 ++++
 drivers/scsi/fcoe/fcoe_sysfs.c     |   24 ++++++++++++------------
 drivers/scsi/fcoe/fcoe_transport.c |   28 ++++++----------------------
 drivers/scsi/libfc/fc_exch.c       |    4 ++--
 drivers/scsi/libfc/fc_rport.c      |   27 +++++++++++++++++++++++++++
 6 files changed, 66 insertions(+), 47 deletions(-)
Signature · 1 line
Thanks, //Rob
#1 lore raw

PATCH 1/8 libfc: Reject PLOGI from nodes with incompatible role

From: Mark Rustad <[email protected]>

Reject a PLOGI from a node with an incompatible role, that is, initiator-to-initiator or target-to-target.

---
 drivers/scsi/libfc/fc_rport.c |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
drivers/scsi/libfc/fc_rport.c +27 -0
index 6bbb944..c710d90 100644
@@ -926,6 +926,20 @@ err:
 	kref_put(&rdata->kref, rdata->local_port->tt.rport_destroy);
 }
 
+static bool
+fc_rport_compatible_roles(struct fc_lport *lport, struct fc_rport_priv *rdata)
+{
+	if (rdata->ids.roles == FC_PORT_ROLE_UNKNOWN)
+		return true;
+	if ((rdata->ids.roles & FC_PORT_ROLE_FCP_TARGET) &&
+	    (lport->service_params & FCP_SPPF_INIT_FCN))
+		return true;
+	if ((rdata->ids.roles & FC_PORT_ROLE_FCP_INITIATOR) &&
+	    (lport->service_params & FCP_SPPF_TARG_FCN))
+		return true;
+	return false;
+}
+
 /**
  * fc_rport_enter_plogi() - Send Port Login (PLOGI) request
  * @rdata: The remote port to send a PLOGI to
@@ -938,6 +952,12 @@ static void fc_rport_enter_plogi(struct fc_rport_priv *rdata)
 	struct fc_lport *lport = rdata->local_port;
 	struct fc_frame *fp;
 
+	if (!fc_rport_compatible_roles(lport, rdata)) {
+		FC_RPORT_DBG(rdata, "PLOGI suppressed for incompatible role\n");
+		fc_rport_state_enter(rdata, RPORT_ST_PLOGI_WAIT);
+		return;
+	}
+
 	FC_RPORT_DBG(rdata, "Port entered PLOGI state from %s state\n",
 		     fc_rport_state(rdata));
 
@@ -1646,6 +1666,13 @@ static void fc_rport_recv_plogi_req(struct fc_lport *lport,
 		rjt_data.explan = ELS_EXPL_NONE;
 		goto reject;
 	}
+	if (!fc_rport_compatible_roles(lport, rdata)) {
+		FC_RPORT_DBG(rdata, "Received PLOGI for incompatible role\n");
+		mutex_unlock(&rdata->rp_mutex);
+		rjt_data.reason = ELS_RJT_LOGIC;
+		rjt_data.explan = ELS_EXPL_NONE;
+		goto reject;
+	}
 
 	/*
 	 * Get session payload size from incoming PLOGI.
#2 lore raw

PATCH 2/8 fcoe: Fix smatch warning in fcoe_fdmi_info function

In reply to Robert Love

From: Neerav Parikh <[email protected]>

This patch fixes a smatch warning as below:

smatch warnings:
drivers/scsi/fcoe/fcoe.c:782 fcoe_fdmi_info() warn: 'fdmi' puts 896 bytes on stack

---
 drivers/scsi/fcoe/fcoe.c |   26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)
drivers/scsi/fcoe/fcoe.c +15 -11
index 32ae6c6..3336e57 100644
@@ -774,7 +774,6 @@ static void fcoe_fdmi_info(struct fc_lport *lport, struct net_device *netdev)
 	struct fcoe_port *port;
 	struct net_device *realdev;
 	int rc;
-	struct netdev_fcoe_hbainfo fdmi;
 
 	port = lport_priv(lport);
 	fcoe = port->priv;
@@ -788,9 +787,13 @@ static void fcoe_fdmi_info(struct fc_lport *lport, struct net_device *netdev)
 		return;
 
 	if (realdev->netdev_ops->ndo_fcoe_get_hbainfo) {
-		memset(&fdmi, 0, sizeof(fdmi));
+		struct netdev_fcoe_hbainfo *fdmi;
+		fdmi = kzalloc(sizeof(*fdmi), GFP_KERNEL);
+		if (!fdmi)
+			return;
+
 		rc = realdev->netdev_ops->ndo_fcoe_get_hbainfo(realdev,
-							       &fdmi);
+							       fdmi);
 		if (rc) {
 			printk(KERN_INFO "fcoe: Failed to retrieve FDMI "
 					"information from netdev.\n");
@@ -800,38 +803,39 @@ static void fcoe_fdmi_info(struct fc_lport *lport, struct net_device *netdev)
 		snprintf(fc_host_serial_number(lport->host),
 			 FC_SERIAL_NUMBER_SIZE,
 			 "%s",
-			 fdmi.serial_number);
+			 fdmi->serial_number);
 		snprintf(fc_host_manufacturer(lport->host),
 			 FC_SERIAL_NUMBER_SIZE,
 			 "%s",
-			 fdmi.manufacturer);
+			 fdmi->manufacturer);
 		snprintf(fc_host_model(lport->host),
 			 FC_SYMBOLIC_NAME_SIZE,
 			 "%s",
-			 fdmi.model);
+			 fdmi->model);
 		snprintf(fc_host_model_description(lport->host),
 			 FC_SYMBOLIC_NAME_SIZE,
 			 "%s",
-			 fdmi.model_description);
+			 fdmi->model_description);
 		snprintf(fc_host_hardware_version(lport->host),
 			 FC_VERSION_STRING_SIZE,
 			 "%s",
-			 fdmi.hardware_version);
+			 fdmi->hardware_version);
 		snprintf(fc_host_driver_version(lport->host),
 			 FC_VERSION_STRING_SIZE,
 			 "%s",
-			 fdmi.driver_version);
+			 fdmi->driver_version);
 		snprintf(fc_host_optionrom_version(lport->host),
 			 FC_VERSION_STRING_SIZE,
 			 "%s",
-			 fdmi.optionrom_version);
+			 fdmi->optionrom_version);
 		snprintf(fc_host_firmware_version(lport->host),
 			 FC_VERSION_STRING_SIZE,
 			 "%s",
-			 fdmi.firmware_version);
+			 fdmi->firmware_version);
 
 		/* Enable FDMI lport states */
 		lport->fdmi_enabled = 1;
+		kfree(fdmi);
 	} else {
 		lport->fdmi_enabled = 0;
 		printk(KERN_INFO "fcoe: No FDMI support.\n");
#3 lore raw

PATCH 3/8 fcoe: fix the link error status block sparse warnings

In reply to Robert Love

From: Yi Zou <[email protected]>

Both fcoe_fc_els_lesb and fc_els_lesb are in __be32 already, and both are exactly the same size in bytes, with somewhat different member names to reflect the fact the former is for Ethernet media the latter is for Fiber Channel, so, remove conversion and use __be32 directly. This fixes the warning from sparse check.

---
 drivers/scsi/fcoe/fcoe_transport.c |   22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)
drivers/scsi/fcoe/fcoe_transport.c +4 -18
index f3a5a53..bedd422 100644
@@ -180,24 +180,10 @@ void fcoe_ctlr_get_lesb(struct fcoe_ctlr_device *ctlr_dev)
 {
 	struct fcoe_ctlr *fip = fcoe_ctlr_device_priv(ctlr_dev);
 	struct net_device *netdev = fcoe_get_netdev(fip->lp);
-	struct fcoe_fc_els_lesb *fcoe_lesb;
-	struct fc_els_lesb fc_lesb;
-
-	__fcoe_get_lesb(fip->lp, &fc_lesb, netdev);
-	fcoe_lesb = (struct fcoe_fc_els_lesb *)(&fc_lesb);
-
-	ctlr_dev->lesb.lesb_link_fail =
-		ntohl(fcoe_lesb->lesb_link_fail);
-	ctlr_dev->lesb.lesb_vlink_fail =
-		ntohl(fcoe_lesb->lesb_vlink_fail);
-	ctlr_dev->lesb.lesb_miss_fka =
-		ntohl(fcoe_lesb->lesb_miss_fka);
-	ctlr_dev->lesb.lesb_symb_err =
-		ntohl(fcoe_lesb->lesb_symb_err);
-	ctlr_dev->lesb.lesb_err_block =
-		ntohl(fcoe_lesb->lesb_err_block);
-	ctlr_dev->lesb.lesb_fcs_error =
-		ntohl(fcoe_lesb->lesb_fcs_error);
+	struct fc_els_lesb *fc_lesb;
+
+	fc_lesb = (struct fc_els_lesb *)(&ctlr_dev->lesb);
+	__fcoe_get_lesb(fip->lp, fc_lesb, netdev);
 }
 EXPORT_SYMBOL_GPL(fcoe_ctlr_get_lesb);
 
#4 lore raw

PATCH 4/8 libfc: Remove extra space in fc_exch_timer_cancel definition

In reply to Robert Love

Simply remove an extra space that violates coding style.

---
 drivers/scsi/libfc/fc_exch.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/libfc/fc_exch.c +1 -1
index 8b928c6..e98ea6a 100644
@@ -337,7 +337,7 @@ static void fc_exch_release(struct fc_exch *ep)
  * fc_exch_timer_cancel() - cancel exch timer
  * @ep:		The exchange whose timer to be canceled
  */
-static inline  void fc_exch_timer_cancel(struct fc_exch *ep)
+static inline void fc_exch_timer_cancel(struct fc_exch *ep)
 {
 	if (cancel_delayed_work(&ep->timeout_work)) {
 		FC_EXCH_DBG(ep, "Exchange timer canceled\n");
Acked-by on v1 4/8
#5 lore raw

Re: [PATCH 4/8] libfc: Remove extra space in fc_exch_timer_cancel definition

Quoted 16 lines · Robert Love

Simply remove an extra space that violates coding style.

---
 drivers/scsi/libfc/fc_exch.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/libfc/fc_exch.c +0 -0
index 8b928c6..e98ea6a 100644

drivers/scsi/libfc/fc_exch.c static void fc_exch_release(struct fc_exch *ep) · line 343

Quoted 8 lines
drivers/scsi/libfc/fc_exch.c +1 -1
@@ -337,7 +337,7 @@ static void fc_exch_release(struct fc_exch *ep)
  * fc_exch_timer_cancel() - cancel exch timer
  * @ep:		The exchange whose timer to be canceled
  */
-static inline  void fc_exch_timer_cancel(struct fc_exch *ep)
+static inline void fc_exch_timer_cancel(struct fc_exch *ep)
 {
 	if (cancel_delayed_work(&ep->timeout_work)) {
 		FC_EXCH_DBG(ep, "Exchange timer canceled\n");
#6 lore raw

PATCH 5/8 libfc: Differentiate echange timer cancellation debug statements

In reply to Robert Love

There are two debug statements with the same output string regarding echange timer cancellation. This patch simply changes the output of one string so that they can be differentiated.

---
 drivers/scsi/libfc/fc_exch.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/libfc/fc_exch.c +1 -1
index e98ea6a..5879929 100644
@@ -1567,7 +1567,7 @@ static void fc_exch_abts_resp(struct fc_exch *ep, struct fc_frame *fp)
 		    fc_exch_rctl_name(fh->fh_r_ctl));
 
 	if (cancel_delayed_work_sync(&ep->timeout_work)) {
-		FC_EXCH_DBG(ep, "Exchange timer canceled\n");
+		FC_EXCH_DBG(ep, "Exchange timer canceled due to ABTS response\n");
 		fc_exch_release(ep);	/* release from pending timer hold */
 	}
 
Acked-by on v1 5/8
#7 lore raw

Re: [PATCH 5/8] libfc: Differentiate echange timer cancellation debug statements

Quoted 18 lines · Robert Love

There are two debug statements with the same output string regarding echange timer cancellation. This patch simply changes the output of one string so that they can be differentiated.

---
 drivers/scsi/libfc/fc_exch.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/libfc/fc_exch.c +0 -0
index e98ea6a..5879929 100644

drivers/scsi/libfc/fc_exch.c static void fc_exch_abts_resp(struct fc_exch *ep, struct fc_frame *fp) · line 1573

Quoted 8 lines
drivers/scsi/libfc/fc_exch.c +1 -1
@@ -1567,7 +1567,7 @@ static void fc_exch_abts_resp(struct fc_exch *ep, struct fc_frame *fp)
 		    fc_exch_rctl_name(fh->fh_r_ctl));
 
 	if (cancel_delayed_work_sync(&ep->timeout_work)) {
-		FC_EXCH_DBG(ep, "Exchange timer canceled\n");
+		FC_EXCH_DBG(ep, "Exchange timer canceled due to ABTS response\n");
 		fc_exch_release(ep);	/* release from pending timer hold */
 	}
 
#8 lore raw

PATCH 6/8 libfcoe: Fix meaningless log statement

In reply to Robert Love

ctlr_dev was initialized to NULL, and never re-assigned. This caused the log statement to always report failure. This patch removes the unused variable and fixes the log statement to always report 'success', as that is what should be logged if the code reaches this point.

---
 drivers/scsi/fcoe/fcoe_transport.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/scsi/fcoe/fcoe_transport.c +2 -4
index bedd422..f1ae5ed 100644
@@ -707,7 +707,6 @@ ssize_t fcoe_ctlr_create_store(struct bus_type *bus,
 {
 	struct net_device *netdev = NULL;
 	struct fcoe_transport *ft = NULL;
-	struct fcoe_ctlr_device *ctlr_dev = NULL;
 	int rc = 0;
 	int err;
 
@@ -754,9 +753,8 @@ ssize_t fcoe_ctlr_create_store(struct bus_type *bus,
 		goto out_putdev;
 	}
 
-	LIBFCOE_TRANSPORT_DBG("transport %s %s to create fcoe on %s.\n",
-			      ft->name, (ctlr_dev) ? "succeeded" : "failed",
-			      netdev->name);
+	LIBFCOE_TRANSPORT_DBG("transport %s succeeded to create fcoe on %s.\n",
+			      ft->name, netdev->name);
 
 out_putdev:
 	dev_put(netdev);
Acked-by on v1 6/8
#9 lore raw

Re: [PATCH 6/8] libfcoe: Fix meaningless log statement

Quoted 29 lines · Robert Love

ctlr_dev was initialized to NULL, and never re-assigned. This caused the log statement to always report failure. This patch removes the unused variable and fixes the log statement to always report 'success', as that is what should be logged if the code reaches this point.

---
 drivers/scsi/fcoe/fcoe_transport.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/scsi/fcoe/fcoe_transport.c +0 -1
index bedd422..f1ae5ed 100644
@@ -707,7 +707,6 @@ ssize_t fcoe_ctlr_create_store(struct bus_type *bus,
 {
 	struct net_device *netdev = NULL;
 	struct fcoe_transport *ft = NULL;
-	struct fcoe_ctlr_device *ctlr_dev = NULL;
 	int rc = 0;
 	int err;
 
@@ -754,9 +753,8 @@ ssize_t fcoe_ctlr_create_store(struct bus_type *bus,
 		goto out_putdev;

drivers/scsi/fcoe/fcoe_transport.c ssize_t fcoe_ctlr_create_store(struct bus_type *bus, · line 760

Quoted 10 lines
drivers/scsi/fcoe/fcoe_transport.c +2 -3
 	}
 
-	LIBFCOE_TRANSPORT_DBG("transport %s %s to create fcoe on %s.\n",
-			      ft->name, (ctlr_dev) ? "succeeded" : "failed",
-			      netdev->name);
+	LIBFCOE_TRANSPORT_DBG("transport %s succeeded to create fcoe on %s.\n",
+			      ft->name, netdev->name);
 
 out_putdev:
 	dev_put(netdev);
#10 lore raw

PATCH 7/8 fcoe: Stop fc_rport_priv structure leak

In reply to Robert Love

From: Mark Rustad <[email protected]>

When repeatedly doing rmmod and modprobe on the ixgbe driver while FCoE is active in a VN2VN configuration, memory leaks would be discovered by kmemleak with the following backtrace:

unreferenced object 0xffff88003d076000 (size 1024):
comm "kworker/0:3", pid 2998, jiffies 4295436448 (age 1015.332s) hex dump (first 32 bytes):

    48 8a fe 6f 00 88 ff ff 00 00 00 00 00 00 00 00  H..o............
    01 00 00 00 02 00 00 00 7b ac 87 21 1b 00 00 10  ........{..!....
  backtrace:
    [<ffffffff814b308b>] kmemleak_alloc+0x5b/0xc0
    [<ffffffff8115c6e8>] __kmalloc+0xd8/0x1b0
    [<ffffffffa0216638>] fc_rport_create+0x48/0x1f0 [libfc]
    [<ffffffffa023cd86>] fcoe_ctlr_vn_add.isra.10+0x56/0x1a0 [libfcoe]
    [<ffffffffa023f440>] fcoe_ctlr_vn_recv+0x8b0/0xab0 [libfcoe]
    [<ffffffffa023fb06>] fcoe_ctlr_recv_work+0x4c6/0xf60 [libfcoe]
    [<ffffffff81067404>] process_one_work+0x1e4/0x4d0
    [<ffffffff81068def>] worker_thread+0x10f/0x380
    [<ffffffff8107019a>] kthread+0xea/0xf0
    [<ffffffff814d32ec>] ret_from_fork+0x7c/0xb0
    [<ffffffffffffffff>] 0xffffffffffffffff

This patch stops the leak of the fc_rport_priv structure.

---
 drivers/scsi/fcoe/fcoe_ctlr.c |    4 ++++
 1 file changed, 4 insertions(+)
drivers/scsi/fcoe/fcoe_ctlr.c +4 -0
index 795843d..203415e 100644
@@ -2090,7 +2090,11 @@ static struct fc_rport_operations fcoe_ctlr_vn_rport_ops = {
  */
 static void fcoe_ctlr_disc_stop_locked(struct fc_lport *lport)
 {
+	struct fc_rport_priv *rdata;
+
 	mutex_lock(&lport->disc.disc_mutex);
+	list_for_each_entry_rcu(rdata, &lport->disc.rports, peers)
+		lport->tt.rport_logoff(rdata);
 	lport->disc.disc_callback = NULL;
 	mutex_unlock(&lport->disc.disc_mutex);
 }
#11 lore raw

PATCH 8/8 fcoe: Reduce number of sparse warnings

In reply to Robert Love

From: Bart Van Assche <[email protected]>

Declare local variables and functions 'static'. This patch does not change any functionality.

---
 drivers/scsi/fcoe/fcoe_sysfs.c |   24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)
drivers/scsi/fcoe/fcoe_sysfs.c +12 -12
index 8c05ae01..c9382d6 100644
@@ -507,7 +507,7 @@ static const struct attribute_group *fcoe_fcf_attr_groups[] = {
 	NULL,
 };
 
-struct bus_type fcoe_bus_type;
+static struct bus_type fcoe_bus_type;
 
 static int fcoe_bus_match(struct device *dev,
 			  struct device_driver *drv)
@@ -541,25 +541,25 @@ static void fcoe_fcf_device_release(struct device *dev)
 	kfree(fcf);
 }
 
-struct device_type fcoe_ctlr_device_type = {
+static struct device_type fcoe_ctlr_device_type = {
 	.name = "fcoe_ctlr",
 	.groups = fcoe_ctlr_attr_groups,
 	.release = fcoe_ctlr_device_release,
 };
 
-struct device_type fcoe_fcf_device_type = {
+static struct device_type fcoe_fcf_device_type = {
 	.name = "fcoe_fcf",
 	.groups = fcoe_fcf_attr_groups,
 	.release = fcoe_fcf_device_release,
 };
 
-struct bus_attribute fcoe_bus_attr_group[] = {
+static struct bus_attribute fcoe_bus_attr_group[] = {
 	__ATTR(ctlr_create, S_IWUSR, NULL, fcoe_ctlr_create_store),
 	__ATTR(ctlr_destroy, S_IWUSR, NULL, fcoe_ctlr_destroy_store),
 	__ATTR_NULL
 };
 
-struct bus_type fcoe_bus_type = {
+static struct bus_type fcoe_bus_type = {
 	.name = "fcoe",
 	.match = &fcoe_bus_match,
 	.bus_attrs = fcoe_bus_attr_group,
@@ -569,7 +569,7 @@ struct bus_type fcoe_bus_type = {
  * fcoe_ctlr_device_flush_work() - Flush a FIP ctlr's workqueue
  * @ctlr: Pointer to the FIP ctlr whose workqueue is to be flushed
  */
-void fcoe_ctlr_device_flush_work(struct fcoe_ctlr_device *ctlr)
+static void fcoe_ctlr_device_flush_work(struct fcoe_ctlr_device *ctlr)
 {
 	if (!fcoe_ctlr_work_q(ctlr)) {
 		printk(KERN_ERR
@@ -590,8 +590,8 @@ void fcoe_ctlr_device_flush_work(struct fcoe_ctlr_device *ctlr)
  * Return value:
  *	1 on success / 0 already queued / < 0 for error
  */
-int fcoe_ctlr_device_queue_work(struct fcoe_ctlr_device *ctlr,
-			       struct work_struct *work)
+static int fcoe_ctlr_device_queue_work(struct fcoe_ctlr_device *ctlr,
+				       struct work_struct *work)
 {
 	if (unlikely(!fcoe_ctlr_work_q(ctlr))) {
 		printk(KERN_ERR
@@ -609,7 +609,7 @@ int fcoe_ctlr_device_queue_work(struct fcoe_ctlr_device *ctlr,
  * fcoe_ctlr_device_flush_devloss() - Flush a FIP ctlr's devloss workqueue
  * @ctlr: Pointer to FIP ctlr whose workqueue is to be flushed
  */
-void fcoe_ctlr_device_flush_devloss(struct fcoe_ctlr_device *ctlr)
+static void fcoe_ctlr_device_flush_devloss(struct fcoe_ctlr_device *ctlr)
 {
 	if (!fcoe_ctlr_devloss_work_q(ctlr)) {
 		printk(KERN_ERR
@@ -631,9 +631,9 @@ void fcoe_ctlr_device_flush_devloss(struct fcoe_ctlr_device *ctlr)
  * Return value:
  *	1 on success / 0 already queued / < 0 for error
  */
-int fcoe_ctlr_device_queue_devloss_work(struct fcoe_ctlr_device *ctlr,
-				       struct delayed_work *work,
-				       unsigned long delay)
+static int fcoe_ctlr_device_queue_devloss_work(struct fcoe_ctlr_device *ctlr,
+					       struct delayed_work *work,
+					       unsigned long delay)
 {
 	if (unlikely(!fcoe_ctlr_devloss_work_q(ctlr))) {
 		printk(KERN_ERR