Re: [dpdk-dev] [PATCH v13 4/6] dmadev: introduce DMA device library implementation

[Conor Walsh]

On 05/08/2021 14:12, fengchengwen wrote:
On 2021/8/5 20:56, Walsh, Conor wrote:
This patch introduce DMA device library implementation which includes
configuration and I/O with the DMA devices.
[snip]

  /**
   * @warning
@@ -952,10 +1029,27 @@ rte_dmadev_completed(uint16_t dev_id,
uint16_t vchan, const uint16_t nb_cpls,
   *   status array are also set.
   */
Hi Chenwen,

When completed status is called with status set to NULL the drivers will 
segfault.
Users may have a valid use case where they pass NULL as status so it needs to be
checked and handled appropriately.
Could you handle this within dmadev similar to what I've added below?
If added the doxygen comment will also need to be updated to specify NULL as a 
valid input.
Hi Conor,

The status must be an array pointer, so below status_tmp will not work well.

This API is slow path (vs completed API), and is designed to obtain detailed
status information, so application should pass valid status parameters.

Thanks for your quick reply.

That is true that it is designed to be slower and return detailed status 
info but should we not handle it more gracefully than segfaulting.

I don't have too strong of an opinion either way so it's ok to ignore.

/Conor.



Thanks,
Conor.

  __rte_experimental
-uint16_t
+static inline uint16_t
  rte_dmadev_completed_status(uint16_t dev_id, uint16_t vchan,
                            const uint16_t nb_cpls, uint16_t *last_idx,
-                           enum rte_dma_status_code *status);
+                           enum rte_dma_status_code *status)
+{
+       struct rte_dmadev *dev = &rte_dmadevices[dev_id];
+       uint16_t idx;
                enum rte_dma_status_code *status_tmp;
+
+#ifdef RTE_DMADEV_DEBUG
+       if (!rte_dmadev_is_valid_dev(dev_id) ||
+           vchan >= dev->data->dev_conf.max_vchans ||
+           nb_cpls == 0 || status == NULL)
+               return 0;
+       RTE_FUNC_PTR_OR_ERR_RET(*dev->completed_status, 0);
+#endif
+
+       if (last_idx == NULL)
+               last_idx = &idx;
                if (status == NULL)
                              status = &status_tmp;
+
+       return (*dev->completed_status)(dev, vchan, nb_cpls, last_idx,
status);
+}

  #ifdef __cplusplus
  }
diff --git a/lib/dmadev/rte_dmadev_core.h
b/lib/dmadev/rte_dmadev_core.h
index 599ab15..9272725 100644
--- a/lib/dmadev/rte_dmadev_core.h
+++ b/lib/dmadev/rte_dmadev_core.h
@@ -177,4 +177,6 @@ struct rte_dmadev {
        uint64_t reserved[2]; /**< Reserved for future fields. */
  } __rte_cache_aligned;

+extern struct rte_dmadev rte_dmadevices[];
+
  #endif /* _RTE_DMADEV_CORE_H_ */
diff --git a/lib/dmadev/version.map b/lib/dmadev/version.map
index 408b93c..86c5e75 100644
--- a/lib/dmadev/version.map
+++ b/lib/dmadev/version.map
@@ -27,6 +27,7 @@ EXPERIMENTAL {
  INTERNAL {
          global:

+       rte_dmadevices;
        rte_dmadev_get_device_by_name;
        rte_dmadev_pmd_allocate;
        rte_dmadev_pmd_release;
--
2.8.1