[PATCH net-next v2 0/7] ibmvnic: Use more consistent locking

[PATCH net-next v2 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.

Changelog[v2] [Address comments from Jakub Kicinski]
	- Fix up commit log for patch 5/7 and drop unnecessary variable
	- Format Fixes line properly (no wrapping, no blank lines)

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: serialize access to work queue
  ibmvnic: check adapter->state under state_lock
  ibmvnic: add comments about state_lock

 drivers/net/ethernet/ibm/ibmvnic.c | 347 ++++++++++++++++++++---------
 drivers/net/ethernet/ibm/ibmvnic.h |  70 +++++-
 2 files changed, 306 insertions(+), 111 deletions(-)

-- 
2.26.2

[PATCH net-next v2 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

Re: [PATCH net-next v2 1/7] ibmvnic: restore state in change-param reset

[Dany Madden]

On 2021-01-12 10:14, Sukadev Bhattiprolu wrote:
> 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>
Reviewed-by: Dany Madden <drt@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);

[PATCH net-next v2 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 net-next v2 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

Re: [PATCH net-next v2 3/7] ibmvnic: avoid allocating rwi entries

[Saeed Mahameed]

On Tue, 2021-01-12 at 10:14 -0800, Sukadev Bhattiprolu wrote:
> 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(-)
> 

[...]

> -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
       this is not the preferred comment style: ^^

I would just drop the comment here, it is clear from the name of the
enum.

Re: [PATCH net-next v2 3/7] ibmvnic: avoid allocating rwi entries

[Sukadev Bhattiprolu]

Saeed Mahameed [saeed@kernel.org] wrote:
> > -struct ibmvnic_rwi {
> > -	enum ibmvnic_reset_reason reset_reason;
> > -	struct list_head list;
> > -};
> > +			   VNIC_RESET_CHANGE_PARAM,
> > +			   VNIC_RESET_MAX};	// must be last
>        this is not the preferred comment style: ^^
> 
> I would just drop the comment here, it is clear from the name of the
> enum.
> 

Yeah, we debated and figured the comment could serve as another reminder.

Here is updated patch, fixing the comment style.

Thanks

Sukadev
---
From 59d4b23fe1f97a67436e14829368744ee288157d Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
Date: Wed, 16 Dec 2020 23:00:34 -0600
Subject: [PATCH net-next v2 3/7] ibmvnic: avoid allocating rwi entries

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 net-next v2 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 net-next v2 5/7] ibmvnic: serialize access to work queue

[Sukadev Bhattiprolu]

The work queue is used to queue reset requests like CHANGE-PARAM or
FAILOVER resets for the worker thread. When the adapter is being removed
the adapter state is set to VNIC_REMOVING and the work queue is flushed
so no new work is added. However the check for adapter being removed is
racy in that the adapter can go into REMOVING state just after we check
and we might end up adding work just as it is being flushed (or after).

Use a new spin lock, ->remove_lock to ensure that after (while) the work
queue is (being) flushed, no new work is queued.

A follow-on patch will convert the existing ->state_lock from a spin lock
to to a mutex to allow us to hold it for longer periods of time. Since
work can be scheduled from a tasklet, we cannot block on the mutex, so
use a new spin lock.

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 | 15 ++++++++++++++-
 drivers/net/ethernet/ibm/ibmvnic.h |  2 ++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index ad551418ac63..7645df37e886 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 flags;
 	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, flags);
+
 	/*
 	 * 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, flags);
 	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);
@@ -5467,7 +5472,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, flags);
 	adapter->state = VNIC_REMOVING;
+	spin_unlock_irqrestore(&adapter->remove_lock, flags);
+
 	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

Re: [PATCH net-next v2 5/7] ibmvnic: serialize access to work queue

[Saeed Mahameed]

On Tue, 2021-01-12 at 10:14 -0800, Sukadev Bhattiprolu wrote:
> The work queue is used to queue reset requests like CHANGE-PARAM or
> FAILOVER resets for the worker thread. When the adapter is being
> removed
> the adapter state is set to VNIC_REMOVING and the work queue is
> flushed
> so no new work is added. However the check for adapter being removed
> is
> racy in that the adapter can go into REMOVING state just after we
> check
> and we might end up adding work just as it is being flushed (or
> after).
> 
> Use a new spin lock, ->remove_lock to ensure that after (while) the
> work
> queue is (being) flushed, no new work is queued.
> 
> A follow-on patch will convert the existing ->state_lock from a spin
> lock
> to to a mutex to allow us to hold it for longer periods of time.
> Since
> work can be scheduled from a tasklet, we cannot block on the mutex,
> so
> use a new spin lock.
> 
> 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 | 15 ++++++++++++++-
>  drivers/net/ethernet/ibm/ibmvnic.h |  2 ++
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
> b/drivers/net/ethernet/ibm/ibmvnic.c
> index ad551418ac63..7645df37e886 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 flags;
>  	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, flags);
> +
>  	/*
>  	 * 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, flags);
>  	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);
> @@ -5467,7 +5472,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, flags);
>  	adapter->state = VNIC_REMOVING;
> +	spin_unlock_irqrestore(&adapter->remove_lock, flags);
> +

Why irqsave/restore variants ? are you expecting this spinlock to be
held in interruptcontext ?

>  	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;

Re: [PATCH net-next v2 5/7] ibmvnic: serialize access to work queue

[Sukadev Bhattiprolu]

Saeed Mahameed [saeed@kernel.org] wrote:
> On Tue, 2021-01-12 at 10:14 -0800, Sukadev Bhattiprolu wrote:

<snip>
> > @@ -5467,7 +5472,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, flags);
> >  	adapter->state = VNIC_REMOVING;
> > +	spin_unlock_irqrestore(&adapter->remove_lock, flags);
> > +
> 
> Why irqsave/restore variants ? are you expecting this spinlock to be
> held in interruptcontext ?
> 
> >  	spin_unlock_irqrestore(&adapter->state_lock, flags);

Good question.

One of the callers of ibmvnic_reset() is the ->ndo_tx_timeout()
method which gets called from the watchdog timer.

Thanks for the review.

Sukadev

Re: [PATCH net-next v2 5/7] ibmvnic: serialize access to work queue

[Jakub Kicinski]

On Tue, 12 Jan 2021 16:40:49 -0800 Sukadev Bhattiprolu wrote:
> Saeed Mahameed [saeed@kernel.org] wrote:
> > On Tue, 2021-01-12 at 10:14 -0800, Sukadev Bhattiprolu wrote:  
> 
> <snip>
> > > @@ -5467,7 +5472,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, flags);
> > >  	adapter->state = VNIC_REMOVING;
> > > +	spin_unlock_irqrestore(&adapter->remove_lock, flags);
> > > +  
> > 
> > Why irqsave/restore variants ? are you expecting this spinlock to be
> > held in interruptcontext ?
> >   
> > >  	spin_unlock_irqrestore(&adapter->state_lock, flags);  
> 
> Good question.
> 
> One of the callers of ibmvnic_reset() is the ->ndo_tx_timeout()
> method which gets called from the watchdog timer.

watchdog is a normal timer, so it's gonna run in softirq, you don't
need to mask irqs.

[PATCH net-next v2 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 | 118 ++++++++++++++++++++---------
 drivers/net/ethernet/ibm/ibmvnic.h |   5 +-
 2 files changed, 87 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 7645df37e886..3ca92a207d16 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);
@@ -5466,9 +5516,9 @@ static int ibmvnic_remove(struct vio_dev *dev)
 	struct ibmvnic_adapter *adapter = netdev_priv(netdev);
 	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;
 	}
 
@@ -5481,7 +5531,7 @@ static int ibmvnic_remove(struct vio_dev *dev)
 	adapter->state = VNIC_REMOVING;
 	spin_unlock_irqrestore(&adapter->remove_lock, flags);
 
-	spin_unlock_irqrestore(&adapter->state_lock, flags);
+	mutex_unlock(&adapter->state_lock);
 
 	flush_work(&adapter->ibmvnic_reset);
 	flush_delayed_work(&adapter->ibmvnic_delayed_reset);
@@ -5497,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 net-next v2 7/7] ibmvnic: add comments about state_lock

[Sukadev Bhattiprolu]

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

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
---
 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 3ca92a207d16..d56e73b64da4 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, flags);
 
+	/* 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 net-next v2 0/7] ibmvnic: Use more consistent locking

[Saeed Mahameed]

On Tue, 2021-01-12 at 10:14 -0800, Sukadev Bhattiprolu wrote:
> 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.
> 
> Changelog[v2] [Address comments from Jakub Kicinski]
> 	- Fix up commit log for patch 5/7 and drop unnecessary variable
> 	- Format Fixes line properly (no wrapping, no blank lines)
> 
> 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: serialize access to work queue
>   ibmvnic: check adapter->state under state_lock
>   ibmvnic: add comments about state_lock
> 
> 
Other than the two minor comments, 
Reviewed-by: Saeed Mahameed <saeedm@nvidia.com>

Re: [PATCH net-next v2 0/7] ibmvnic: Use more consistent locking

[Jakub Kicinski]

On Tue, 12 Jan 2021 10:14:34 -0800 Sukadev Bhattiprolu wrote:
> 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.

Apologies for not having enough time to suggest details, but let me
state this again - the patches which fix bugs need to go into net with
Fixes tags, the refactoring needs to go to net-next without Fixes tags.
If there are dependencies, patches go to net first, then within a week
or so the reset can be posted for net-next, after net -> net-next merge.

Re: [PATCH net-next v2 0/7] ibmvnic: Use more consistent locking

[Sukadev Bhattiprolu]

Jakub Kicinski [kuba@kernel.org] wrote:
> On Tue, 12 Jan 2021 10:14:34 -0800 Sukadev Bhattiprolu wrote:
> > 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.
> 
> Apologies for not having enough time to suggest details, but let me
> state this again - the patches which fix bugs need to go into net with
> Fixes tags, the refactoring needs to go to net-next without Fixes tags.
> If there are dependencies, patches go to net first, then within a week
> or so the reset can be posted for net-next, after net -> net-next merge.

Well, the patch set fixes a set of bugs - main one is a locking bug fixed
in patch 6. Other bugs are more minor or corner cases. Fixing the locking
bug requires some refactoring/simplifying/reordering checks so lock can be
properly acquired.

Because of the size/cleanup, should we treat it as "next" material? i.e
should I just drop the Fixes tag and resend to net-next?

Or can we ignore the size of patchset and treat it all as bug fixes?

Appreciate your input.

Thanks,

Sukadev

Re: [PATCH net-next v2 0/7] ibmvnic: Use more consistent locking

[Jakub Kicinski]

On Tue, 12 Jan 2021 20:42:47 -0800 Sukadev Bhattiprolu wrote:
> Jakub Kicinski [kuba@kernel.org] wrote:
> > On Tue, 12 Jan 2021 10:14:34 -0800 Sukadev Bhattiprolu wrote:  
> > > 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.  
> > 
> > Apologies for not having enough time to suggest details, but let me
> > state this again - the patches which fix bugs need to go into net with
> > Fixes tags, the refactoring needs to go to net-next without Fixes tags.
> > If there are dependencies, patches go to net first, then within a week
> > or so the reset can be posted for net-next, after net -> net-next merge.  
> 
> Well, the patch set fixes a set of bugs - main one is a locking bug fixed
> in patch 6. Other bugs are more minor or corner cases. Fixing the locking
> bug requires some refactoring/simplifying/reordering checks so lock can be
> properly acquired.
> 
> Because of the size/cleanup, should we treat it as "next" material? i.e
> should I just drop the Fixes tag and resend to net-next?
> 
> Or can we ignore the size of patchset and treat it all as bug fixes?

No, focus on doing this right rather than trying to justify why your
patches deserve special treatment.

Throw this entire series out and start over with the goal of fixing 
the bugs with minimal patches.

Re: [PATCH net-next v2 0/7] ibmvnic: Use more consistent locking

[Sukadev Bhattiprolu]

Jakub Kicinski [kuba@kernel.org] wrote:
> On Tue, 12 Jan 2021 20:42:47 -0800 Sukadev Bhattiprolu wrote:
> > Jakub Kicinski [kuba@kernel.org] wrote:
> > > On Tue, 12 Jan 2021 10:14:34 -0800 Sukadev Bhattiprolu wrote:  
> > > > 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.  
> > > 
> > > Apologies for not having enough time to suggest details, but let me
> > > state this again - the patches which fix bugs need to go into net with
> > > Fixes tags, the refactoring needs to go to net-next without Fixes tags.
> > > If there are dependencies, patches go to net first, then within a week
> > > or so the reset can be posted for net-next, after net -> net-next merge.  
> > 
> > Well, the patch set fixes a set of bugs - main one is a locking bug fixed
> > in patch 6. Other bugs are more minor or corner cases. Fixing the locking
> > bug requires some refactoring/simplifying/reordering checks so lock can be
> > properly acquired.
> > 
> > Because of the size/cleanup, should we treat it as "next" material? i.e
> > should I just drop the Fixes tag and resend to net-next?
> > 
> > Or can we ignore the size of patchset and treat it all as bug fixes?
> 
> No, focus on doing this right rather than trying to justify why your
> patches deserve special treatment.

I am not asking for special treatment. I just don't get the rationale
for saying that a bug fix cannot have some amount of refactoring.
Specially a bug fix like this locking one which clearly touches various
parts of the code. To take the lock properly we do have to move code
around.
> 
> Throw this entire series out and start over with the goal of fixing 
> the bugs with minimal patches.

Fixing corner case bugs that have been around for a while in code that
we are going to throw away feels like spinning wheels just to comply
with the "process".

Other than the optimization for the spin lock, there have been no
reported technical issues with the patch set. Throwing away the
patch set and starting over - I would be focusing not on the bugs
or making the driver better but somehow complying with the process.

The tiny memory leak issues I mention for completeness are just rare
corner cases and a distraction. The main issue that people actually
hit is the locking one. Fixing the locking issue touches lot of code.

I to throw away the list implementation and add a couple of simple
interfaces because if the allocation fails we call ibmvnic_close() -
that messes with the locking I am trying to fix.

Sukadev

Re: [PATCH net-next v2 0/7] ibmvnic: Use more consistent locking

[Sukadev Bhattiprolu]

Jakub Kicinski [kuba@kernel.org] wrote:
> On Tue, 12 Jan 2021 10:14:34 -0800 Sukadev Bhattiprolu wrote:
> > 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.
> 
> Apologies for not having enough time to suggest details, but let me
> state this again - the patches which fix bugs need to go into net with
> Fixes tags, the refactoring needs to go to net-next without Fixes tags.
> If there are dependencies, patches go to net first, then within a week
> or so the reset can be posted for net-next, after net -> net-next merge.

I think the locking bug fixes need the refactoring. So would it be ok to
send the refactoring (patches 2 through 4) first to net-next and when
they are merged send the the bug fixes (1, 5 and 6)? Patch 7 can be
sent to net-next later after that.

Thanks,

Sukadev