[PATCHv2 0/3] fcoe: fixup device_del() inversion

[PATCHv2 0/3] fcoe: fixup device_del() inversion

[Hannes Reinecke]

Hi all,

when shutting down an FCoE target system one is greeted with a flood
of kernel warnings about

sysfs group ffffffff81eff0e0 not found for kobject 'XXX'

Reason here is that the fcoe driver tries to remove devices from
a workqueue, but the NETDEV_UNREGISTER notification expects any
calls to be synchronous as it will tear down the device once
the notifications return.
With this patch device removals are done synchronously, so the
correct ordering is respected and the warnings are gone.

As usual, comments and reviews are welcome.

Hannes Reinecke (3):
  fcoe: move fcoe_interface_remove() out of fcoe_interface_cleanup()
  fcoe: separate out fcoe_vport_remove()
  fcoe: open-code fcoe_destroy_work() for NETDEV_UNREGISTER

 drivers/scsi/fcoe/fcoe.c | 81 ++++++++++++++++++++++++++++++------------------
 1 file changed, 51 insertions(+), 30 deletions(-)

-- 
1.8.5.6

[PATCH 1/3] fcoe: move fcoe_interface_remove() out of fcoe_interface_cleanup()

[Hannes Reinecke]

This closes a possible race condition in _fcoe_create() where
we drop the rtnl_lock() before calling fcoe_interface_remove().

Signed-off-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: Lee Duncan <lduncan@suse.com>
Acked-by: Johannes Thumshirn <jth@kernel.org>
---
 drivers/scsi/fcoe/fcoe.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 85f9a3e..135bdcf 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -501,11 +501,6 @@ static void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
 	struct net_device *netdev = fcoe->netdev;
 	struct fcoe_ctlr *fip = fcoe_to_ctlr(fcoe);
 
-	rtnl_lock();
-	if (!fcoe->removed)
-		fcoe_interface_remove(fcoe);
-	rtnl_unlock();
-
 	/* Release the self-reference taken during fcoe_interface_create() */
 	/* tear-down the FCoE controller */
 	fcoe_ctlr_destroy(fip);
@@ -2140,6 +2135,11 @@ static void fcoe_destroy_work(struct work_struct *work)
 	cdev = fcoe_ctlr_to_ctlr_dev(ctlr);
 
 	fcoe_if_destroy(port->lport);
+
+	rtnl_lock();
+	if (!fcoe->removed)
+		fcoe_interface_remove(fcoe);
+	rtnl_unlock();
 	fcoe_interface_cleanup(fcoe);
 
 	mutex_unlock(&fcoe_config_mutex);
@@ -2254,6 +2254,8 @@ static int _fcoe_create(struct net_device *netdev, enum fip_mode fip_mode,
 		printk(KERN_ERR "fcoe: Failed to create interface (%s)\n",
 		       netdev->name);
 		rc = -EIO;
+		if (!fcoe->removed)
+			fcoe_interface_remove(fcoe);
 		rtnl_unlock();
 		fcoe_interface_cleanup(fcoe);
 		mutex_unlock(&fcoe_config_mutex);
-- 
1.8.5.6

[PATCH 2/3] fcoe: separate out fcoe_vport_remove()

[Hannes Reinecke]

Separate out fcoe_vport_remove() from fcoe_destroy_work().
Required for the next patch.

Signed-off-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: Lee Duncan <lduncan@suse.com>
Acked-by: Johannes Thumshirn <jth@kernel.org>
---
 drivers/scsi/fcoe/fcoe.c | 55 +++++++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 22 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 135bdcf..617348f 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -155,7 +155,7 @@ static struct fc_seq *fcoe_elsct_send(struct fc_lport *,
 static void fcoe_set_vport_symbolic_name(struct fc_vport *);
 static void fcoe_set_port_id(struct fc_lport *, u32, struct fc_frame *);
 static void fcoe_fcf_get_vlan_id(struct fcoe_fcf_device *);
-
+static void fcoe_vport_remove(struct fc_lport *);
 
 static struct fcoe_sysfs_function_template fcoe_sysfs_templ = {
 	.set_fcoe_ctlr_mode = fcoe_ctlr_mode,
@@ -2103,30 +2103,10 @@ static void fcoe_destroy_work(struct work_struct *work)
 	struct fcoe_ctlr *ctlr;
 	struct fcoe_port *port;
 	struct fcoe_interface *fcoe;
-	struct Scsi_Host *shost;
-	struct fc_host_attrs *fc_host;
-	unsigned long flags;
-	struct fc_vport *vport;
-	struct fc_vport *next_vport;
 
 	port = container_of(work, struct fcoe_port, destroy_work);
-	shost = port->lport->host;
-	fc_host = shost_to_fc_host(shost);
-
-	/* Loop through all the vports and mark them for deletion */
-	spin_lock_irqsave(shost->host_lock, flags);
-	list_for_each_entry_safe(vport, next_vport, &fc_host->vports, peers) {
-		if (vport->flags & (FC_VPORT_DEL | FC_VPORT_CREATING)) {
-			continue;
-		} else {
-			vport->flags |= FC_VPORT_DELETING;
-			queue_work(fc_host_work_q(shost),
-				   &vport->vport_delete_work);
-		}
-	}
-	spin_unlock_irqrestore(shost->host_lock, flags);
 
-	flush_workqueue(fc_host_work_q(shost));
+	fcoe_vport_remove(port->lport);
 
 	mutex_lock(&fcoe_config_mutex);
 
@@ -2747,6 +2727,37 @@ static int fcoe_vport_destroy(struct fc_vport *vport)
 }
 
 /**
+ * fcoe_vport_remove() - remove attached vports
+ * @lport: lport for which the vports should be removed
+ */
+static void fcoe_vport_remove(struct fc_lport *lport)
+{
+	struct Scsi_Host *shost;
+	struct fc_host_attrs *fc_host;
+	unsigned long flags;
+	struct fc_vport *vport;
+	struct fc_vport *next_vport;
+
+	shost = lport->host;
+	fc_host = shost_to_fc_host(shost);
+
+	/* Loop through all the vports and mark them for deletion */
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_for_each_entry_safe(vport, next_vport, &fc_host->vports, peers) {
+		if (vport->flags & (FC_VPORT_DEL | FC_VPORT_CREATING)) {
+			continue;
+		} else {
+			vport->flags |= FC_VPORT_DELETING;
+			queue_work(fc_host_work_q(shost),
+				   &vport->vport_delete_work);
+		}
+	}
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
+	flush_workqueue(fc_host_work_q(shost));
+}
+
+/**
  * fcoe_vport_disable() - change vport state
  * @vport: vport to bring online/offline
  * @disable: should the vport be disabled?
-- 
1.8.5.6

[PATCH 3/3] fcoe: open-code fcoe_destroy_work() for NETDEV_UNREGISTER

[Hannes Reinecke]

When a NETDEV_UNREGISTER notification is received the network
device is _deleted_ after the callback returns.
So we cannot use a workqueue here, as this would cause an
inversion when removing the device as the netdev is already gone.
This manifests with a nasty warning during shutdown:

sysfs group ffffffff81eff0e0 not found for kobject 'fc_host7'

So open-code fcoe_destroy_work() when receiving the notification
to avoid this inversion.

Signed-off-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: Lee Duncan <lduncan@suse.com>
Acked-by: Johannes Thumshirn <jth@kernel.org>
---
 drivers/scsi/fcoe/fcoe.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 617348f..77adced 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1009,6 +1009,8 @@ static inline int fcoe_em_config(struct fc_lport *lport)
  * fcoe_if_destroy() - Tear down a SW FCoE instance
  * @lport: The local port to be destroyed
  *
+ * Locking: Must be called with the RTNL mutex held.
+ *
  */
 static void fcoe_if_destroy(struct fc_lport *lport)
 {
@@ -1030,14 +1032,12 @@ static void fcoe_if_destroy(struct fc_lport *lport)
 	/* Free existing transmit skbs */
 	fcoe_clean_pending_queue(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 */
 	fcoe_percpu_clean(lport);
@@ -1898,7 +1898,14 @@ static int fcoe_device_notification(struct notifier_block *notifier,
 	case NETDEV_UNREGISTER:
 		list_del(&fcoe->list);
 		port = lport_priv(ctlr->lp);
-		queue_work(fcoe_wq, &port->destroy_work);
+		fcoe_vport_remove(lport);
+		mutex_lock(&fcoe_config_mutex);
+		fcoe_if_destroy(lport);
+		if (!fcoe->removed)
+			fcoe_interface_remove(fcoe);
+		fcoe_interface_cleanup(fcoe);
+		mutex_unlock(&fcoe_config_mutex);
+		fcoe_ctlr_device_delete(fcoe_ctlr_to_ctlr_dev(ctlr));
 		goto out;
 		break;
 	case NETDEV_FEAT_CHANGE:
@@ -2114,9 +2121,8 @@ static void fcoe_destroy_work(struct work_struct *work)
 	ctlr = fcoe_to_ctlr(fcoe);
 	cdev = fcoe_ctlr_to_ctlr_dev(ctlr);
 
-	fcoe_if_destroy(port->lport);
-
 	rtnl_lock();
+	fcoe_if_destroy(port->lport);
 	if (!fcoe->removed)
 		fcoe_interface_remove(fcoe);
 	rtnl_unlock();
@@ -2720,7 +2726,9 @@ static int fcoe_vport_destroy(struct fc_vport *vport)
 	mutex_unlock(&n_port->lp_mutex);
 
 	mutex_lock(&fcoe_config_mutex);
+	rtnl_lock();
 	fcoe_if_destroy(vn_port);
+	rtnl_unlock();
 	mutex_unlock(&fcoe_config_mutex);
 
 	return 0;
-- 
1.8.5.6

Re: [PATCHv2 0/3] fcoe: fixup device_del() inversion

[Martin K. Petersen]

Hannes,

> when shutting down an FCoE target system one is greeted with a flood
> of kernel warnings about

Applied to 4.15/scsi-queue. Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering