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

[Jakub Kicinski]

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

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

[Thomas Falcon]

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 
---
 drivers/net/ethernet/ibm/ibmvnic.c | 117 -
 1 file changed, 102 insertions(+), 15 deletions(-)

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(>fw_done);
+
+   rc = ibmvnic_wait_for_completion(adapter, >fw_done,
+msecs_to_jiffies(1));
+   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 = >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(>fw_done);
+
+   rc = ibmvnic_wait_for_completion(adapter, >fw_done,
+msecs_to_jiffies(1));
+   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(>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, );
if (rc)
return rc;
-   wait_for_completion(>fw_done);
+
+   rc = ibmvnic_wait_for_completion(adapter, >fw_done,
+msecs_to_jiffies(1));
+   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(>fw_done);
+
+   rc = ibmvnic_wait_for_completion(adapter, >fw_done,
+msecs_to_jiffies(1));
+   if (rc) {
+   dev_err(dev, "Unable to retrieve VPD, rc = %d\n", rc);
+   kfree(adapter->vpd->buff);
+   adapter->vpd->buff = NULL;
+   return rc;
+   }