diff mbox series

[net,3/4] ibmvnic: Bound waits for device queries

Message ID 1574451706-19058-4-git-send-email-tlfalcon@linux.ibm.com
State Changes Requested
Delegated to: David Miller
Headers show
Series ibmvnic: Harden device commands and queries | expand

Commit Message

Thomas Falcon Nov. 22, 2019, 7:41 p.m. UTC
Create a wrapper for wait_for_completion calls with additional
driver checks to ensure that the driver does not wait on a
disabled device. In those cases or if the device does not respond
in an extended amount of time, this will allow the driver an
opportunity to recover.

Signed-off-by: Thomas Falcon <tlfalcon@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 117 ++++++++++++++++++++++++++++++++-----
 1 file changed, 102 insertions(+), 15 deletions(-)

Comments

Jakub Kicinski Nov. 24, 2019, 1:46 a.m. UTC | #1
On Fri, 22 Nov 2019 13:41:45 -0600, Thomas Falcon wrote:
> +static int ibmvnic_wait_for_completion(struct ibmvnic_adapter *adapter,
> +				       struct completion *comp_done,
> +				       unsigned long timeout)
> +{
> +	struct net_device *netdev = adapter->netdev;
> +	u8 retry = 5;
> +
> +restart_timer:
> +	if (!adapter->crq.active) {
> +		netdev_err(netdev, "Device down!\n");
> +		return -ENODEV;
> +	}
> +	/* periodically check that the device is up while waiting for
> +	 * a response
> +	 */
> +	if (!wait_for_completion_timeout(comp_done, timeout / retry)) {
> +		if (!adapter->crq.active) {
> +			netdev_err(netdev, "Device down!\n");
> +			return -ENODEV;
> +		} else {
> +			retry--;
> +			if (retry)
> +				goto restart_timer;
> +			netdev_err(netdev, "Operation timing out...\n");
> +			return -ETIMEDOUT;

Hm. This is not great. I don't see the need to open code a loop with
a goto:

while (true) {
	if (down())
		return E;

	if (retry--)
		break;

	if (wait())
		return 0
}

print(time out);
return E;

The wait_for_completion_timeout() will not be very precise, but I think
with 5 sleeps it shouldn't drift off too far from the desired 10sec.

> +		}
> +	}
> +
> +	return 0;
> +}
diff mbox series

Patch

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 78a3ef70f1ef..9806eccc5670 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -159,6 +159,37 @@  static long h_reg_sub_crq(unsigned long unit_address, unsigned long token,
 	return rc;
 }
 
+static int ibmvnic_wait_for_completion(struct ibmvnic_adapter *adapter,
+				       struct completion *comp_done,
+				       unsigned long timeout)
+{
+	struct net_device *netdev = adapter->netdev;
+	u8 retry = 5;
+
+restart_timer:
+	if (!adapter->crq.active) {
+		netdev_err(netdev, "Device down!\n");
+		return -ENODEV;
+	}
+	/* periodically check that the device is up while waiting for
+	 * a response
+	 */
+	if (!wait_for_completion_timeout(comp_done, timeout / retry)) {
+		if (!adapter->crq.active) {
+			netdev_err(netdev, "Device down!\n");
+			return -ENODEV;
+		} else {
+			retry--;
+			if (retry)
+				goto restart_timer;
+			netdev_err(netdev, "Operation timing out...\n");
+			return -ETIMEDOUT;
+		}
+	}
+
+	return 0;
+}
+
 static int alloc_long_term_buff(struct ibmvnic_adapter *adapter,
 				struct ibmvnic_long_term_buff *ltb, int size)
 {
@@ -183,7 +214,16 @@  static int alloc_long_term_buff(struct ibmvnic_adapter *adapter,
 		dma_free_coherent(dev, ltb->size, ltb->buff, ltb->addr);
 		return rc;
 	}
-	wait_for_completion(&adapter->fw_done);
+
+	rc = ibmvnic_wait_for_completion(adapter, &adapter->fw_done,
+					 msecs_to_jiffies(10000));
+	if (rc) {
+		dev_err(dev,
+			"Long term map request aborted or timed out,rc = %d\n",
+			rc);
+		dma_free_coherent(dev, ltb->size, ltb->buff, ltb->addr);
+		return rc;
+	}
 
 	if (adapter->fw_done_rc) {
 		dev_err(dev, "Couldn't map long term buffer,rc = %d\n",
@@ -211,6 +251,7 @@  static void free_long_term_buff(struct ibmvnic_adapter *adapter,
 static int reset_long_term_buff(struct ibmvnic_adapter *adapter,
 				struct ibmvnic_long_term_buff *ltb)
 {
+	struct device *dev = &adapter->vdev->dev;
 	int rc;
 
 	memset(ltb->buff, 0, ltb->size);
@@ -219,10 +260,17 @@  static int reset_long_term_buff(struct ibmvnic_adapter *adapter,
 	rc = send_request_map(adapter, ltb->addr, ltb->size, ltb->map_id);
 	if (rc)
 		return rc;
-	wait_for_completion(&adapter->fw_done);
+
+	rc = ibmvnic_wait_for_completion(adapter, &adapter->fw_done,
+					 msecs_to_jiffies(10000));
+	if (rc) {
+		dev_info(dev,
+			 "Reset failed, long term map request timed out or aborted\n");
+		return rc;
+	}
 
 	if (adapter->fw_done_rc) {
-		dev_info(&adapter->vdev->dev,
+		dev_info(dev,
 			 "Reset failed, attempting to free and reallocate buffer\n");
 		free_long_term_buff(adapter, ltb);
 		return alloc_long_term_buff(adapter, ltb, ltb->size);
@@ -949,7 +997,13 @@  static int ibmvnic_get_vpd(struct ibmvnic_adapter *adapter)
 	rc = ibmvnic_send_crq(adapter, &crq);
 	if (rc)
 		return rc;
-	wait_for_completion(&adapter->fw_done);
+
+	rc = ibmvnic_wait_for_completion(adapter, &adapter->fw_done,
+					 msecs_to_jiffies(10000));
+	if (rc) {
+		dev_err(dev, "Could not retrieve VPD size, rc = %d\n", rc);
+		return rc;
+	}
 
 	if (!adapter->vpd->len)
 		return -ENODATA;
@@ -987,7 +1041,15 @@  static int ibmvnic_get_vpd(struct ibmvnic_adapter *adapter)
 		adapter->vpd->buff = NULL;
 		return rc;
 	}
-	wait_for_completion(&adapter->fw_done);
+
+	rc = ibmvnic_wait_for_completion(adapter, &adapter->fw_done,
+					 msecs_to_jiffies(10000));
+	if (rc) {
+		dev_err(dev, "Unable to retrieve VPD, rc = %d\n", rc);
+		kfree(adapter->vpd->buff);
+		adapter->vpd->buff = NULL;
+		return rc;
+	}
 
 	return 0;
 }
@@ -1696,9 +1758,10 @@  static int __ibmvnic_set_mac(struct net_device *netdev, u8 *dev_addr)
 		goto err;
 	}
 
-	wait_for_completion(&adapter->fw_done);
+	rc = ibmvnic_wait_for_completion(adapter, &adapter->fw_done,
+					 msecs_to_jiffies(10000));
 	/* netdev->dev_addr is changed in handle_change_mac_rsp function */
-	if (adapter->fw_done_rc) {
+	if (rc || adapter->fw_done_rc) {
 		rc = -EIO;
 		goto err;
 	}
@@ -2319,9 +2382,17 @@  static int wait_for_reset(struct ibmvnic_adapter *adapter)
 	reinit_completion(&adapter->reset_done);
 	adapter->wait_for_reset = true;
 	rc = ibmvnic_reset(adapter, VNIC_RESET_CHANGE_PARAM);
-	if (rc)
-		return rc;
-	wait_for_completion(&adapter->reset_done);
+
+	if (rc) {
+		ret = rc;
+		goto out;
+	}
+	rc = ibmvnic_wait_for_completion(adapter, &adapter->reset_done,
+					 msecs_to_jiffies(60000));
+	if (rc) {
+		ret = -ENODEV;
+		goto out;
+	}
 
 	ret = 0;
 	if (adapter->reset_done_rc) {
@@ -2335,10 +2406,18 @@  static int wait_for_reset(struct ibmvnic_adapter *adapter)
 		reinit_completion(&adapter->reset_done);
 		adapter->wait_for_reset = true;
 		rc = ibmvnic_reset(adapter, VNIC_RESET_CHANGE_PARAM);
-		if (rc)
-			return ret;
-		wait_for_completion(&adapter->reset_done);
+		if (rc) {
+			ret = rc;
+			goto out;
+		}
+		rc = ibmvnic_wait_for_completion(adapter, &adapter->reset_done,
+						 msecs_to_jiffies(60000));
+		if (rc) {
+			ret = -ENODEV;
+			goto out;
+		}
 	}
+out:
 	adapter->wait_for_reset = false;
 
 	return ret;
@@ -2607,7 +2686,10 @@  static void ibmvnic_get_ethtool_stats(struct net_device *dev,
 	rc = ibmvnic_send_crq(adapter, &crq);
 	if (rc)
 		return;
-	wait_for_completion(&adapter->stats_done);
+	rc = ibmvnic_wait_for_completion(adapter, &adapter->stats_done,
+					 msecs_to_jiffies(10000));
+	if (rc)
+		return;
 
 	for (i = 0; i < ARRAY_SIZE(ibmvnic_stats); i++)
 		data[i] = be64_to_cpu(IBMVNIC_GET_STAT(adapter,
@@ -4407,7 +4489,12 @@  static int send_query_phys_parms(struct ibmvnic_adapter *adapter)
 	rc = ibmvnic_send_crq(adapter, &crq);
 	if (rc)
 		return rc;
-	wait_for_completion(&adapter->fw_done);
+
+	rc = ibmvnic_wait_for_completion(adapter, &adapter->fw_done,
+					 msecs_to_jiffies(10000));
+	if (rc)
+		return rc;
+
 	return adapter->fw_done_rc ? -EIO : 0;
 }