[PATCH 0/7] ibmvnic: Use more consistent locking

[PATCH 0/7] ibmvnic: Use more consistent locking

[Sukadev Bhattiprolu]

Use more consistent locking when reading/writing the adapter->state
field. This patch set fixes a race condition during ibmvnic_open()
where the adapter could be left in the PROBED state if a reset occurs
at the wrong time. This can cause networking to not come up during
boot and potentially require manual intervention in bringing up
applications that depend on the network.

Sukadev Bhattiprolu (7):
  ibmvnic: restore state in change-param reset
  ibmvnic: update reset function prototypes
  ibmvnic: avoid allocating rwi entries
  ibmvnic: switch order of checks in ibmvnic_reset
  ibmvnic: use a lock to serialize remove/reset
  ibmvnic: check adapter->state under state_lock
  ibmvnic: add comments about adapter->state_lock

 drivers/net/ethernet/ibm/ibmvnic.c | 351 ++++++++++++++++++++---------
 drivers/net/ethernet/ibm/ibmvnic.h |  70 +++++-
 2 files changed, 308 insertions(+), 113 deletions(-)

-- 
2.26.2

[PATCH 1/7] ibmvnic: restore state in change-param reset

[Sukadev Bhattiprolu]

Restore adapter state before returning from change-param reset.
In case of errors, caller will try a hard-reset anyway.

Fixes: 0cb4bc66ba5e ("ibmvnic: restore adapter state on failed reset")

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index f302504faa8a..d548779561fd 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -1960,7 +1960,7 @@ static int do_change_param_reset(struct ibmvnic_adapter *adapter,
 	if (rc) {
 		netdev_err(adapter->netdev,
 			   "Couldn't initialize crq. rc=%d\n", rc);
-		return rc;
+		goto out;
 	}
 
 	rc = ibmvnic_reset_init(adapter, true);
-- 
2.26.2

[PATCH 2/7] ibmvnic: update reset function prototypes

[Sukadev Bhattiprolu]

The reset functions need just the 'reset reason' parameter and not
the ibmvnic_rwi list element. Update the functions so we can simplify
the handling of the ->rwi_list in a follow-on patch.

Fixes: 2770a7984db5 ("ibmvnic: Introduce hard reset recovery")

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 39 ++++++++++++++++--------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index d548779561fd..cd8108dbddec 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -1929,17 +1929,17 @@ static int ibmvnic_set_mac(struct net_device *netdev, void *p)
  * events, or non-zero if we hit a fatal error and must halt.
  */
 static int do_change_param_reset(struct ibmvnic_adapter *adapter,
-				 struct ibmvnic_rwi *rwi,
+				 enum ibmvnic_reset_reason reason,
 				 u32 reset_state)
 {
 	struct net_device *netdev = adapter->netdev;
 	int i, rc;
 
 	netdev_dbg(adapter->netdev, "Change param resetting driver (%d)\n",
-		   rwi->reset_reason);
+		   reason);
 
 	netif_carrier_off(netdev);
-	adapter->reset_reason = rwi->reset_reason;
+	adapter->reset_reason = reason;
 
 	ibmvnic_cleanup(netdev);
 
@@ -2015,7 +2015,7 @@ static int do_change_param_reset(struct ibmvnic_adapter *adapter,
  * non-zero if we hit a fatal error and must halt.
  */
 static int do_reset(struct ibmvnic_adapter *adapter,
-		    struct ibmvnic_rwi *rwi, u32 reset_state)
+		    enum ibmvnic_reset_reason reason, u32 reset_state)
 {
 	u64 old_num_rx_queues, old_num_tx_queues;
 	u64 old_num_rx_slots, old_num_tx_slots;
@@ -2025,7 +2025,7 @@ static int do_reset(struct ibmvnic_adapter *adapter,
 	netdev_dbg(adapter->netdev,
 		   "[S:%d FOP:%d] Reset reason %d, reset_state %d\n",
 		   adapter->state, adapter->failover_pending,
-		   rwi->reset_reason, reset_state);
+		   reason, reset_state);
 
 	rtnl_lock();
 	/*
@@ -2033,11 +2033,11 @@ static int do_reset(struct ibmvnic_adapter *adapter,
 	 * This will ensure ibmvnic_open() has either completed or will
 	 * block until failover is complete.
 	 */
-	if (rwi->reset_reason == VNIC_RESET_FAILOVER)
+	if (reason == VNIC_RESET_FAILOVER)
 		adapter->failover_pending = false;
 
 	netif_carrier_off(netdev);
-	adapter->reset_reason = rwi->reset_reason;
+	adapter->reset_reason = reason;
 
 	old_num_rx_queues = adapter->req_rx_queues;
 	old_num_tx_queues = adapter->req_tx_queues;
@@ -2188,16 +2188,16 @@ static int do_reset(struct ibmvnic_adapter *adapter,
 }
 
 static int do_hard_reset(struct ibmvnic_adapter *adapter,
-			 struct ibmvnic_rwi *rwi, u32 reset_state)
+			 enum ibmvnic_reset_reason reason, u32 reset_state)
 {
 	struct net_device *netdev = adapter->netdev;
 	int rc;
 
 	netdev_dbg(adapter->netdev, "Hard resetting driver (%d)\n",
-		   rwi->reset_reason);
+		   reason);
 
 	netif_carrier_off(netdev);
-	adapter->reset_reason = rwi->reset_reason;
+	adapter->reset_reason = reason;
 
 	ibmvnic_cleanup(netdev);
 	release_resources(adapter);
@@ -2278,6 +2278,7 @@ static struct ibmvnic_rwi *get_next_rwi(struct ibmvnic_adapter *adapter)
 
 static void __ibmvnic_reset(struct work_struct *work)
 {
+	enum ibmvnic_reset_reason reason;
 	struct ibmvnic_rwi *rwi;
 	struct ibmvnic_adapter *adapter;
 	bool saved_state = false;
@@ -2294,6 +2295,7 @@ static void __ibmvnic_reset(struct work_struct *work)
 	}
 
 	rwi = get_next_rwi(adapter);
+	reason = rwi->reset_reason;
 	while (rwi) {
 		spin_lock_irqsave(&adapter->state_lock, flags);
 
@@ -2311,9 +2313,9 @@ static void __ibmvnic_reset(struct work_struct *work)
 		}
 		spin_unlock_irqrestore(&adapter->state_lock, flags);
 
-		if (rwi->reset_reason == VNIC_RESET_CHANGE_PARAM) {
+		if (reason == VNIC_RESET_CHANGE_PARAM) {
 			/* CHANGE_PARAM requestor holds rtnl_lock */
-			rc = do_change_param_reset(adapter, rwi, reset_state);
+			rc = do_change_param_reset(adapter, reason, reset_state);
 		} else if (adapter->force_reset_recovery) {
 			/*
 			 * Since we are doing a hard reset now, clear the
@@ -2326,11 +2328,11 @@ static void __ibmvnic_reset(struct work_struct *work)
 			if (adapter->wait_for_reset) {
 				/* Previous was CHANGE_PARAM; caller locked */
 				adapter->force_reset_recovery = false;
-				rc = do_hard_reset(adapter, rwi, reset_state);
+				rc = do_hard_reset(adapter, reason, reset_state);
 			} else {
 				rtnl_lock();
 				adapter->force_reset_recovery = false;
-				rc = do_hard_reset(adapter, rwi, reset_state);
+				rc = do_hard_reset(adapter, reason, reset_state);
 				rtnl_unlock();
 			}
 			if (rc) {
@@ -2341,9 +2343,9 @@ static void __ibmvnic_reset(struct work_struct *work)
 				set_current_state(TASK_UNINTERRUPTIBLE);
 				schedule_timeout(60 * HZ);
 			}
-		} else if (!(rwi->reset_reason == VNIC_RESET_FATAL &&
+		} else if (!(reason == VNIC_RESET_FATAL &&
 				adapter->from_passive_init)) {
-			rc = do_reset(adapter, rwi, reset_state);
+			rc = do_reset(adapter, reason, reset_state);
 		}
 		kfree(rwi);
 		adapter->last_reset_time = jiffies;
@@ -2352,9 +2354,10 @@ static void __ibmvnic_reset(struct work_struct *work)
 			netdev_dbg(adapter->netdev, "Reset failed, rc=%d\n", rc);
 
 		rwi = get_next_rwi(adapter);
+		reason = rwi->reset_reason;
 
-		if (rwi && (rwi->reset_reason == VNIC_RESET_FAILOVER ||
-			    rwi->reset_reason == VNIC_RESET_MOBILITY))
+		if (reason && (reason == VNIC_RESET_FAILOVER ||
+			       reason == VNIC_RESET_MOBILITY))
 			adapter->force_reset_recovery = true;
 	}
 
-- 
2.26.2

[PATCH 3/7] ibmvnic: avoid allocating rwi entries

[Sukadev Bhattiprolu]

Whenever we need to schedule a reset, we allocate an rwi (reset work
item?) entry and add to the list of pending resets.

Since we only add one rwi for a given reason type to the list (no duplicates).
we will only have a handful of reset types in the list - even in the
worst case. In the common case we should just have a couple of entries
at most.

Rather than allocating/freeing every time (and dealing with the corner
case of the allocation failing), use a fixed number of rwi entries.
The extra memory needed is tiny and most of it will be used over the
active life of the adapter.

This also fixes a couple of tiny memory leaks. One is in ibmvnic_reset()
where we don't free the rwi entries after deleting them from the list due
to a transport event.  The second is in __ibmvnic_reset() where if we
find that the adapter is being removed, we simply break out of the loop
(with rc = EBUSY) but ignore any rwi entries that remain on the list.

Fixes: 2770a7984db58 ("Introduce hard reset recovery")
Fixes: 36f1031c51a2 ("ibmvnic: Do not process reset during or after
      		     device removal")

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 123 +++++++++++++++++------------
 drivers/net/ethernet/ibm/ibmvnic.h |  14 ++--
 2 files changed, 78 insertions(+), 59 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index cd8108dbddec..d1c2aaed1478 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -2257,29 +2257,81 @@ static int do_hard_reset(struct ibmvnic_adapter *adapter,
 	return rc;
 }
 
-static struct ibmvnic_rwi *get_next_rwi(struct ibmvnic_adapter *adapter)
+/**
+ * Next reset will always be the first on the list.
+ * When we take it off the list, we move any remaining resets so
+ * that the next one is again the first on the list. Most of the
+ * time the pending_resets[] should have a couple of types of resets
+ * (FAILOVER, TIMEOUT or CHANGE-PARAM and less often, MOBILITY).
+ */
+static enum ibmvnic_reset_reason get_pending_reset(struct ibmvnic_adapter *adapter)
 {
-	struct ibmvnic_rwi *rwi;
+	enum ibmvnic_reset_reason *pending_resets;
+	enum ibmvnic_reset_reason reason = 0;
 	unsigned long flags;
+	int i;
 
 	spin_lock_irqsave(&adapter->rwi_lock, flags);
 
-	if (!list_empty(&adapter->rwi_list)) {
-		rwi = list_first_entry(&adapter->rwi_list, struct ibmvnic_rwi,
-				       list);
-		list_del(&rwi->list);
-	} else {
-		rwi = NULL;
+	pending_resets = &adapter->pending_resets[0];
+
+	reason = pending_resets[0];
+
+	if (reason)  {
+		for (i = 0; i < adapter->next_reset; i++) {
+			pending_resets[i] = pending_resets[i+1];
+			if (!pending_resets[i])
+				break;
+		}
+		adapter->next_reset--;
+	}
+
+	spin_unlock_irqrestore(&adapter->rwi_lock, flags);
+	return reason;
+}
+
+/**
+ * Add a pending reset, making sure not to add duplicates.
+ * If @clear is set, clear all existing resets before adding.
+ *
+ * TODO: If clear (i.e force_reset_recovery) is true AND we have a
+ * 	 duplicate reset, wouldn't it still make sense to clear the
+ * 	 queue including the duplicate and add this reset? Preserving
+ * 	 existing behavior for now.
+ */
+static void add_pending_reset(struct ibmvnic_adapter *adapter,
+			      enum ibmvnic_reset_reason reason,
+			      bool clear)
+{
+	enum ibmvnic_reset_reason *pending_resets;
+	unsigned long flags;
+	int i;
+
+	spin_lock_irqsave(&adapter->rwi_lock, flags);
+
+	pending_resets = &adapter->pending_resets[0];
+
+	for (i = 0; i < adapter->next_reset; i++) {
+		if (pending_resets[i] == reason)
+			goto out;
+	}
+
+	if (clear) {
+		for (i = 0; i < adapter->next_reset; i++) {
+			pending_resets[i] = 0;
+		}
+		adapter->next_reset = 0;
 	}
 
+	pending_resets[adapter->next_reset] = reason;
+	adapter->next_reset++;
+out:
 	spin_unlock_irqrestore(&adapter->rwi_lock, flags);
-	return rwi;
 }
 
 static void __ibmvnic_reset(struct work_struct *work)
 {
 	enum ibmvnic_reset_reason reason;
-	struct ibmvnic_rwi *rwi;
 	struct ibmvnic_adapter *adapter;
 	bool saved_state = false;
 	unsigned long flags;
@@ -2294,15 +2346,13 @@ static void __ibmvnic_reset(struct work_struct *work)
 		return;
 	}
 
-	rwi = get_next_rwi(adapter);
-	reason = rwi->reset_reason;
-	while (rwi) {
+	reason = get_pending_reset(adapter);
+	while (reason) {
 		spin_lock_irqsave(&adapter->state_lock, flags);
 
 		if (adapter->state == VNIC_REMOVING ||
 		    adapter->state == VNIC_REMOVED) {
 			spin_unlock_irqrestore(&adapter->state_lock, flags);
-			kfree(rwi);
 			rc = EBUSY;
 			break;
 		}
@@ -2347,14 +2397,12 @@ static void __ibmvnic_reset(struct work_struct *work)
 				adapter->from_passive_init)) {
 			rc = do_reset(adapter, reason, reset_state);
 		}
-		kfree(rwi);
 		adapter->last_reset_time = jiffies;
 
 		if (rc)
 			netdev_dbg(adapter->netdev, "Reset failed, rc=%d\n", rc);
 
-		rwi = get_next_rwi(adapter);
-		reason = rwi->reset_reason;
+		reason = get_pending_reset(adapter);
 
 		if (reason && (reason == VNIC_RESET_FAILOVER ||
 			       reason == VNIC_RESET_MOBILITY))
@@ -2386,17 +2434,14 @@ static void __ibmvnic_delayed_reset(struct work_struct *work)
 static int ibmvnic_reset(struct ibmvnic_adapter *adapter,
 			 enum ibmvnic_reset_reason reason)
 {
-	struct list_head *entry, *tmp_entry;
-	struct ibmvnic_rwi *rwi, *tmp;
 	struct net_device *netdev = adapter->netdev;
-	unsigned long flags;
 	int ret;
 
 	/*
 	 * If failover is pending don't schedule any other reset.
 	 * Instead let the failover complete. If there is already a
 	 * a failover reset scheduled, we will detect and drop the
-	 * duplicate reset when walking the ->rwi_list below.
+	 * duplicate reset when walking the ->pending_resets list.
 	 */
 	if (adapter->state == VNIC_REMOVING ||
 	    adapter->state == VNIC_REMOVED ||
@@ -2412,36 +2457,11 @@ static int ibmvnic_reset(struct ibmvnic_adapter *adapter,
 		goto err;
 	}
 
-	spin_lock_irqsave(&adapter->rwi_lock, flags);
-
-	list_for_each(entry, &adapter->rwi_list) {
-		tmp = list_entry(entry, struct ibmvnic_rwi, list);
-		if (tmp->reset_reason == reason) {
-			netdev_dbg(netdev, "Skipping matching reset, reason=%d\n",
-				   reason);
-			spin_unlock_irqrestore(&adapter->rwi_lock, flags);
-			ret = EBUSY;
-			goto err;
-		}
-	}
-
-	rwi = kzalloc(sizeof(*rwi), GFP_ATOMIC);
-	if (!rwi) {
-		spin_unlock_irqrestore(&adapter->rwi_lock, flags);
-		ibmvnic_close(netdev);
-		ret = ENOMEM;
-		goto err;
-	}
-	/* if we just received a transport event,
-	 * flush reset queue and process this reset
+	/* If we just received a transport event, clear
+	 * any pending resets and add just this reset.
 	 */
-	if (adapter->force_reset_recovery && !list_empty(&adapter->rwi_list)) {
-		list_for_each_safe(entry, tmp_entry, &adapter->rwi_list)
-			list_del(entry);
-	}
-	rwi->reset_reason = reason;
-	list_add_tail(&rwi->list, &adapter->rwi_list);
-	spin_unlock_irqrestore(&adapter->rwi_lock, flags);
+	add_pending_reset(adapter, reason, adapter->force_reset_recovery);
+
 	netdev_dbg(adapter->netdev, "Scheduling reset (reason %d)\n", reason);
 	schedule_work(&adapter->ibmvnic_reset);
 
@@ -5363,7 +5383,8 @@ static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id)
 	INIT_WORK(&adapter->ibmvnic_reset, __ibmvnic_reset);
 	INIT_DELAYED_WORK(&adapter->ibmvnic_delayed_reset,
 			  __ibmvnic_delayed_reset);
-	INIT_LIST_HEAD(&adapter->rwi_list);
+	adapter->next_reset = 0;
+	memset(&adapter->pending_resets, 0, sizeof(adapter->pending_resets));
 	spin_lock_init(&adapter->rwi_lock);
 	spin_lock_init(&adapter->state_lock);
 	mutex_init(&adapter->fw_lock);
diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h
index c09c3f6bba9f..1179a95a3f92 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.h
+++ b/drivers/net/ethernet/ibm/ibmvnic.h
@@ -945,17 +945,14 @@ enum vnic_state {VNIC_PROBING = 1,
 		 VNIC_REMOVING,
 		 VNIC_REMOVED};
 
-enum ibmvnic_reset_reason {VNIC_RESET_FAILOVER = 1,
+enum ibmvnic_reset_reason {VNIC_RESET_UNUSED = 0,
+			   VNIC_RESET_FAILOVER = 1,
 			   VNIC_RESET_MOBILITY,
 			   VNIC_RESET_FATAL,
 			   VNIC_RESET_NON_FATAL,
 			   VNIC_RESET_TIMEOUT,
-			   VNIC_RESET_CHANGE_PARAM};
-
-struct ibmvnic_rwi {
-	enum ibmvnic_reset_reason reset_reason;
-	struct list_head list;
-};
+			   VNIC_RESET_CHANGE_PARAM,
+			   VNIC_RESET_MAX};	// must be last
 
 struct ibmvnic_tunables {
 	u64 rx_queues;
@@ -1082,7 +1079,8 @@ struct ibmvnic_adapter {
 	enum vnic_state state;
 	enum ibmvnic_reset_reason reset_reason;
 	spinlock_t rwi_lock;
-	struct list_head rwi_list;
+	enum ibmvnic_reset_reason pending_resets[VNIC_RESET_MAX-1];
+	short next_reset;
 	struct work_struct ibmvnic_reset;
 	struct delayed_work ibmvnic_delayed_reset;
 	unsigned long resetting;
-- 
2.26.2

[PATCH 4/7] ibmvnic: switch order of checks in ibmvnic_reset

[Sukadev Bhattiprolu]

We check separately for REMOVING and PROBING in ibmvnic_reset().
Switch the order of checks to facilitate better locking  when
checking for REMOVING/REMOVED state.

Fixes: 6a2fb0e99f9c ("ibmvnic: driver initialization for kdump/kexec")

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index d1c2aaed1478..ad551418ac63 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -2437,6 +2437,12 @@ static int ibmvnic_reset(struct ibmvnic_adapter *adapter,
 	struct net_device *netdev = adapter->netdev;
 	int ret;
 
+	if (adapter->state == VNIC_PROBING) {
+		netdev_warn(netdev, "Adapter reset during probe\n");
+		ret = adapter->init_done_rc = EAGAIN;
+		goto err;
+	}
+
 	/*
 	 * If failover is pending don't schedule any other reset.
 	 * Instead let the failover complete. If there is already a
@@ -2451,12 +2457,6 @@ static int ibmvnic_reset(struct ibmvnic_adapter *adapter,
 		goto err;
 	}
 
-	if (adapter->state == VNIC_PROBING) {
-		netdev_warn(netdev, "Adapter reset during probe\n");
-		ret = adapter->init_done_rc = EAGAIN;
-		goto err;
-	}
-
 	/* If we just received a transport event, clear
 	 * any pending resets and add just this reset.
 	 */
-- 
2.26.2

[PATCH 5/7] ibmvnic: use a lock to serialize remove/reset

[Sukadev Bhattiprolu]

Use a separate lock to serialze ibmvnic_reset() and ibmvnic_remove()
functions. ibmvnic_reset() schedules work for the worker thread and
ibmvnic_remove() flushes the work before removing the adapter. We
don't want any work to be scheduled once we start removing the
adapter (i.e after we have already flushed the work).

A follow-on patch will convert the ->state_lock from a spinklock
to a mutex to allow us to hold it for longer periods of time.
ibmvnic_reset() can be called from a tasklet and cannot use the
mutex.

Fixes: 6954a9e4192b ("ibmvnic: Flush existing work items before
                     device removal")

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 16 +++++++++++++++-
 drivers/net/ethernet/ibm/ibmvnic.h |  2 ++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index ad551418ac63..c7675ab0b7e3 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -2435,6 +2435,7 @@ static int ibmvnic_reset(struct ibmvnic_adapter *adapter,
 			 enum ibmvnic_reset_reason reason)
 {
 	struct net_device *netdev = adapter->netdev;
+	unsigned long rmflags;
 	int ret;
 
 	if (adapter->state == VNIC_PROBING) {
@@ -2443,6 +2444,8 @@ static int ibmvnic_reset(struct ibmvnic_adapter *adapter,
 		goto err;
 	}
 
+	spin_lock_irqsave(&adapter->remove_lock, rmflags);
+
 	/*
 	 * If failover is pending don't schedule any other reset.
 	 * Instead let the failover complete. If there is already a
@@ -2465,8 +2468,9 @@ static int ibmvnic_reset(struct ibmvnic_adapter *adapter,
 	netdev_dbg(adapter->netdev, "Scheduling reset (reason %d)\n", reason);
 	schedule_work(&adapter->ibmvnic_reset);
 
-	return 0;
+	ret = 0;
 err:
+	spin_unlock_irqrestore(&adapter->remove_lock, rmflags);
 	return -ret;
 }
 
@@ -5387,6 +5391,7 @@ static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id)
 	memset(&adapter->pending_resets, 0, sizeof(adapter->pending_resets));
 	spin_lock_init(&adapter->rwi_lock);
 	spin_lock_init(&adapter->state_lock);
+	spin_lock_init(&adapter->remove_lock);
 	mutex_init(&adapter->fw_lock);
 	init_completion(&adapter->init_done);
 	init_completion(&adapter->fw_done);
@@ -5459,6 +5464,7 @@ static int ibmvnic_remove(struct vio_dev *dev)
 {
 	struct net_device *netdev = dev_get_drvdata(&dev->dev);
 	struct ibmvnic_adapter *adapter = netdev_priv(netdev);
+	unsigned long rmflags;
 	unsigned long flags;
 
 	spin_lock_irqsave(&adapter->state_lock, flags);
@@ -5467,7 +5473,15 @@ static int ibmvnic_remove(struct vio_dev *dev)
 		return -EBUSY;
 	}
 
+	/* If ibmvnic_reset() is scheduling a reset, wait for it to
+	 * finish. Then prevent it from scheduling any more resets
+	 * and have the reset functions ignore any resets that have
+	 * already been scheduled.
+	 */
+	spin_lock_irqsave(&adapter->remove_lock, rmflags);
 	adapter->state = VNIC_REMOVING;
+	spin_unlock_irqrestore(&adapter->remove_lock, rmflags);
+
 	spin_unlock_irqrestore(&adapter->state_lock, flags);
 
 	flush_work(&adapter->ibmvnic_reset);
diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h
index 1179a95a3f92..2779696ade09 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.h
+++ b/drivers/net/ethernet/ibm/ibmvnic.h
@@ -1081,6 +1081,8 @@ struct ibmvnic_adapter {
 	spinlock_t rwi_lock;
 	enum ibmvnic_reset_reason pending_resets[VNIC_RESET_MAX-1];
 	short next_reset;
+	/* serialize ibmvnic_reset() and ibmvnic_remove() */
+	spinlock_t remove_lock;
 	struct work_struct ibmvnic_reset;
 	struct delayed_work ibmvnic_delayed_reset;
 	unsigned long resetting;
-- 
2.26.2

[PATCH 6/7] ibmvnic: check adapter->state under state_lock

[Sukadev Bhattiprolu]

Consider following code from __ibmvnic_reset()

                spin_lock_irqsave(&adapter->state_lock, flags);

                if (adapter->state == VNIC_REMOVING ||
                    adapter->state == VNIC_REMOVED) {
                        spin_unlock_irqrestore(&adapter->state_lock, flags);
                        kfree(rwi);
                        rc = EBUSY;
                        break;
                }

                if (!saved_state) {
                        reset_state = adapter->state;
                        saved_state = true;
                }
                spin_unlock_irqrestore(&adapter->state_lock, flags);

and following from ibmvnic_open():

	if (adapter->failover_pending) {
		adapter->state = VNIC_OPEN;
		return 0;
	}

They have following issues:

	a. __ibmvnic_reset() caches the adapter->state while holding
	   the state_lock but ibmvnic_open() sets state to OPEN without
	   holding a lock.

	b. Even if adapter state changes to OPEN after __ibmvnic_reset()
	   cached the state but before reset begins, the reset process
	   will leave the adapter in PROBED state instead of OPEN state.

The reason current code caches the adapter state is so we know what state
to go back to if the reset fails. But due to recent bug fixes, the reset
functions __restore__ the adapter state on both success/failure, so we
no longer need to cache the state.

To fix the race condition b above, use ->state_lock more consistently and
throughout the open, close and reset functions. But since these may have
to block, change the ->state_lock from a spinlock to mutex.

A follow-on patch will audit/document the uses of ->state field outside
open/close/reset.

Thanks to a lot of input from Dany Madden, Lijun Pan and Rick Lindsley.

Fixes: 7d7195a026ba ("ibmvnic: Do not process device remove during device
		      reset")

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
---
---
 drivers/net/ethernet/ibm/ibmvnic.c | 119 ++++++++++++++++++++---------
 drivers/net/ethernet/ibm/ibmvnic.h |   5 +-
 2 files changed, 87 insertions(+), 37 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index c7675ab0b7e3..236ec2456a38 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -1150,6 +1150,8 @@ static int __ibmvnic_open(struct net_device *netdev)
 	enum vnic_state prev_state = adapter->state;
 	int i, rc;
 
+	WARN_ON_ONCE(!mutex_is_locked(&adapter->state_lock));
+
 	adapter->state = VNIC_OPENING;
 	replenish_pools(adapter);
 	ibmvnic_napi_enable(adapter);
@@ -1196,11 +1198,14 @@ static int ibmvnic_open(struct net_device *netdev)
 	struct ibmvnic_adapter *adapter = netdev_priv(netdev);
 	int rc;
 
+	mutex_lock(&adapter->state_lock);
+
 	/* If device failover is pending, just set device state and return.
 	 * Device operation will be handled by reset routine.
 	 */
 	if (adapter->failover_pending) {
 		adapter->state = VNIC_OPEN;
+		mutex_unlock(&adapter->state_lock);
 		return 0;
 	}
 
@@ -1228,6 +1233,8 @@ static int ibmvnic_open(struct net_device *netdev)
 		adapter->state = VNIC_OPEN;
 		rc = 0;
 	}
+
+	mutex_unlock(&adapter->state_lock);
 	return rc;
 }
 
@@ -1350,6 +1357,8 @@ static int __ibmvnic_close(struct net_device *netdev)
 	struct ibmvnic_adapter *adapter = netdev_priv(netdev);
 	int rc = 0;
 
+	WARN_ON_ONCE(!mutex_is_locked(&adapter->state_lock));
+
 	adapter->state = VNIC_CLOSING;
 	rc = set_link_state(adapter, IBMVNIC_LOGICAL_LNK_DN);
 	if (rc)
@@ -1363,6 +1372,8 @@ static int ibmvnic_close(struct net_device *netdev)
 	struct ibmvnic_adapter *adapter = netdev_priv(netdev);
 	int rc;
 
+	mutex_lock(&adapter->state_lock);
+
 	netdev_dbg(netdev, "[S:%d FOP:%d FRR:%d] Closing\n",
 		   adapter->state, adapter->failover_pending,
 		   adapter->force_reset_recovery);
@@ -1372,12 +1383,15 @@ static int ibmvnic_close(struct net_device *netdev)
 	 */
 	if (adapter->failover_pending) {
 		adapter->state = VNIC_CLOSED;
+		mutex_unlock(&adapter->state_lock);
 		return 0;
 	}
 
+
 	rc = __ibmvnic_close(netdev);
 	ibmvnic_cleanup(netdev);
 
+	mutex_unlock(&adapter->state_lock);
 	return rc;
 }
 
@@ -1929,15 +1943,24 @@ static int ibmvnic_set_mac(struct net_device *netdev, void *p)
  * events, or non-zero if we hit a fatal error and must halt.
  */
 static int do_change_param_reset(struct ibmvnic_adapter *adapter,
-				 enum ibmvnic_reset_reason reason,
-				 u32 reset_state)
+				 enum ibmvnic_reset_reason reason)
 {
 	struct net_device *netdev = adapter->netdev;
+	u32 reset_state;
 	int i, rc;
 
 	netdev_dbg(adapter->netdev, "Change param resetting driver (%d)\n",
 		   reason);
 
+	mutex_lock(&adapter->state_lock);
+
+	reset_state = adapter->state;
+	if (reset_state == VNIC_REMOVING || reset_state == VNIC_REMOVED) {
+		netdev_err(netdev, "Adapter removed before change-param!\n");
+		rc = IBMVNIC_NODEV;
+		goto out;
+	}
+
 	netif_carrier_off(netdev);
 	adapter->reset_reason = reason;
 
@@ -2007,6 +2030,9 @@ static int do_change_param_reset(struct ibmvnic_adapter *adapter,
 out:
 	if (rc)
 		adapter->state = reset_state;
+
+	mutex_unlock(&adapter->state_lock);
+
 	return rc;
 }
 
@@ -2015,19 +2041,31 @@ static int do_change_param_reset(struct ibmvnic_adapter *adapter,
  * non-zero if we hit a fatal error and must halt.
  */
 static int do_reset(struct ibmvnic_adapter *adapter,
-		    enum ibmvnic_reset_reason reason, u32 reset_state)
+		    enum ibmvnic_reset_reason reason)
 {
+	struct net_device *netdev = adapter->netdev;
 	u64 old_num_rx_queues, old_num_tx_queues;
 	u64 old_num_rx_slots, old_num_tx_slots;
-	struct net_device *netdev = adapter->netdev;
+	u32 reset_state;
 	int i, rc;
 
+	rtnl_lock();
+
+	mutex_lock(&adapter->state_lock);
+
+	reset_state = adapter->state;
+
 	netdev_dbg(adapter->netdev,
 		   "[S:%d FOP:%d] Reset reason %d, reset_state %d\n",
 		   adapter->state, adapter->failover_pending,
 		   reason, reset_state);
 
-	rtnl_lock();
+	if (reset_state == VNIC_REMOVING || reset_state == VNIC_REMOVED) {
+		netdev_err(netdev, "Adapter removed before reset!\n");
+		rc = IBMVNIC_NODEV;
+		goto out;
+	}
+
 	/*
 	 * Now that we have the rtnl lock, clear any pending failover.
 	 * This will ensure ibmvnic_open() has either completed or will
@@ -2054,11 +2092,21 @@ static int do_reset(struct ibmvnic_adapter *adapter,
 		/* Release the RTNL lock before link state change and
 		 * re-acquire after the link state change to allow
 		 * linkwatch_event to grab the RTNL lock and run during
-		 * a reset.
+		 * a reset. To reacquire RTNL, we must also drop/reacquire
+		 * state_lock. Once we reacquire state_lock, we don't need
+		 * to check for REMOVING since ->resetting bit is still set
+		 * (any ibmvnic_remove() in between would have failed).
+		 *
+		 * We set the state to CLOSING above. If adapter is no
+		 * longer in CLOSING state, another thread changed the
+		 * state when we dropped the lock, so fail the reset
+		 * and retry.
 		 */
+		mutex_unlock(&adapter->state_lock);
 		rtnl_unlock();
 		rc = set_link_state(adapter, IBMVNIC_LOGICAL_LNK_DN);
 		rtnl_lock();
+		mutex_lock(&adapter->state_lock);
 		if (rc)
 			goto out;
 
@@ -2180,6 +2228,8 @@ static int do_reset(struct ibmvnic_adapter *adapter,
 	/* restore the adapter state if reset failed */
 	if (rc)
 		adapter->state = reset_state;
+
+	mutex_unlock(&adapter->state_lock);
 	rtnl_unlock();
 
 	netdev_dbg(adapter->netdev, "[S:%d FOP:%d] Reset done, rc %d\n",
@@ -2188,11 +2238,24 @@ static int do_reset(struct ibmvnic_adapter *adapter,
 }
 
 static int do_hard_reset(struct ibmvnic_adapter *adapter,
-			 enum ibmvnic_reset_reason reason, u32 reset_state)
+			 enum ibmvnic_reset_reason reason)
 {
 	struct net_device *netdev = adapter->netdev;
+	u32 reset_state;
 	int rc;
 
+	WARN_ON_ONCE(!rtnl_is_locked());
+
+	mutex_lock(&adapter->state_lock);
+
+	reset_state = adapter->state;
+
+	if (reset_state == VNIC_REMOVING || reset_state == VNIC_REMOVED) {
+		netdev_err(netdev, "Adapter removed before hard reset!\n");
+		rc = IBMVNIC_NODEV;
+		goto out;
+	}
+
 	netdev_dbg(adapter->netdev, "Hard resetting driver (%d)\n",
 		   reason);
 
@@ -2254,6 +2317,7 @@ static int do_hard_reset(struct ibmvnic_adapter *adapter,
 		adapter->state = reset_state;
 	netdev_dbg(adapter->netdev, "[S:%d FOP:%d] Hard reset done, rc %d\n",
 		   adapter->state, adapter->failover_pending, rc);
+	mutex_unlock(&adapter->state_lock);
 	return rc;
 }
 
@@ -2333,9 +2397,6 @@ static void __ibmvnic_reset(struct work_struct *work)
 {
 	enum ibmvnic_reset_reason reason;
 	struct ibmvnic_adapter *adapter;
-	bool saved_state = false;
-	unsigned long flags;
-	u32 reset_state;
 	int rc = 0;
 
 	adapter = container_of(work, struct ibmvnic_adapter, ibmvnic_reset);
@@ -2348,24 +2409,9 @@ static void __ibmvnic_reset(struct work_struct *work)
 
 	reason = get_pending_reset(adapter);
 	while (reason) {
-		spin_lock_irqsave(&adapter->state_lock, flags);
-
-		if (adapter->state == VNIC_REMOVING ||
-		    adapter->state == VNIC_REMOVED) {
-			spin_unlock_irqrestore(&adapter->state_lock, flags);
-			rc = EBUSY;
-			break;
-		}
-
-		if (!saved_state) {
-			reset_state = adapter->state;
-			saved_state = true;
-		}
-		spin_unlock_irqrestore(&adapter->state_lock, flags);
-
 		if (reason == VNIC_RESET_CHANGE_PARAM) {
 			/* CHANGE_PARAM requestor holds rtnl_lock */
-			rc = do_change_param_reset(adapter, reason, reset_state);
+			rc = do_change_param_reset(adapter, reason);
 		} else if (adapter->force_reset_recovery) {
 			/*
 			 * Since we are doing a hard reset now, clear the
@@ -2378,11 +2424,11 @@ static void __ibmvnic_reset(struct work_struct *work)
 			if (adapter->wait_for_reset) {
 				/* Previous was CHANGE_PARAM; caller locked */
 				adapter->force_reset_recovery = false;
-				rc = do_hard_reset(adapter, reason, reset_state);
+				rc = do_hard_reset(adapter, reason);
 			} else {
 				rtnl_lock();
 				adapter->force_reset_recovery = false;
-				rc = do_hard_reset(adapter, reason, reset_state);
+				rc = do_hard_reset(adapter, reason);
 				rtnl_unlock();
 			}
 			if (rc) {
@@ -2395,12 +2441,16 @@ static void __ibmvnic_reset(struct work_struct *work)
 			}
 		} else if (!(reason == VNIC_RESET_FATAL &&
 				adapter->from_passive_init)) {
-			rc = do_reset(adapter, reason, reset_state);
+			rc = do_reset(adapter, reason);
 		}
 		adapter->last_reset_time = jiffies;
 
 		if (rc)
 			netdev_dbg(adapter->netdev, "Reset failed, rc=%d\n", rc);
+		if (rc == IBMVNIC_NODEV) {
+			rc = EBUSY;
+			break;
+		}
 
 		reason = get_pending_reset(adapter);
 
@@ -5390,7 +5440,7 @@ static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id)
 	adapter->next_reset = 0;
 	memset(&adapter->pending_resets, 0, sizeof(adapter->pending_resets));
 	spin_lock_init(&adapter->rwi_lock);
-	spin_lock_init(&adapter->state_lock);
+	mutex_init(&adapter->state_lock);
 	spin_lock_init(&adapter->remove_lock);
 	mutex_init(&adapter->fw_lock);
 	init_completion(&adapter->init_done);
@@ -5465,11 +5515,10 @@ static int ibmvnic_remove(struct vio_dev *dev)
 	struct net_device *netdev = dev_get_drvdata(&dev->dev);
 	struct ibmvnic_adapter *adapter = netdev_priv(netdev);
 	unsigned long rmflags;
-	unsigned long flags;
 
-	spin_lock_irqsave(&adapter->state_lock, flags);
+	mutex_lock(&adapter->state_lock);
 	if (test_bit(0, &adapter->resetting)) {
-		spin_unlock_irqrestore(&adapter->state_lock, flags);
+		mutex_unlock(&adapter->state_lock);
 		return -EBUSY;
 	}
 
@@ -5482,7 +5531,7 @@ static int ibmvnic_remove(struct vio_dev *dev)
 	adapter->state = VNIC_REMOVING;
 	spin_unlock_irqrestore(&adapter->remove_lock, rmflags);
 
-	spin_unlock_irqrestore(&adapter->state_lock, flags);
+	mutex_unlock(&adapter->state_lock);
 
 	flush_work(&adapter->ibmvnic_reset);
 	flush_delayed_work(&adapter->ibmvnic_delayed_reset);
@@ -5498,7 +5547,7 @@ static int ibmvnic_remove(struct vio_dev *dev)
 	release_stats_buffers(adapter);
 
 	adapter->state = VNIC_REMOVED;
-
+	mutex_destroy(&adapter->state_lock);
 	rtnl_unlock();
 	mutex_destroy(&adapter->fw_lock);
 	device_remove_file(&dev->dev, &dev_attr_failover);
diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h
index 2779696ade09..ac79dfa76333 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.h
+++ b/drivers/net/ethernet/ibm/ibmvnic.h
@@ -21,6 +21,7 @@
 #define IBMVNIC_STATS_TIMEOUT	1
 #define IBMVNIC_INIT_FAILED	2
 #define IBMVNIC_OPEN_FAILED	3
+#define IBMVNIC_NODEV		4
 
 /* basic structures plus 100 2k buffers */
 #define IBMVNIC_IO_ENTITLEMENT_DEFAULT	610305
@@ -1097,6 +1098,6 @@ struct ibmvnic_adapter {
 	struct ibmvnic_tunables desired;
 	struct ibmvnic_tunables fallback;
 
-	/* Used for serializatin of state field */
-	spinlock_t state_lock;
+	/* Used for serialization of state field */
+	struct mutex state_lock;
 };
-- 
2.26.2

[PATCH 7/7] ibmvnic: add comments about adapter->state_lock

[Sukadev Bhattiprolu]

Add some comments, notes and TODOs about ->state_lock and RTNL.

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
---
Note:	This is fixing lot of comments so not identifying fixes. It
  	"seems" to fit this patch set but can send to net-next if
	necessary.

 drivers/net/ethernet/ibm/ibmvnic.c | 58 ++++++++++++++++++++++++++++++
 drivers/net/ethernet/ibm/ibmvnic.h | 51 +++++++++++++++++++++++++-
 2 files changed, 108 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 236ec2456a38..1aae730ddafd 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -1202,6 +1202,14 @@ static int ibmvnic_open(struct net_device *netdev)
 
 	/* If device failover is pending, just set device state and return.
 	 * Device operation will be handled by reset routine.
+	 *
+	 * Note that ->failover_pending is not protected by ->state_lock
+	 * because the tasklet (executing ibmvnic_handle_crq()) cannot
+	 * block. Even otherwise this can deadlock due to CRQs issued in
+	 * ibmvnic_open().
+	 *
+	 * We check failover_pending again at the end in case of errors.
+	 * so its okay if we miss the change to true here.
 	 */
 	if (adapter->failover_pending) {
 		adapter->state = VNIC_OPEN;
@@ -1380,6 +1388,9 @@ static int ibmvnic_close(struct net_device *netdev)
 
 	/* If device failover is pending, just set device state and return.
 	 * Device operation will be handled by reset routine.
+	 *
+	 * Note that ->failover_pending is not protected by ->state_lock
+	 * See comments in ibmvnic_open().
 	 */
 	if (adapter->failover_pending) {
 		adapter->state = VNIC_CLOSED;
@@ -1930,6 +1941,14 @@ static int ibmvnic_set_mac(struct net_device *netdev, void *p)
 	if (!is_valid_ether_addr(addr->sa_data))
 		return -EADDRNOTAVAIL;
 
+	/*
+	 * TODO: Can this race with a reset? The reset could briefly
+	 *       set state to PROBED causing us to skip setting the
+	 *       mac address. When reset complets, we set the old mac
+	 *       address? Can we check ->resetting bit instead and
+	 *       save the new mac address in adapter->mac_addr
+	 *       so reset function can set it when it is done?
+	 */
 	if (adapter->state != VNIC_PROBED) {
 		ether_addr_copy(adapter->mac_addr, addr->sa_data);
 		rc = __ibmvnic_set_mac(netdev, addr->sa_data);
@@ -1941,6 +1960,14 @@ static int ibmvnic_set_mac(struct net_device *netdev, void *p)
 /**
  * do_change_param_reset returns zero if we are able to keep processing reset
  * events, or non-zero if we hit a fatal error and must halt.
+ *
+ * Notes:
+ * 	- Regardless of success/failure, this function restores adapter state
+ * 	  to what as it was on entry. In case of failure, it is assumed that
+ * 	  a new hard-reset will be attempted.
+ *	- Caller must hold the rtnl lock before calling and release upon
+ *	  return.
+ *
  */
 static int do_change_param_reset(struct ibmvnic_adapter *adapter,
 				 enum ibmvnic_reset_reason reason)
@@ -2039,6 +2066,11 @@ static int do_change_param_reset(struct ibmvnic_adapter *adapter,
 /**
  * do_reset returns zero if we are able to keep processing reset events, or
  * non-zero if we hit a fatal error and must halt.
+ *
+ * Notes:
+ * 	- Regardless of success/failure, this function restores adapter state
+ * 	  to what as it was on entry. In case of failure, it is assumed that
+ * 	  a new hard-reset will be attempted.
  */
 static int do_reset(struct ibmvnic_adapter *adapter,
 		    enum ibmvnic_reset_reason reason)
@@ -2237,6 +2269,17 @@ static int do_reset(struct ibmvnic_adapter *adapter,
 	return rc;
 }
 
+/**
+ * Perform a hard reset possibly because a prior reset encountered
+ * an error.
+ *
+ * Notes:
+ * 	- Regardless of success/failure, this function restores adapter state
+ * 	  to what as it was on entry. In case of failure, it is assumed that
+ * 	  a new hard-reset will be attempted.
+ *	- Caller must hold the rtnl lock before calling and release upon
+ *	  return.
+ */
 static int do_hard_reset(struct ibmvnic_adapter *adapter,
 			 enum ibmvnic_reset_reason reason)
 {
@@ -2651,6 +2694,11 @@ static int ibmvnic_poll(struct napi_struct *napi, int budget)
 		frames_processed++;
 	}
 
+	/* TODO: Can this race with reset and/or is release_rx_pools()?
+	 *       Is that why we check for VNIC_CLOSING? What if we go to
+	 *       CLOSING just after we check? We cannot take ->state_lock
+	 *       since we are in interrupt context.
+	 */
 	if (adapter->state != VNIC_CLOSING &&
 	    ((atomic_read(&adapter->rx_pool[scrq_num].available) <
 	      adapter->req_rx_add_entries_per_subcrq / 2) ||
@@ -5358,6 +5406,9 @@ static int ibmvnic_reset_init(struct ibmvnic_adapter *adapter, bool reset)
 	}
 
 	if (adapter->from_passive_init) {
+		/* ibmvnic_reset_init() is always called with ->state_lock
+		 * held except from ibmvnic_probe(), so safe to update state.
+		 */
 		adapter->state = VNIC_OPEN;
 		adapter->from_passive_init = false;
 		return -1;
@@ -5531,6 +5582,9 @@ static int ibmvnic_remove(struct vio_dev *dev)
 	adapter->state = VNIC_REMOVING;
 	spin_unlock_irqrestore(&adapter->remove_lock, rmflags);
 
+	/* drop state_lock so __ibmvnic_reset() can make progress
+	 * during flush_work()
+	 */
 	mutex_unlock(&adapter->state_lock);
 
 	flush_work(&adapter->ibmvnic_reset);
@@ -5546,6 +5600,9 @@ static int ibmvnic_remove(struct vio_dev *dev)
 	release_stats_token(adapter);
 	release_stats_buffers(adapter);
 
+	/* Adapter going away. There better be no one checking ->state
+	 * or getting state_lock now TODO: Do we need the REMOVED state?
+	 */
 	adapter->state = VNIC_REMOVED;
 	mutex_destroy(&adapter->state_lock);
 	rtnl_unlock();
@@ -5627,6 +5684,7 @@ static int ibmvnic_resume(struct device *dev)
 	struct net_device *netdev = dev_get_drvdata(dev);
 	struct ibmvnic_adapter *adapter = netdev_priv(netdev);
 
+	/* resuming from power-down so ignoring state_lock */
 	if (adapter->state != VNIC_OPEN)
 		return 0;
 
diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h
index ac79dfa76333..d79bc9444c9f 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.h
+++ b/drivers/net/ethernet/ibm/ibmvnic.h
@@ -963,6 +963,55 @@ struct ibmvnic_tunables {
 	u64 mtu;
 };
 
+/**
+ * ->state_lock:
+ *  	Mutex to serialize read/write of adapter->state field specially
+ *  	between open and reset functions. If rtnl also needs to be held,
+ *  	acquire rtnl first and then state_lock (to be consistent with
+ *  	functions that enter ibmvnic with rtnl already held).
+ *
+ *  	In general, ->state_lock must be held for all read/writes to the
+ *  	state field. Exceptions are:
+ *  	- checks for VNIC_PROBING state - since the adapter is itself
+ *  	  under construction and because we never go _back_ to PROBING.
+ *
+ *  	- in debug messages involving ->state
+ *
+ *  	- in ibmvnic_tx_interrupt(), ibmvnic_rx_interrupt() because
+ *  	  a) this is a mutex and b) no (known) impact of getting a stale
+ *  	  state (i.e we will likely recover on the next interrupt).
+ *
+ *  	- ibmvnic_resume() - we are resuming from a power-down state?
+ *
+ *  	- ibmvnic_reset() - see ->remove_lock below.
+ *
+ *  	Besides these, there are couple of TODOs in ibmvnic_poll() and
+ *  	ibmvnic_set_mac() that need to be investigated separately.
+ *
+ *  ->remove_lock
+ *  	A spin lock used to serialize ibmvnic_reset() and ibmvnic_remove().
+ *  	ibmvnic_reset() can be called from a tasklet which cannot block,
+ *  	so it cannot use the ->state_lock. The only states ibmvnic_reset()
+ *  	checks for are PROBING, REMOVING and REMOVED. PROBING can be ignored
+ *  	as mentioned above. On entering REMOVING state, ibmvnic_reset()
+ *  	will skip scheduling resets for the adapter.
+ *
+ *  ->pending_resets[], ->next_reset:
+ *  	A "queue" of pending resets, implemented as a simple array. Resets
+ *  	are not frequent and even when they do occur, we will usually have
+ *  	just 1 or 2 entries in the queue at any time. Note that we don't
+ *  	need/allow duplicates in the queue. In the worst case, we would have
+ *  	VNIC_RESET_MAX-1 entries (but that means adapter is processing all
+ *  	valid types of resets at once!) so the slight overhead of the array
+ *  	is probably acceptable.
+ *
+ *  	We could still use a linked list but then have to deal with allocation
+ *  	failure when scheduling a reset. We sometimes enqueue reset from a
+ *  	tasklet so cannot block when we have allocation failure. Trying to
+ *  	close the adapter on failure requires us to hold the state_lock, which
+ *  	then cannot be a mutex (tasklet cannot block) - i.e complicates locking
+ *  	just for the occasional memory allocation failure.
+ */
 struct ibmvnic_adapter {
 	struct vio_dev *vdev;
 	struct net_device *netdev;
@@ -1098,6 +1147,6 @@ struct ibmvnic_adapter {
 	struct ibmvnic_tunables desired;
 	struct ibmvnic_tunables fallback;
 
-	/* Used for serialization of state field */
+	/* see block comments above */
 	struct mutex state_lock;
 };
-- 
2.26.2

Re: [PATCH 2/7] ibmvnic: update reset function prototypes

[Jakub Kicinski]

On Thu,  7 Jan 2021 23:12:31 -0800 Sukadev Bhattiprolu wrote:
> The reset functions need just the 'reset reason' parameter and not
> the ibmvnic_rwi list element. Update the functions so we can simplify
> the handling of the ->rwi_list in a follow-on patch.
> 
> Fixes: 2770a7984db5 ("ibmvnic: Introduce hard reset recovery")
> 

No empty lines after Fixes tags, please. They should also not be
wrapped.

> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>

Are these patches for net or net-next? It looks like they are fixing
races, but at the same time they are rather large. Can you please
produce minimal fixes, e.g. patch 3 should just fix the existing leaks
rather than refactor the code to not do allocations. 130+ LoC is a lot
for a fix.

Re: [PATCH 5/7] ibmvnic: use a lock to serialize remove/reset

[Jakub Kicinski]

On Thu,  7 Jan 2021 23:12:34 -0800 Sukadev Bhattiprolu wrote:
> Use a separate lock to serialze ibmvnic_reset() and ibmvnic_remove()
> functions. ibmvnic_reset() schedules work for the worker thread and
> ibmvnic_remove() flushes the work before removing the adapter. We
> don't want any work to be scheduled once we start removing the
> adapter (i.e after we have already flushed the work).

Locking based on functions, not on data being accessed is questionable
IMO. If you don't want work to be scheduled isn't it enough to have a
bit / flag that you set to let other flows know not to schedule reset?

> @@ -5459,6 +5464,7 @@ static int ibmvnic_remove(struct vio_dev *dev)
>  {
>  	struct net_device *netdev = dev_get_drvdata(&dev->dev);
>  	struct ibmvnic_adapter *adapter = netdev_priv(netdev);
> +	unsigned long rmflags;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&adapter->state_lock, flags);
> @@ -5467,7 +5473,15 @@ static int ibmvnic_remove(struct vio_dev *dev)
>  		return -EBUSY;
>  	}

> +	spin_lock_irqsave(&adapter->remove_lock, rmflags);

You can just use flags again, no need for separate variables.

>  	adapter->state = VNIC_REMOVING;
> +	spin_unlock_irqrestore(&adapter->remove_lock, rmflags);

Re: [PATCH 2/7] ibmvnic: update reset function prototypes

[Sukadev Bhattiprolu]

Jakub Kicinski [kuba@kernel.org] wrote:
> On Thu,  7 Jan 2021 23:12:31 -0800 Sukadev Bhattiprolu wrote:
> > The reset functions need just the 'reset reason' parameter and not
> > the ibmvnic_rwi list element. Update the functions so we can simplify
> > the handling of the ->rwi_list in a follow-on patch.
> > 
> > Fixes: 2770a7984db5 ("ibmvnic: Introduce hard reset recovery")
> > 
> 
> No empty lines after Fixes tags, please. They should also not be
> wrapped.

Ah ok, will fix.
> 
> > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
> 
> Are these patches for net or net-next? It looks like they are fixing
> races, but at the same time they are rather large. Can you please
> produce minimal fixes, e.g. patch 3 should just fix the existing leaks
> rather than refactor the code to not do allocations. 130+ LoC is a lot
> for a fix.

This is a set of bug fixes, but yes a bit large. Should I submit to
net-next instead?

Thanks,

Sukadev

Re: [PATCH 5/7] ibmvnic: use a lock to serialize remove/reset

[Sukadev Bhattiprolu]

Jakub Kicinski [kuba@kernel.org] wrote:
> On Thu,  7 Jan 2021 23:12:34 -0800 Sukadev Bhattiprolu wrote:
> > Use a separate lock to serialze ibmvnic_reset() and ibmvnic_remove()
> > functions. ibmvnic_reset() schedules work for the worker thread and
> > ibmvnic_remove() flushes the work before removing the adapter. We
> > don't want any work to be scheduled once we start removing the
> > adapter (i.e after we have already flushed the work).
> 
> Locking based on functions, not on data being accessed is questionable
> IMO. If you don't want work to be scheduled isn't it enough to have a
> bit / flag that you set to let other flows know not to schedule reset?

Maybe I could improve the description, but the "data" being protected
is the work queue. Basically don't add to the work queue while/after
it is (being) flushed.

Existing code is checking for the VNIC_REMOVING state before scheduling
the work but without a lock. If state goes to REMOVING after we check,
we could schedule work after the flush?
> 
> > @@ -5459,6 +5464,7 @@ static int ibmvnic_remove(struct vio_dev *dev)
> >  {
> >  	struct net_device *netdev = dev_get_drvdata(&dev->dev);
> >  	struct ibmvnic_adapter *adapter = netdev_priv(netdev);
> > +	unsigned long rmflags;
> >  	unsigned long flags;
> >  
> >  	spin_lock_irqsave(&adapter->state_lock, flags);
> > @@ -5467,7 +5473,15 @@ static int ibmvnic_remove(struct vio_dev *dev)
> >  		return -EBUSY;
> >  	}
> 
> > +	spin_lock_irqsave(&adapter->remove_lock, rmflags);
> 
> You can just use flags again, no need for separate variables.

Ok.
> 
> >  	adapter->state = VNIC_REMOVING;
> > +	spin_unlock_irqrestore(&adapter->remove_lock, rmflags);

Re: [PATCH 2/7] ibmvnic: update reset function prototypes

[Jakub Kicinski]

On Sun, 10 Jan 2021 19:12:21 -0800 Sukadev Bhattiprolu wrote:
> Jakub Kicinski [kuba@kernel.org] wrote:
> > On Thu,  7 Jan 2021 23:12:31 -0800 Sukadev Bhattiprolu wrote:  
> > > The reset functions need just the 'reset reason' parameter and not
> > > the ibmvnic_rwi list element. Update the functions so we can simplify
> > > the handling of the ->rwi_list in a follow-on patch.
> > > 
> > > Fixes: 2770a7984db5 ("ibmvnic: Introduce hard reset recovery")
> > >   
> > 
> > No empty lines after Fixes tags, please. They should also not be
> > wrapped.  
> 
> Ah ok, will fix.
> >   
> > > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>  
> > 
> > Are these patches for net or net-next? It looks like they are fixing
> > races, but at the same time they are rather large. Can you please
> > produce minimal fixes, e.g. patch 3 should just fix the existing leaks
> > rather than refactor the code to not do allocations. 130+ LoC is a lot
> > for a fix.  
> 
> This is a set of bug fixes, but yes a bit large. Should I submit to
> net-next instead?

I'd rather you tried to address the problems with minimal patches, then
you can refactor the code in net-next.

Re: [PATCH 5/7] ibmvnic: use a lock to serialize remove/reset

[Jakub Kicinski]

On Sun, 10 Jan 2021 19:52:25 -0800 Sukadev Bhattiprolu wrote:
> Jakub Kicinski [kuba@kernel.org] wrote:
> > On Thu,  7 Jan 2021 23:12:34 -0800 Sukadev Bhattiprolu wrote:  
> > > Use a separate lock to serialze ibmvnic_reset() and ibmvnic_remove()
> > > functions. ibmvnic_reset() schedules work for the worker thread and
> > > ibmvnic_remove() flushes the work before removing the adapter. We
> > > don't want any work to be scheduled once we start removing the
> > > adapter (i.e after we have already flushed the work).  
> > 
> > Locking based on functions, not on data being accessed is questionable
> > IMO. If you don't want work to be scheduled isn't it enough to have a
> > bit / flag that you set to let other flows know not to schedule reset?  
> 
> Maybe I could improve the description, but the "data" being protected
> is the work queue. Basically don't add to the work queue while/after
> it is (being) flushed.
> 
> Existing code is checking for the VNIC_REMOVING state before scheduling
> the work but without a lock. If state goes to REMOVING after we check,
> we could schedule work after the flush?

I see, and you can't just use the state_lock because it has to be a
spin_lock? If that's the case please just update the commit message 
and comments to describe the data protected.

Re: [PATCH 2/7] ibmvnic: update reset function prototypes

[Sukadev Bhattiprolu]

Jakub Kicinski [kuba@kernel.org] wrote:
> On Sun, 10 Jan 2021 19:12:21 -0800 Sukadev Bhattiprolu wrote:
> > Jakub Kicinski [kuba@kernel.org] wrote:
> > > On Thu,  7 Jan 2021 23:12:31 -0800 Sukadev Bhattiprolu wrote:  
> > > > The reset functions need just the 'reset reason' parameter and not
> > > > the ibmvnic_rwi list element. Update the functions so we can simplify
> > > > the handling of the ->rwi_list in a follow-on patch.
> > > > 
> > > > Fixes: 2770a7984db5 ("ibmvnic: Introduce hard reset recovery")
> > > >   
> > > 
> > > No empty lines after Fixes tags, please. They should also not be
> > > wrapped.  
> > 
> > Ah ok, will fix.
> > >   
> > > > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>  
> > > 
> > > Are these patches for net or net-next? It looks like they are fixing
> > > races, but at the same time they are rather large. Can you please
> > > produce minimal fixes, e.g. patch 3 should just fix the existing leaks
> > > rather than refactor the code to not do allocations. 130+ LoC is a lot
> > > for a fix.  
> > 
> > This is a set of bug fixes, but yes a bit large. Should I submit to
> > net-next instead?
> 
> I'd rather you tried to address the problems with minimal patches, then
> you can refactor the code in net-next.

I had thought about that but fixing the leaks and then throwing away the
code did not seem very useful. The diff stat shows 78 insertions and 59
deletions. The bulk of the new code, about 70 lines including comments,
is just the fairly straightforward helper functions:

	- get_pending_reset()
	- add_pending_reset() 

Fixing the leak in the existing code would not reduce the size of these
helpers. We are now using a simpler approach with no allocations, so no
leaks.

Sukadev

Re: [PATCH 5/7] ibmvnic: use a lock to serialize remove/reset

[Sukadev Bhattiprolu]

Jakub Kicinski [kuba@kernel.org] wrote:
> On Sun, 10 Jan 2021 19:52:25 -0800 Sukadev Bhattiprolu wrote:
> > Jakub Kicinski [kuba@kernel.org] wrote:
> > > On Thu,  7 Jan 2021 23:12:34 -0800 Sukadev Bhattiprolu wrote:  
> > > > Use a separate lock to serialze ibmvnic_reset() and ibmvnic_remove()
> > > > functions. ibmvnic_reset() schedules work for the worker thread and
> > > > ibmvnic_remove() flushes the work before removing the adapter. We
> > > > don't want any work to be scheduled once we start removing the
> > > > adapter (i.e after we have already flushed the work).  
> > > 
> > > Locking based on functions, not on data being accessed is questionable
> > > IMO. If you don't want work to be scheduled isn't it enough to have a
> > > bit / flag that you set to let other flows know not to schedule reset?  
> > 
> > Maybe I could improve the description, but the "data" being protected
> > is the work queue. Basically don't add to the work queue while/after
> > it is (being) flushed.
> > 
> > Existing code is checking for the VNIC_REMOVING state before scheduling
> > the work but without a lock. If state goes to REMOVING after we check,
> > we could schedule work after the flush?
> 
> I see, and you can't just use the state_lock because it has to be a
> spin_lock? If that's the case please just update the commit message 
> and comments to describe the data protected.

Yes, has to be spin lock. Will update description.

Thanks,

Sukadev