diff mbox series

[SRU,F:linux-bluefield,1/1] UBUNTU: SAUCE: pka: Handle ring open scenario when rings are busy

Message ID 1613766527-1666-2-git-send-email-mahantesh@nvidia.com
State New
Headers show
Series UBUNTU: SAUCE: pka: Handle ring open scenario when rings are busy | expand

Commit Message

Mahantesh Salimath Feb. 19, 2021, 8:28 p.m. UTC 
BugLink: https://bugs.launchpad.net/ubuntu/+source/linux-bluefield/+bug/1916289

    * Introduce mutex lock/unlock when updating
      ring status while open/close ring device. If ring status
      is busy while open operation then return -EBUSY error.

    * If -EBUSY error is received while trying to open ring,
      this means the ring is being used by another application.
      Hence, continue to look for other rings that might be available.
      If ring open fails then no need to close the ring as this might affect
      other 'pka' instances using that ring.

    * Also, first check if the ring is available (by opening the ring)
      before initializing the ring region. This will remove the need to reset
      ring region if ring is not available.

Signed-off-by: Mahantesh Salimath <mahantesh@nvidia.com>
Reviewed-by: Khalil Blaiech <kblaiech@nvidia.com>
Signed-off-by: Mahantesh Salimath <mahantesh@nvidia.com>

Comments

Kleber Sacilotto de Souza Feb. 25, 2021, 9:07 a.m. UTC | #1 
On 19.02.21 21:28, Mahantesh Salimath wrote:
> BugLink: https://bugs.launchpad.net/ubuntu/+source/linux-bluefield/+bug/1916289

The BugLink should use the short format:

https://bugs.launchpad.net/bugs/1916289

> 
>      * Introduce mutex lock/unlock when updating
>        ring status while open/close ring device. If ring status
>        is busy while open operation then return -EBUSY error.
> 
>      * If -EBUSY error is received while trying to open ring,
>        this means the ring is being used by another application.
>        Hence, continue to look for other rings that might be available.
>        If ring open fails then no need to close the ring as this might affect
>        other 'pka' instances using that ring.
> 
>      * Also, first check if the ring is available (by opening the ring)
>        before initializing the ring region. This will remove the need to reset
>        ring region if ring is not available.
> 
> Signed-off-by: Mahantesh Salimath <mahantesh@nvidia.com>
> Reviewed-by: Khalil Blaiech <kblaiech@nvidia.com>
> Signed-off-by: Mahantesh Salimath <mahantesh@nvidia.com>

The BugLink issue can be fixed when applying the patch, it doesn't need to be
re-sent. Apart from that the changes look good.

Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

> 
> diff --git a/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.c b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.c
> index 74512ba..4539bae 100644
> --- a/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.c
> +++ b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.c
> @@ -339,6 +339,7 @@ static int pka_dev_init_ring(pka_dev_ring_t *ring, uint32_t ring_id,
>           return -ENOMEM;
>       }
> 
> +    mutex_init(&ring->mutex);
>       ring->status  = PKA_DEV_RING_STATUS_INITIALIZED;
>   
>       return ret;
> @@ -1707,20 +1708,35 @@ int __pka_dev_open_ring(uint32_t ring_id)
>   
>       shim = ring->shim;
>   
> +    mutex_lock(&ring->mutex);
> +
>       if (shim->status == PKA_SHIM_STATUS_UNDEFINED ||
>             shim->status == PKA_SHIM_STATUS_CREATED ||
>             shim->status == PKA_SHIM_STATUS_FINALIZED)
> -        return -EPERM;
> +    {
> +        ret = -EPERM;
> +        goto unlock_return;
> +    }
> +
> +    if (ring->status == PKA_DEV_RING_STATUS_BUSY)
> +    {
> +        ret = -EBUSY;
> +        goto unlock_return;
> +    }
>   
>       if (ring->status != PKA_DEV_RING_STATUS_INITIALIZED)
> -        return -EPERM;
> +    {
> +        ret = -EPERM;
> +        goto unlock_return;
> +    }
>   
>       // Set ring information words.
>       ret = pka_dev_set_ring_info(ring);
>       if (ret)
>       {
>           PKA_ERROR(PKA_DEV, "failed to set ring information\n");
> -        return -EWOULDBLOCK;
> +        ret = -EWOULDBLOCK;
> +        goto unlock_return;
>       }
>   
>       if (shim->busy_ring_num == 0)
> @@ -1729,6 +1745,8 @@ int __pka_dev_open_ring(uint32_t ring_id)
>       ring->status = PKA_DEV_RING_STATUS_BUSY;
>       shim->busy_ring_num += 1;
>   
> +unlock_return:
> +    mutex_unlock(&ring->mutex);
>       return ret;
>   }
>   
> @@ -1755,9 +1773,14 @@ int __pka_dev_close_ring(uint32_t ring_id)
>   
>       shim = ring->shim;
>   
> +    mutex_lock(&ring->mutex);
> +
>       if (shim->status != PKA_SHIM_STATUS_RUNNING &&
>               ring->status != PKA_DEV_RING_STATUS_BUSY)
> -        return -EPERM;
> +    {
> +        ret = -EPERM;
> +        goto unlock_return;
> +    }
>   
>       ring->status         = PKA_DEV_RING_STATUS_INITIALIZED;
>       shim->busy_ring_num -= 1;
> @@ -1765,6 +1788,8 @@ int __pka_dev_close_ring(uint32_t ring_id)
>       if (shim->busy_ring_num == 0)
>           shim->status = PKA_SHIM_STATUS_STOPPED;
>   
> +unlock_return:
> +    mutex_unlock(&ring->mutex);
>       return ret;
>   }
>   
> diff --git a/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.h b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.h
> index 12545ae..5a6cc8fd 100644
> --- a/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.h
> +++ b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.h
> @@ -128,6 +128,8 @@ typedef struct
>       uint32_t                num_cmd_desc;   ///< number of command descriptors.
>   
>       int8_t                  status;         ///< status of the ring.
> +
> +    struct mutex            mutex;          ///< mutex lock for sharing ring device
>   } pka_dev_ring_t;
>   
>   /// defines for pka_dev_ring->status
> diff --git a/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_drv.c b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_drv.c
> index 36dfed0..f93efb66 100644
> --- a/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_drv.c
> +++ b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_drv.c
> @@ -270,20 +270,20 @@ static int pka_drv_ring_open(void *device_data)
>   	if (!try_module_get(info->module))
>   		return -ENODEV;
>   
> -	/* Initialize regions */
> -	error = pka_drv_ring_regions_init(ring_dev);
> +	ring_info.ring_id = ring_dev->device_id;
> +	error = pka_dev_open_ring(&ring_info);
>   	if (error) {
> -		PKA_ERROR(PKA_DRIVER, "failed to initialize regions\n");
> +		PKA_DEBUG(PKA_DRIVER,
> +			  "failed to open ring %u\n", ring_dev->device_id);
>   		module_put(info->module);
>   		return error;
>   	}
>   
> -	ring_info.ring_id = ring_dev->device_id;
> -	error = pka_dev_open_ring(&ring_info);
> +	/* Initialize regions */
> +	error = pka_drv_ring_regions_init(ring_dev);
>   	if (error) {
> -		PKA_ERROR(PKA_DRIVER,
> -			  "failed to open ring %u\n", ring_dev->device_id);
> -		pka_drv_ring_regions_cleanup(ring_dev);
> +		PKA_DEBUG(PKA_DRIVER, "failed to initialize regions\n");
> +		pka_dev_close_ring(&ring_info);
>   		module_put(info->module);
>   		return error;
>   	}
> @@ -303,14 +303,15 @@ static void pka_drv_ring_release(void *device_data)
>   		  "release ring device %u (device_data:%p)\n",
>   		  ring_dev->device_id, ring_dev);
>   
> +	pka_drv_ring_regions_cleanup(ring_dev);
> +
>   	ring_info.ring_id = ring_dev->device_id;
>   	error = pka_dev_close_ring(&ring_info);
>   	if (error)
> -		PKA_ERROR(PKA_DRIVER,
> +		PKA_DEBUG(PKA_DRIVER,
>   			  "failed to close ring %u\n",
>   			  ring_dev->device_id);
>   
> -	pka_drv_ring_regions_cleanup(ring_dev);
>   	module_put(info->module);
>   }
>   
>
Stefan Bader Feb. 26, 2021, 8:06 a.m. UTC | #2 
On 19.02.21 21:28, Mahantesh Salimath wrote:
> BugLink: https://bugs.launchpad.net/ubuntu/+source/linux-bluefield/+bug/1916289
> 
>     * Introduce mutex lock/unlock when updating
>       ring status while open/close ring device. If ring status
>       is busy while open operation then return -EBUSY error.
> 
>     * If -EBUSY error is received while trying to open ring,
>       this means the ring is being used by another application.
>       Hence, continue to look for other rings that might be available.
>       If ring open fails then no need to close the ring as this might affect
>       other 'pka' instances using that ring.
> 
>     * Also, first check if the ring is available (by opening the ring)
>       before initializing the ring region. This will remove the need to reset
>       ring region if ring is not available.
> 
> Signed-off-by: Mahantesh Salimath <mahantesh@nvidia.com>
> Reviewed-by: Khalil Blaiech <kblaiech@nvidia.com>
> Signed-off-by: Mahantesh Salimath <mahantesh@nvidia.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>

---

Agreeing with Kleber on all comments.

-Stefan
> 
> diff --git a/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.c b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.c
> index 74512ba..4539bae 100644
> --- a/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.c
> +++ b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.c
> @@ -339,6 +339,7 @@ static int pka_dev_init_ring(pka_dev_ring_t *ring, uint32_t ring_id,
>          return -ENOMEM;
>      }
> 
> +    mutex_init(&ring->mutex);
>      ring->status  = PKA_DEV_RING_STATUS_INITIALIZED;
>  
>      return ret;
> @@ -1707,20 +1708,35 @@ int __pka_dev_open_ring(uint32_t ring_id)
>  
>      shim = ring->shim;
>  
> +    mutex_lock(&ring->mutex);
> +
>      if (shim->status == PKA_SHIM_STATUS_UNDEFINED ||
>            shim->status == PKA_SHIM_STATUS_CREATED ||
>            shim->status == PKA_SHIM_STATUS_FINALIZED)
> -        return -EPERM;
> +    {
> +        ret = -EPERM;
> +        goto unlock_return;
> +    }
> +
> +    if (ring->status == PKA_DEV_RING_STATUS_BUSY)
> +    {
> +        ret = -EBUSY;
> +        goto unlock_return;
> +    }
>  
>      if (ring->status != PKA_DEV_RING_STATUS_INITIALIZED)
> -        return -EPERM;
> +    {
> +        ret = -EPERM;
> +        goto unlock_return;
> +    }
>  
>      // Set ring information words.
>      ret = pka_dev_set_ring_info(ring);
>      if (ret)
>      {
>          PKA_ERROR(PKA_DEV, "failed to set ring information\n");
> -        return -EWOULDBLOCK;
> +        ret = -EWOULDBLOCK;
> +        goto unlock_return;
>      }
>  
>      if (shim->busy_ring_num == 0)
> @@ -1729,6 +1745,8 @@ int __pka_dev_open_ring(uint32_t ring_id)
>      ring->status = PKA_DEV_RING_STATUS_BUSY;
>      shim->busy_ring_num += 1;
>  
> +unlock_return:
> +    mutex_unlock(&ring->mutex);
>      return ret;
>  }
>  
> @@ -1755,9 +1773,14 @@ int __pka_dev_close_ring(uint32_t ring_id)
>  
>      shim = ring->shim;
>  
> +    mutex_lock(&ring->mutex);
> +
>      if (shim->status != PKA_SHIM_STATUS_RUNNING &&
>              ring->status != PKA_DEV_RING_STATUS_BUSY)
> -        return -EPERM;
> +    {
> +        ret = -EPERM;
> +        goto unlock_return;
> +    }
>  
>      ring->status         = PKA_DEV_RING_STATUS_INITIALIZED;
>      shim->busy_ring_num -= 1;
> @@ -1765,6 +1788,8 @@ int __pka_dev_close_ring(uint32_t ring_id)
>      if (shim->busy_ring_num == 0)
>          shim->status = PKA_SHIM_STATUS_STOPPED;
>  
> +unlock_return:
> +    mutex_unlock(&ring->mutex);
>      return ret;
>  }
>  
> diff --git a/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.h b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.h
> index 12545ae..5a6cc8fd 100644
> --- a/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.h
> +++ b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.h
> @@ -128,6 +128,8 @@ typedef struct
>      uint32_t                num_cmd_desc;   ///< number of command descriptors.
>  
>      int8_t                  status;         ///< status of the ring.
> +
> +    struct mutex            mutex;          ///< mutex lock for sharing ring device
>  } pka_dev_ring_t;
>  
>  /// defines for pka_dev_ring->status
> diff --git a/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_drv.c b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_drv.c
> index 36dfed0..f93efb66 100644
> --- a/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_drv.c
> +++ b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_drv.c
> @@ -270,20 +270,20 @@ static int pka_drv_ring_open(void *device_data)
>  	if (!try_module_get(info->module))
>  		return -ENODEV;
>  
> -	/* Initialize regions */
> -	error = pka_drv_ring_regions_init(ring_dev);
> +	ring_info.ring_id = ring_dev->device_id;
> +	error = pka_dev_open_ring(&ring_info);
>  	if (error) {
> -		PKA_ERROR(PKA_DRIVER, "failed to initialize regions\n");
> +		PKA_DEBUG(PKA_DRIVER,
> +			  "failed to open ring %u\n", ring_dev->device_id);
>  		module_put(info->module);
>  		return error;
>  	}
>  
> -	ring_info.ring_id = ring_dev->device_id;
> -	error = pka_dev_open_ring(&ring_info);
> +	/* Initialize regions */
> +	error = pka_drv_ring_regions_init(ring_dev);
>  	if (error) {
> -		PKA_ERROR(PKA_DRIVER,
> -			  "failed to open ring %u\n", ring_dev->device_id);
> -		pka_drv_ring_regions_cleanup(ring_dev);
> +		PKA_DEBUG(PKA_DRIVER, "failed to initialize regions\n");
> +		pka_dev_close_ring(&ring_info);
>  		module_put(info->module);
>  		return error;
>  	}
> @@ -303,14 +303,15 @@ static void pka_drv_ring_release(void *device_data)
>  		  "release ring device %u (device_data:%p)\n",
>  		  ring_dev->device_id, ring_dev);
>  
> +	pka_drv_ring_regions_cleanup(ring_dev);
> +
>  	ring_info.ring_id = ring_dev->device_id;
>  	error = pka_dev_close_ring(&ring_info);
>  	if (error)
> -		PKA_ERROR(PKA_DRIVER,
> +		PKA_DEBUG(PKA_DRIVER,
>  			  "failed to close ring %u\n",
>  			  ring_dev->device_id);
>  
> -	pka_drv_ring_regions_cleanup(ring_dev);
>  	module_put(info->module);
>  }
>  
>
diff mbox series

Patch

diff --git a/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.c b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.c
index 74512ba..4539bae 100644
--- a/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.c
+++ b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.c
@@ -339,6 +339,7 @@  static int pka_dev_init_ring(pka_dev_ring_t *ring, uint32_t ring_id,
         return -ENOMEM;
     }

+    mutex_init(&ring->mutex);
     ring->status  = PKA_DEV_RING_STATUS_INITIALIZED;
 
     return ret;
@@ -1707,20 +1708,35 @@  int __pka_dev_open_ring(uint32_t ring_id)
 
     shim = ring->shim;
 
+    mutex_lock(&ring->mutex);
+
     if (shim->status == PKA_SHIM_STATUS_UNDEFINED ||
           shim->status == PKA_SHIM_STATUS_CREATED ||
           shim->status == PKA_SHIM_STATUS_FINALIZED)
-        return -EPERM;
+    {
+        ret = -EPERM;
+        goto unlock_return;
+    }
+
+    if (ring->status == PKA_DEV_RING_STATUS_BUSY)
+    {
+        ret = -EBUSY;
+        goto unlock_return;
+    }
 
     if (ring->status != PKA_DEV_RING_STATUS_INITIALIZED)
-        return -EPERM;
+    {
+        ret = -EPERM;
+        goto unlock_return;
+    }
 
     // Set ring information words.
     ret = pka_dev_set_ring_info(ring);
     if (ret)
     {
         PKA_ERROR(PKA_DEV, "failed to set ring information\n");
-        return -EWOULDBLOCK;
+        ret = -EWOULDBLOCK;
+        goto unlock_return;
     }
 
     if (shim->busy_ring_num == 0)
@@ -1729,6 +1745,8 @@  int __pka_dev_open_ring(uint32_t ring_id)
     ring->status = PKA_DEV_RING_STATUS_BUSY;
     shim->busy_ring_num += 1;
 
+unlock_return:
+    mutex_unlock(&ring->mutex);
     return ret;
 }
 
@@ -1755,9 +1773,14 @@  int __pka_dev_close_ring(uint32_t ring_id)
 
     shim = ring->shim;
 
+    mutex_lock(&ring->mutex);
+
     if (shim->status != PKA_SHIM_STATUS_RUNNING &&
             ring->status != PKA_DEV_RING_STATUS_BUSY)
-        return -EPERM;
+    {
+        ret = -EPERM;
+        goto unlock_return;
+    }
 
     ring->status         = PKA_DEV_RING_STATUS_INITIALIZED;
     shim->busy_ring_num -= 1;
@@ -1765,6 +1788,8 @@  int __pka_dev_close_ring(uint32_t ring_id)
     if (shim->busy_ring_num == 0)
         shim->status = PKA_SHIM_STATUS_STOPPED;
 
+unlock_return:
+    mutex_unlock(&ring->mutex);
     return ret;
 }
 
diff --git a/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.h b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.h
index 12545ae..5a6cc8fd 100644
--- a/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.h
+++ b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.h
@@ -128,6 +128,8 @@  typedef struct
     uint32_t                num_cmd_desc;   ///< number of command descriptors.
 
     int8_t                  status;         ///< status of the ring.
+
+    struct mutex            mutex;          ///< mutex lock for sharing ring device
 } pka_dev_ring_t;
 
 /// defines for pka_dev_ring->status
diff --git a/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_drv.c b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_drv.c
index 36dfed0..f93efb66 100644
--- a/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_drv.c
+++ b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_drv.c
@@ -270,20 +270,20 @@  static int pka_drv_ring_open(void *device_data)
 	if (!try_module_get(info->module))
 		return -ENODEV;
 
-	/* Initialize regions */
-	error = pka_drv_ring_regions_init(ring_dev);
+	ring_info.ring_id = ring_dev->device_id;
+	error = pka_dev_open_ring(&ring_info);
 	if (error) {
-		PKA_ERROR(PKA_DRIVER, "failed to initialize regions\n");
+		PKA_DEBUG(PKA_DRIVER,
+			  "failed to open ring %u\n", ring_dev->device_id);
 		module_put(info->module);
 		return error;
 	}
 
-	ring_info.ring_id = ring_dev->device_id;
-	error = pka_dev_open_ring(&ring_info);
+	/* Initialize regions */
+	error = pka_drv_ring_regions_init(ring_dev);
 	if (error) {
-		PKA_ERROR(PKA_DRIVER,
-			  "failed to open ring %u\n", ring_dev->device_id);
-		pka_drv_ring_regions_cleanup(ring_dev);
+		PKA_DEBUG(PKA_DRIVER, "failed to initialize regions\n");
+		pka_dev_close_ring(&ring_info);
 		module_put(info->module);
 		return error;
 	}
@@ -303,14 +303,15 @@  static void pka_drv_ring_release(void *device_data)
 		  "release ring device %u (device_data:%p)\n",
 		  ring_dev->device_id, ring_dev);
 
+	pka_drv_ring_regions_cleanup(ring_dev);
+
 	ring_info.ring_id = ring_dev->device_id;
 	error = pka_dev_close_ring(&ring_info);
 	if (error)
-		PKA_ERROR(PKA_DRIVER,
+		PKA_DEBUG(PKA_DRIVER,
 			  "failed to close ring %u\n",
 			  ring_dev->device_id);
 
-	pka_drv_ring_regions_cleanup(ring_dev);
 	module_put(info->module);
 }