[PATCH 0/6] libfc, libfcoe and fcoe patches for scsi-misc

[PATCH 0/6] libfc, libfcoe and fcoe patches for scsi-misc

[Robert Love]

Here are a variety of fixes for the libfc, libfcoe and fcoe kernel
modules. I've included a pull request that points to a signed-tag.
I don't have many signatures on my pgp key as I haven't been to a
key signing party. It's up to you, James, as to whether you want
to use/trust it or not.

git://open-fcoe.org/fcoe/linux-fcoe.git tags/for-james-1

Thanks, //Rob

---

Dan Carpenter (1):
      fcoe: remove a stray unlock

Robert Love (1):
      fcoe: Don't hold rtnl_mutex in fcoe_update_src_mac

Vasu Dev (3):
      fcoe: remove lport from net device before doing per cpu rx thread cleanup
      libfc: flush lport worker after its disabled
      libfc: defer releasing master lport until complete fcoe interface cleanuped up

Yi Zou (1):
      libfcoe: fix VN2VN N_Port_ID Beacon source MAC


 drivers/scsi/fcoe/fcoe.c      |   41 ++++++++++++++++++++++++++++++-----------
 drivers/scsi/fcoe/fcoe.h      |    4 +++-
 drivers/scsi/fcoe/fcoe_ctlr.c |    8 +++++++-
 drivers/scsi/libfc/fc_lport.c |    2 +-
 4 files changed, 41 insertions(+), 14 deletions(-)

-- 

[PATCH 1/6] fcoe: remove lport from net device before doing per cpu rx thread cleanup

[Robert Love]

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

Remove lport from net device and then do synchronize net device to flush
inflight rx frames for the lport before doing fcoe_percpu_clean.

In case of master lport, remove all rx packet handlers completely and
then only do fcoe_percpu_clean. This required splitting fcoe_interface_cleanup
to do remove part separately and for that added func fcoe_interface_remove
and then  call it from fcoe_if_destory before doing fcoe_percpu_clean.
However if fcoe_interface_remove() is already called then
don't call again from fcoe_interface_cleanup() to preserve its
existing flows.

This patch along with Neil's other patch to avoid soft irq context
on ingress will avoid passing up frames on disabled lport as
discussed in this mail thread:-
http://lists.open-fcoe.org/pipermail/devel/2012-February/011947.html

Signed-off-by: Vasu Dev <vasu.dev@intel.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
Tested-by: Ross Brattain <ross.b.brattain@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/fcoe/fcoe.c |   25 +++++++++++++++++++++----
 drivers/scsi/fcoe/fcoe.h |    4 +++-
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 335e851..09a6a26 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -411,20 +411,18 @@ out:
 }
 
 /**
- * fcoe_interface_cleanup() - Clean up a FCoE interface
+ * fcoe_interface_remove() - remove FCoE interface from netdev
  * @fcoe: The FCoE interface to be cleaned up
  *
  * Caller must be holding the RTNL mutex
  */
-static void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
+static void fcoe_interface_remove(struct fcoe_interface *fcoe)
 {
 	struct net_device *netdev = fcoe->netdev;
 	struct fcoe_ctlr *fip = &fcoe->ctlr;
 	u8 flogi_maddr[ETH_ALEN];
 	const struct net_device_ops *ops;
 
-	rtnl_lock();
-
 	/*
 	 * Don't listen for Ethernet packets anymore.
 	 * synchronize_net() ensures that the packet handlers are not running
@@ -453,7 +451,22 @@ static void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
 			FCOE_NETDEV_DBG(netdev, "Failed to disable FCoE"
 					" specific feature for LLD.\n");
 	}
+	fcoe->removed = 1;
+}
+
+
+/**
+ * fcoe_interface_cleanup() - Clean up a FCoE interface
+ * @fcoe: The FCoE interface to be cleaned up
+ */
+static void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
+{
+	struct net_device *netdev = fcoe->netdev;
+	struct fcoe_ctlr *fip = &fcoe->ctlr;
 
+	rtnl_lock();
+	if (!fcoe->removed)
+		fcoe_interface_remove(fcoe);
 	rtnl_unlock();
 
 	/* Release the self-reference taken during fcoe_interface_create() */
@@ -941,6 +954,10 @@ static void fcoe_if_destroy(struct fc_lport *lport)
 	rtnl_lock();
 	if (!is_zero_ether_addr(port->data_src_addr))
 		dev_uc_del(netdev, port->data_src_addr);
+	if (lport->vport)
+		synchronize_net();
+	else
+		fcoe_interface_remove(fcoe);
 	rtnl_unlock();
 
 	/* Free queued packets for the per-CPU receive threads */
diff --git a/drivers/scsi/fcoe/fcoe.h b/drivers/scsi/fcoe/fcoe.h
index 3c2733a..96ac938 100644
--- a/drivers/scsi/fcoe/fcoe.h
+++ b/drivers/scsi/fcoe/fcoe.h
@@ -71,7 +71,8 @@ do {                                                            	\
  * @ctlr:	      The FCoE controller (for FIP)
  * @oem:	      The offload exchange manager for all local port
  *		      instances associated with this port
- * This structure is 1:1 with a net devive.
+ * @removed:	      Indicates fcoe interface removed from net device
+ * This structure is 1:1 with a net device.
  */
 struct fcoe_interface {
 	struct list_head   list;
@@ -81,6 +82,7 @@ struct fcoe_interface {
 	struct packet_type fip_packet_type;
 	struct fcoe_ctlr   ctlr;
 	struct fc_exch_mgr *oem;
+	u8	removed;
 };
 
 #define fcoe_from_ctlr(fip) container_of(fip, struct fcoe_interface, ctlr)

[PATCH 2/6] libfc: flush lport worker after its disabled

[Robert Love]

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

The lport could get timeout armed while its getting disabled,
so flush lport worker after its disabled and ignore lport
retry in that case instead of WARN_ON.

 [13192.936858] WARNING: at drivers/scsi/libfc/fc_lport.c:1573 fc_lport_timeout+0x53/0xa9 [libfc]()
 [13192.938026] Hardware name: Bochs
 [13192.938620] Modules linked in: fcoe libfcoe libfc scsi_transport_fc scsi_tgt fuse 8021q garp stp llc sunrpc ipv6 uinput microcode joydev pcspkr ixgbe e1000 i2c_piix4 i2c_core virtio_balloon dca mdio virtio_blk virtio_pci virtio_ring virtio floppy [last unloaded: speedstep_lib]
 [13192.942589] Pid: 23605, comm: kworker/0:6 Tainted: G        W    3.2.0+ #71
 [13192.943587] Call Trace:
 [13192.944052]  [<ffffffff810403f4>] warn_slowpath_common+0x85/0x9d
 [13192.944940]  [<ffffffff81040426>] warn_slowpath_null+0x1a/0x1c
 [13192.945734]  [<ffffffffa02746eb>] fc_lport_timeout+0x53/0xa9 [libfc]
 [13192.946665]  [<ffffffff81058d88>] process_one_work+0x20c/0x3ad
 [13192.947541]  [<ffffffff81058cbe>] ? process_one_work+0x142/0x3ad
 [13192.948423]  [<ffffffffa0274698>] ? fc_lport_enter_ns+0x178/0x178 [libfc]
 [13192.949363]  [<ffffffff8105a313>] worker_thread+0xfd/0x181
 [13192.950191]  [<ffffffff8105a216>] ? manage_workers.clone.15+0x173/0x173
 [13192.951100]  [<ffffffff8105e19b>] kthread+0xa4/0xac
 [13192.951755]  [<ffffffff814edbb4>] kernel_thread_helper+0x4/0x10
 [13192.952520]  [<ffffffff814e5cb4>] ? retint_restore_args+0x13/0x13
 [13192.953398]  [<ffffffff8105e0f7>] ? __init_kthread_worker+0x5b/0x5b
 [13192.954278]  [<ffffffff814edbb0>] ? gs_change+0x13/0x13
 [13192.954911] ---[ end trace 9763213b95bbd803 ]---

Signed-off-by: Vasu Dev <vasu.dev@intel.com>
Acked-by: Neil Horman <nhorman@tuxdriver.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, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/libfc/fc_lport.c b/drivers/scsi/libfc/fc_lport.c
index cc83b66..c1402fb 100644
--- a/drivers/scsi/libfc/fc_lport.c
+++ b/drivers/scsi/libfc/fc_lport.c
@@ -648,6 +648,7 @@ int fc_lport_destroy(struct fc_lport *lport)
 	lport->tt.fcp_abort_io(lport);
 	lport->tt.disc_stop_final(lport);
 	lport->tt.exch_mgr_reset(lport, 0, 0);
+	cancel_delayed_work_sync(&lport->retry_work);
 	fc_fc4_del_lport(lport);
 	return 0;
 }
@@ -1564,7 +1565,6 @@ static void fc_lport_timeout(struct work_struct *work)
 
 	switch (lport->state) {
 	case LPORT_ST_DISABLED:
-		WARN_ON(1);
 		break;
 	case LPORT_ST_READY:
 		break;

[PATCH 3/6] libfc: defer releasing master lport until complete fcoe interface cleanuped up

[Robert Love]

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

The fcoe controller has back references, therefore defer
releasing master lport which gets freed along scsi_host_put
and then free it once fcoe interface is fully cleaned.

Signed-off-by: Vasu Dev <vasu.dev@intel.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
Tested-by: Ross Brattain <ross.b.brattain@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/fcoe/fcoe.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 09a6a26..481ba6f 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -472,6 +472,7 @@ static void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
 	/* Release the self-reference taken during fcoe_interface_create() */
 	/* tear-down the FCoE controller */
 	fcoe_ctlr_destroy(fip);
+	scsi_host_put(fcoe->ctlr.lp->host);
 	kfree(fcoe);
 	dev_put(netdev);
 	module_put(THIS_MODULE);
@@ -976,8 +977,12 @@ static void fcoe_if_destroy(struct fc_lport *lport)
 	/* Free memory used by statistical counters */
 	fc_lport_free_stats(lport);
 
-	/* Release the Scsi_Host */
-	scsi_host_put(lport->host);
+	/*
+	 * Release the Scsi_Host for vport but hold on to
+	 * master lport until it fcoe interface fully cleaned-up.
+	 */
+	if (lport->vport)
+		scsi_host_put(lport->host);
 }
 
 /**

[PATCH 4/6] fcoe: Don't hold rtnl_mutex in fcoe_update_src_mac

[Robert Love]

The rtnl_mutex was held to protect calls to dev_uc_add
and dev_uc_del. Holding rtnl is not required as those
functions make use of the netif_addr_lock* API to
protect the MAC changing.

This change fixes the following regression by removing
the rtnl usage when fcoe_update_src_mac is called.

https://bugzilla.kernel.org/show_bug.cgi?id=42918

the existing dependency chain (in reverse order) is:

-> #1 (&fip->ctlr_mutex){+.+...}:
       [<c1091f70>] lock_acquire+0x80/0x1b0
       [<c147655d>] mutex_lock_nested+0x6d/0x340
       [<f8970c32>] fcoe_ctlr_link_up+0x22/0x180 [libfcoe]
       [<f894620e>] fcoe_create+0x47e/0x6e0 [fcoe]
       [<f8973dd3>] fcoe_transport_create+0x143/0x250 [libfcoe]
       [<c10527e0>] param_attr_store+0x30/0x60
       [<c1052696>] module_attr_store+0x26/0x40
       [<c11a201e>] sysfs_write_file+0xae/0x100
       [<c11449df>] vfs_write+0x8f/0x160
       [<c1144cbd>] sys_write+0x3d/0x70
       [<c147a0c4>] syscall_call+0x7/0xb

-> #0 (rtnl_mutex){+.+.+.}:
       [<c109164b>] __lock_acquire+0x140b/0x1720
       [<c1091f70>] lock_acquire+0x80/0x1b0
       [<c147655d>] mutex_lock_nested+0x6d/0x340
       [<c13a10c4>] rtnl_lock+0x14/0x20
       [<f89445ac>] fcoe_update_src_mac+0x2c/0xb0 [fcoe]
       [<f8971712>] fcoe_ctlr_timer_work+0x712/0xb60 [libfcoe]
       [<c104fb69>] process_one_work+0x179/0x5d0
       [<c10502f1>] worker_thread+0x121/0x2d0
       [<c10550ed>] kthread+0x7d/0x90
       [<c1481a82>] kernel_thread_helper+0x6/0x10

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&fip->ctlr_mutex);
                               lock(rtnl_mutex);
                               lock(&fip->ctlr_mutex);
  lock(rtnl_mutex);

 *** DEADLOCK ***

Signed-off-by: Robert Love <robert.w.love@intel.com>
Nacked-by: Robert Love <robert.w.love@intel.com>
Tested-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/fcoe/fcoe.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 481ba6f..ac1df93 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -536,13 +536,11 @@ static void fcoe_update_src_mac(struct fc_lport *lport, u8 *addr)
 	struct fcoe_port *port = lport_priv(lport);
 	struct fcoe_interface *fcoe = port->priv;
 
-	rtnl_lock();
 	if (!is_zero_ether_addr(port->data_src_addr))
 		dev_uc_del(fcoe->netdev, port->data_src_addr);
 	if (!is_zero_ether_addr(addr))
 		dev_uc_add(fcoe->netdev, addr);
 	memcpy(port->data_src_addr, addr, ETH_ALEN);
-	rtnl_unlock();
 }
 
 /**

[PATCH 5/6] libfcoe: fix VN2VN N_Port_ID Beacon source MAC

[Robert Love]

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

FC-BB-6 v1.04 7.9.8.14 N_Port_ID Beacon:

"A N_Port_ID Beacon is multicast and uses the VN_Port MAC address as source
address."

Currently, libfcoe is using ENode MAC, this seems ok and functionality wise
not a problem in my back to back testing setup, however, just fix this to
make libfcoe VN2VN support more spec compliant.

Signed-off-by: Yi Zou <yi.zou@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/fcoe/fcoe_ctlr.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe_ctlr.c b/drivers/scsi/fcoe/fcoe_ctlr.c
index 249a106..5a4c725 100644
--- a/drivers/scsi/fcoe/fcoe_ctlr.c
+++ b/drivers/scsi/fcoe/fcoe_ctlr.c
@@ -1883,7 +1883,13 @@ static void fcoe_ctlr_vn_send(struct fcoe_ctlr *fip,
 	frame = (struct fip_frame *)skb->data;
 	memset(frame, 0, len);
 	memcpy(frame->eth.h_dest, dest, ETH_ALEN);
-	memcpy(frame->eth.h_source, fip->ctl_src_addr, ETH_ALEN);
+
+	if (sub == FIP_SC_VN_BEACON) {
+		hton24(frame->eth.h_source, FIP_VN_FC_MAP);
+		hton24(frame->eth.h_source + 3, fip->port_id);
+	} else {
+		memcpy(frame->eth.h_source, fip->ctl_src_addr, ETH_ALEN);
+	}
 	frame->eth.h_proto = htons(ETH_P_FIP);
 
 	frame->fip.fip_ver = FIP_VER_ENCAPS(FIP_VER);

[PATCH 6/6] fcoe: remove a stray unlock

[Robert Love]

From: Dan Carpenter <dan.carpenter@oracle.com>

We moved the locking in dd060e74fb "[SCSI] fcoe: remove frame dropping
code from fcoe_percpu_clean" but this unlock was missed.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/fcoe/fcoe.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index ac1df93..76e3d0b 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -2294,10 +2294,9 @@ static void fcoe_percpu_clean(struct fc_lport *lport)
 			continue;
 
 		skb = dev_alloc_skb(0);
-		if (!skb) {
-			spin_unlock_bh(&pp->fcoe_rx_list.lock);
+		if (!skb)
 			continue;
-		}
+
 		skb->destructor = fcoe_percpu_flush_done;
 
 		spin_lock_bh(&pp->fcoe_rx_list.lock);

Re: [PATCH 4/6] fcoe: Don't hold rtnl_mutex in fcoe_update_src_mac

[James Bottomley]

On Fri, 2012-04-20 at 12:16 -0700, Robert Love wrote:
> The rtnl_mutex was held to protect calls to dev_uc_add
> and dev_uc_del. Holding rtnl is not required as those
> functions make use of the netif_addr_lock* API to
> protect the MAC changing.
> 
> This change fixes the following regression by removing
> the rtnl usage when fcoe_update_src_mac is called.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=42918
> 
> the existing dependency chain (in reverse order) is:
> 
> -> #1 (&fip->ctlr_mutex){+.+...}:
>        [<c1091f70>] lock_acquire+0x80/0x1b0
>        [<c147655d>] mutex_lock_nested+0x6d/0x340
>        [<f8970c32>] fcoe_ctlr_link_up+0x22/0x180 [libfcoe]
>        [<f894620e>] fcoe_create+0x47e/0x6e0 [fcoe]
>        [<f8973dd3>] fcoe_transport_create+0x143/0x250 [libfcoe]
>        [<c10527e0>] param_attr_store+0x30/0x60
>        [<c1052696>] module_attr_store+0x26/0x40
>        [<c11a201e>] sysfs_write_file+0xae/0x100
>        [<c11449df>] vfs_write+0x8f/0x160
>        [<c1144cbd>] sys_write+0x3d/0x70
>        [<c147a0c4>] syscall_call+0x7/0xb
> 
> -> #0 (rtnl_mutex){+.+.+.}:
>        [<c109164b>] __lock_acquire+0x140b/0x1720
>        [<c1091f70>] lock_acquire+0x80/0x1b0
>        [<c147655d>] mutex_lock_nested+0x6d/0x340
>        [<c13a10c4>] rtnl_lock+0x14/0x20
>        [<f89445ac>] fcoe_update_src_mac+0x2c/0xb0 [fcoe]
>        [<f8971712>] fcoe_ctlr_timer_work+0x712/0xb60 [libfcoe]
>        [<c104fb69>] process_one_work+0x179/0x5d0
>        [<c10502f1>] worker_thread+0x121/0x2d0
>        [<c10550ed>] kthread+0x7d/0x90
>        [<c1481a82>] kernel_thread_helper+0x6/0x10
> 
> other info that might help us debug this:
> 
>  Possible unsafe locking scenario:
> 
>        CPU0                    CPU1
>        ----                    ----
>   lock(&fip->ctlr_mutex);
>                                lock(rtnl_mutex);
>                                lock(&fip->ctlr_mutex);
>   lock(rtnl_mutex);
> 
>  *** DEADLOCK ***
> 
> Signed-off-by: Robert Love <robert.w.love@intel.com>
> Nacked-by: Robert Love <robert.w.love@intel.com>

Nacked-by?  I'm assuming, since you signed off above, that you're
actually happy with the patch, and I just removed this line as some
accidental addition.

James