Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH net-next 0/5] refactoring of ibmvnic code
@ 2020-08-19  5:35 Lijun Pan
  2020-08-19  5:35 ` [PATCH net-next 1/5] ibmvnic: print caller in several error messages Lijun Pan
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Lijun Pan @ 2020-08-19  5:35 UTC (permalink / raw)
  To: netdev; +Cc: linuxppc-dev, Lijun Pan

This patch series refactor reset_init and init functions,
improve the debugging messages, and make some other cosmetic changes
to make the code easier to read and debug.

Lijun Pan (5):
  ibmvnic: print caller in several error messages
  ibmvnic: compare adapter->init_done_rc with more readable
    ibmvnic_rc_codes
  ibmvnic: improve ibmvnic_init and ibmvnic_reset_init
  ibmvnic: remove never executed if statement
  ibmvnic: merge ibmvnic_reset_init and ibmvnic_init

 drivers/net/ethernet/ibm/ibmvnic.c | 98 +++++++++---------------------
 1 file changed, 28 insertions(+), 70 deletions(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH net-next 1/5] ibmvnic: print caller in several error messages
  2020-08-19  5:35 [PATCH net-next 0/5] refactoring of ibmvnic code Lijun Pan
@ 2020-08-19  5:35 ` Lijun Pan
  2020-08-19 19:08   ` David Miller
  2020-08-19  5:35 ` [PATCH net-next 2/5] ibmvnic: compare adapter->init_done_rc with more readable ibmvnic_rc_codes Lijun Pan
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 8+ messages in thread
From: Lijun Pan @ 2020-08-19  5:35 UTC (permalink / raw)
  To: netdev; +Cc: linuxppc-dev, Lijun Pan

The error messages in the changed functions are exactly the same.
In order to differentiate them and make debugging easier,
we print the function names in the error messages.

Signed-off-by: Lijun Pan <ljp@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 5afb3c9c52d2..aba1cd9862ac 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -1864,7 +1864,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);
+			   "%s: Couldn't initialize crq. rc=%d\n", __func__, rc);
 		return rc;
 	}
 
@@ -2089,7 +2089,7 @@ static int do_hard_reset(struct ibmvnic_adapter *adapter,
 	rc = init_crq_queue(adapter);
 	if (rc) {
 		netdev_err(adapter->netdev,
-			   "Couldn't initialize crq. rc=%d\n", rc);
+			   "%s: Couldn't initialize crq. rc=%d\n", __func__, rc);
 		return rc;
 	}
 
@@ -2912,7 +2912,7 @@ static struct ibmvnic_sub_crq_queue *init_sub_crq_queue(struct ibmvnic_adapter
 		rc = ibmvnic_reset_crq(adapter);
 
 	if (rc == H_CLOSED) {
-		dev_warn(dev, "Partner adapter not ready, waiting.\n");
+		dev_warn(dev, "%s: Partner adapter not ready, waiting.\n", __func__);
 	} else if (rc) {
 		dev_warn(dev, "Error %d registering sub-crq\n", rc);
 		goto reg_failed;
@@ -4865,7 +4865,7 @@ static int ibmvnic_reset_crq(struct ibmvnic_adapter *adapter)
 
 	if (rc == H_CLOSED)
 		/* Adapter is good, but other end is not ready */
-		dev_warn(dev, "Partner adapter not ready\n");
+		dev_warn(dev, "%s: Partner adapter not ready\n", __func__);
 	else if (rc != 0)
 		dev_warn(dev, "Couldn't register crq (rc=%d)\n", rc);
 
@@ -4926,7 +4926,7 @@ static int init_crq_queue(struct ibmvnic_adapter *adapter)
 	retrc = rc;
 
 	if (rc == H_CLOSED) {
-		dev_warn(dev, "Partner adapter not ready\n");
+		dev_warn(dev, "%s: Partner adapter not ready\n", __func__);
 	} else if (rc) {
 		dev_warn(dev, "Error %d opening adapter\n", rc);
 		goto reg_crq_failed;
@@ -5129,8 +5129,8 @@ static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id)
 	do {
 		rc = init_crq_queue(adapter);
 		if (rc) {
-			dev_err(&dev->dev, "Couldn't initialize crq. rc=%d\n",
-				rc);
+			dev_err(&dev->dev, "%s: Couldn't initialize crq. rc=%d\n",
+				__func__, rc);
 			goto ibmvnic_init_fail;
 		}
 
-- 
2.23.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH net-next 2/5] ibmvnic: compare adapter->init_done_rc with more readable ibmvnic_rc_codes
  2020-08-19  5:35 [PATCH net-next 0/5] refactoring of ibmvnic code Lijun Pan
  2020-08-19  5:35 ` [PATCH net-next 1/5] ibmvnic: print caller in several error messages Lijun Pan
@ 2020-08-19  5:35 ` Lijun Pan
  2020-08-19  5:35 ` [PATCH net-next 3/5] ibmvnic: improve ibmvnic_init and ibmvnic_reset_init Lijun Pan
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Lijun Pan @ 2020-08-19  5:35 UTC (permalink / raw)
  To: netdev; +Cc: linuxppc-dev, Lijun Pan

Instead of comparing (adapter->init_done_rc == 1), let it
be (adapter->init_done_rc == PARTIALSUCCESS).

Signed-off-by: Lijun Pan <ljp@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 aba1cd9862ac..50e86e65961e 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -974,7 +974,7 @@ static int set_link_state(struct ibmvnic_adapter *adapter, u8 link_state)
 			return -1;
 		}
 
-		if (adapter->init_done_rc == 1) {
+		if (adapter->init_done_rc == PARTIALSUCCESS) {
 			/* Partuial success, delay and re-send */
 			mdelay(1000);
 			resend = true;
-- 
2.23.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH net-next 3/5] ibmvnic: improve ibmvnic_init and ibmvnic_reset_init
  2020-08-19  5:35 [PATCH net-next 0/5] refactoring of ibmvnic code Lijun Pan
  2020-08-19  5:35 ` [PATCH net-next 1/5] ibmvnic: print caller in several error messages Lijun Pan
  2020-08-19  5:35 ` [PATCH net-next 2/5] ibmvnic: compare adapter->init_done_rc with more readable ibmvnic_rc_codes Lijun Pan
@ 2020-08-19  5:35 ` Lijun Pan
  2020-08-19 19:09   ` David Miller
  2020-08-19  5:35 ` [PATCH net-next 4/5] ibmvnic: remove never executed if statement Lijun Pan
  2020-08-19  5:35 ` [PATCH net-next 5/5] ibmvnic: merge ibmvnic_reset_init and ibmvnic_init Lijun Pan
  4 siblings, 1 reply; 8+ messages in thread
From: Lijun Pan @ 2020-08-19  5:35 UTC (permalink / raw)
  To: netdev; +Cc: linuxppc-dev, Lijun Pan

When H_SEND_CRQ command returns with H_CLOSED, it means the
server's CRQ is not ready yet. Instead of resetting immediately,
we wait for the server to launch passive init.
ibmvnic_init() and ibmvnic_reset_init() should also return the
error code from ibmvnic_send_crq_init() call.

Signed-off-by: Lijun Pan <ljp@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 50e86e65961e..e366fd42a8c4 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -3568,8 +3568,7 @@ static int ibmvnic_send_crq(struct ibmvnic_adapter *adapter,
 	if (rc) {
 		if (rc == H_CLOSED) {
 			dev_warn(dev, "CRQ Queue closed\n");
-			if (test_bit(0, &adapter->resetting))
-				ibmvnic_reset(adapter, VNIC_RESET_FATAL);
+			/* do not reset, report the fail, wait for passive init from server */
 		}
 
 		dev_warn(dev, "Send error (rc=%d)\n", rc);
@@ -4985,7 +4984,12 @@ static int ibmvnic_reset_init(struct ibmvnic_adapter *adapter)
 
 	reinit_completion(&adapter->init_done);
 	adapter->init_done_rc = 0;
-	ibmvnic_send_crq_init(adapter);
+	rc = ibmvnic_send_crq_init(adapter);
+	if (rc) {
+		dev_err(dev, "%s: Send crq init failed with error %d\n", __func__, rc);
+		return rc;
+	}
+
 	if (!wait_for_completion_timeout(&adapter->init_done, timeout)) {
 		dev_err(dev, "Initialization sequence timed out\n");
 		return -1;
@@ -5039,7 +5043,12 @@ static int ibmvnic_init(struct ibmvnic_adapter *adapter)
 	adapter->from_passive_init = false;
 
 	adapter->init_done_rc = 0;
-	ibmvnic_send_crq_init(adapter);
+	rc = ibmvnic_send_crq_init(adapter);
+	if (rc) {
+		dev_err(dev, "%s: Send crq init failed with error %d\n", __func__, rc);
+		return rc;
+	}
+
 	if (!wait_for_completion_timeout(&adapter->init_done, timeout)) {
 		dev_err(dev, "Initialization sequence timed out\n");
 		return -1;
-- 
2.23.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH net-next 4/5] ibmvnic: remove never executed if statement
  2020-08-19  5:35 [PATCH net-next 0/5] refactoring of ibmvnic code Lijun Pan
                   ` (2 preceding siblings ...)
  2020-08-19  5:35 ` [PATCH net-next 3/5] ibmvnic: improve ibmvnic_init and ibmvnic_reset_init Lijun Pan
@ 2020-08-19  5:35 ` Lijun Pan
  2020-08-19  5:35 ` [PATCH net-next 5/5] ibmvnic: merge ibmvnic_reset_init and ibmvnic_init Lijun Pan
  4 siblings, 0 replies; 8+ messages in thread
From: Lijun Pan @ 2020-08-19  5:35 UTC (permalink / raw)
  To: netdev; +Cc: linuxppc-dev, Lijun Pan

At the beginning of the function, from_passive_init is set false by
"adapter->from_passive_init = false;",
hence the if statement will never run.

Signed-off-by: Lijun Pan <ljp@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index e366fd42a8c4..280358dce8ba 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -5000,12 +5000,6 @@ static int ibmvnic_reset_init(struct ibmvnic_adapter *adapter)
 		return adapter->init_done_rc;
 	}
 
-	if (adapter->from_passive_init) {
-		adapter->state = VNIC_OPEN;
-		adapter->from_passive_init = false;
-		return -1;
-	}
-
 	if (test_bit(0, &adapter->resetting) && !adapter->wait_for_reset &&
 	    adapter->reset_reason != VNIC_RESET_MOBILITY) {
 		if (adapter->req_rx_queues != old_num_rx_queues ||
@@ -5059,12 +5053,6 @@ static int ibmvnic_init(struct ibmvnic_adapter *adapter)
 		return adapter->init_done_rc;
 	}
 
-	if (adapter->from_passive_init) {
-		adapter->state = VNIC_OPEN;
-		adapter->from_passive_init = false;
-		return -1;
-	}
-
 	rc = init_sub_crqs(adapter);
 	if (rc) {
 		dev_err(dev, "Initialization of sub crqs failed\n");
-- 
2.23.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH net-next 5/5] ibmvnic: merge ibmvnic_reset_init and ibmvnic_init
  2020-08-19  5:35 [PATCH net-next 0/5] refactoring of ibmvnic code Lijun Pan
                   ` (3 preceding siblings ...)
  2020-08-19  5:35 ` [PATCH net-next 4/5] ibmvnic: remove never executed if statement Lijun Pan
@ 2020-08-19  5:35 ` Lijun Pan
  4 siblings, 0 replies; 8+ messages in thread
From: Lijun Pan @ 2020-08-19  5:35 UTC (permalink / raw)
  To: netdev; +Cc: linuxppc-dev, Lijun Pan

These two functions share the majority of the code, hence merge
them together. In the meanwhile, add a reset pass-in parameter
to differentiate them. Thus, the code is easier to read and to tell
the difference between reset_init and regular init.

Signed-off-by: Lijun Pan <ljp@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 65 ++++++------------------------
 1 file changed, 13 insertions(+), 52 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 280358dce8ba..c92615b74833 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -104,8 +104,7 @@ static int send_login(struct ibmvnic_adapter *adapter);
 static void send_cap_queries(struct ibmvnic_adapter *adapter);
 static int init_sub_crqs(struct ibmvnic_adapter *);
 static int init_sub_crq_irqs(struct ibmvnic_adapter *adapter);
-static int ibmvnic_init(struct ibmvnic_adapter *);
-static int ibmvnic_reset_init(struct ibmvnic_adapter *);
+static int ibmvnic_reset_init(struct ibmvnic_adapter *, bool reset);
 static void release_crq_queue(struct ibmvnic_adapter *);
 static int __ibmvnic_set_mac(struct net_device *, u8 *);
 static int init_crq_queue(struct ibmvnic_adapter *adapter);
@@ -1868,7 +1867,7 @@ static int do_change_param_reset(struct ibmvnic_adapter *adapter,
 		return rc;
 	}
 
-	rc = ibmvnic_reset_init(adapter);
+	rc = ibmvnic_reset_init(adapter, true);
 	if (rc)
 		return IBMVNIC_INIT_FAILED;
 
@@ -1986,7 +1985,7 @@ static int do_reset(struct ibmvnic_adapter *adapter,
 			goto out;
 		}
 
-		rc = ibmvnic_reset_init(adapter);
+		rc = ibmvnic_reset_init(adapter, true);
 		if (rc) {
 			rc = IBMVNIC_INIT_FAILED;
 			goto out;
@@ -2093,7 +2092,7 @@ static int do_hard_reset(struct ibmvnic_adapter *adapter,
 		return rc;
 	}
 
-	rc = ibmvnic_init(adapter);
+	rc = ibmvnic_reset_init(adapter, false);
 	if (rc)
 		return rc;
 
@@ -4970,7 +4969,7 @@ static int init_crq_queue(struct ibmvnic_adapter *adapter)
 	return retrc;
 }
 
-static int ibmvnic_reset_init(struct ibmvnic_adapter *adapter)
+static int ibmvnic_reset_init(struct ibmvnic_adapter *adapter, bool reset)
 {
 	struct device *dev = &adapter->vdev->dev;
 	unsigned long timeout = msecs_to_jiffies(30000);
@@ -4979,10 +4978,12 @@ static int ibmvnic_reset_init(struct ibmvnic_adapter *adapter)
 
 	adapter->from_passive_init = false;
 
-	old_num_rx_queues = adapter->req_rx_queues;
-	old_num_tx_queues = adapter->req_tx_queues;
+	if (reset) {
+		old_num_rx_queues = adapter->req_rx_queues;
+		old_num_tx_queues = adapter->req_tx_queues;
+		reinit_completion(&adapter->init_done);
+	}
 
-	reinit_completion(&adapter->init_done);
 	adapter->init_done_rc = 0;
 	rc = ibmvnic_send_crq_init(adapter);
 	if (rc) {
@@ -5000,7 +5001,8 @@ static int ibmvnic_reset_init(struct ibmvnic_adapter *adapter)
 		return adapter->init_done_rc;
 	}
 
-	if (test_bit(0, &adapter->resetting) && !adapter->wait_for_reset &&
+	if (reset &&
+	    test_bit(0, &adapter->resetting) && !adapter->wait_for_reset &&
 	    adapter->reset_reason != VNIC_RESET_MOBILITY) {
 		if (adapter->req_rx_queues != old_num_rx_queues ||
 		    adapter->req_tx_queues != old_num_tx_queues) {
@@ -5028,47 +5030,6 @@ static int ibmvnic_reset_init(struct ibmvnic_adapter *adapter)
 	return rc;
 }
 
-static int ibmvnic_init(struct ibmvnic_adapter *adapter)
-{
-	struct device *dev = &adapter->vdev->dev;
-	unsigned long timeout = msecs_to_jiffies(30000);
-	int rc;
-
-	adapter->from_passive_init = false;
-
-	adapter->init_done_rc = 0;
-	rc = ibmvnic_send_crq_init(adapter);
-	if (rc) {
-		dev_err(dev, "%s: Send crq init failed with error %d\n", __func__, rc);
-		return rc;
-	}
-
-	if (!wait_for_completion_timeout(&adapter->init_done, timeout)) {
-		dev_err(dev, "Initialization sequence timed out\n");
-		return -1;
-	}
-
-	if (adapter->init_done_rc) {
-		release_crq_queue(adapter);
-		return adapter->init_done_rc;
-	}
-
-	rc = init_sub_crqs(adapter);
-	if (rc) {
-		dev_err(dev, "Initialization of sub crqs failed\n");
-		release_crq_queue(adapter);
-		return rc;
-	}
-
-	rc = init_sub_crq_irqs(adapter);
-	if (rc) {
-		dev_err(dev, "Failed to initialize sub crq irqs\n");
-		release_crq_queue(adapter);
-	}
-
-	return rc;
-}
-
 static struct device_attribute dev_attr_failover;
 
 static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id)
@@ -5131,7 +5092,7 @@ static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id)
 			goto ibmvnic_init_fail;
 		}
 
-		rc = ibmvnic_init(adapter);
+		rc = ibmvnic_reset_init(adapter, false);
 		if (rc && rc != EAGAIN)
 			goto ibmvnic_init_fail;
 	} while (rc == EAGAIN);
-- 
2.23.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net-next 1/5] ibmvnic: print caller in several error messages
  2020-08-19  5:35 ` [PATCH net-next 1/5] ibmvnic: print caller in several error messages Lijun Pan
@ 2020-08-19 19:08   ` David Miller
  0 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2020-08-19 19:08 UTC (permalink / raw)
  To: ljp; +Cc: netdev, linuxppc-dev

From: Lijun Pan <ljp@linux.ibm.com>
Date: Wed, 19 Aug 2020 00:35:08 -0500

> The error messages in the changed functions are exactly the same.
> In order to differentiate them and make debugging easier,
> we print the function names in the error messages.
> 
> Signed-off-by: Lijun Pan <ljp@linux.ibm.com>

I don't see any value in emitting function names in kernel
error log messages.

Sorry.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net-next 3/5] ibmvnic: improve ibmvnic_init and ibmvnic_reset_init
  2020-08-19  5:35 ` [PATCH net-next 3/5] ibmvnic: improve ibmvnic_init and ibmvnic_reset_init Lijun Pan
@ 2020-08-19 19:09   ` David Miller
  0 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2020-08-19 19:09 UTC (permalink / raw)
  To: ljp; +Cc: netdev, linuxppc-dev

From: Lijun Pan <ljp@linux.ibm.com>
Date: Wed, 19 Aug 2020 00:35:10 -0500

> +	if (rc) {
> +		dev_err(dev, "%s: Send crq init failed with error %d\n", __func__, rc);
> +		return rc;

Consistent with my feedback on patch #1, please get rid of this __func__ stuff.

Thank you.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-08-19 19:09 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-19  5:35 [PATCH net-next 0/5] refactoring of ibmvnic code Lijun Pan
2020-08-19  5:35 ` [PATCH net-next 1/5] ibmvnic: print caller in several error messages Lijun Pan
2020-08-19 19:08   ` David Miller
2020-08-19  5:35 ` [PATCH net-next 2/5] ibmvnic: compare adapter->init_done_rc with more readable ibmvnic_rc_codes Lijun Pan
2020-08-19  5:35 ` [PATCH net-next 3/5] ibmvnic: improve ibmvnic_init and ibmvnic_reset_init Lijun Pan
2020-08-19 19:09   ` David Miller
2020-08-19  5:35 ` [PATCH net-next 4/5] ibmvnic: remove never executed if statement Lijun Pan
2020-08-19  5:35 ` [PATCH net-next 5/5] ibmvnic: merge ibmvnic_reset_init and ibmvnic_init Lijun Pan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).