[PATCH 0/7] qaic cleanups for 6.8

[PATCH 0/7] qaic cleanups for 6.8

[Jeffrey Hugo]

A set of cleanups to the driver to improve error cases and reduce some
code duplication.

Jeffrey Hugo (2):
  accel/qaic: Fix MHI channel struct field order
  accel/qaic: Order pci_remove() operations in reverse of probe()

Pranjal Ramajor Asha Kanojiya (5):
  accel/qaic: Deprecate ->size field from attach slice IOCTL structure
  accel/qaic: Remove bo->queued field
  accel/qaic: Drop the reference to BO in error path of create BO IOCTL
  accel/qaic: Call drm_gem_create_mmap_offset() once for each BO
  accel/qaic: Leverage DRM managed APIs to release resources

 drivers/accel/qaic/mhi_controller.c |   4 +-
 drivers/accel/qaic/qaic.h           |   3 +-
 drivers/accel/qaic/qaic_data.c      |  59 ++++++------
 drivers/accel/qaic/qaic_drv.c       | 140 ++++++++++++++++++----------
 include/uapi/drm/qaic_accel.h       |  13 +--
 5 files changed, 119 insertions(+), 100 deletions(-)

-- 
2.34.1

[PATCH 1/7] accel/qaic: Deprecate ->size field from attach slice IOCTL structure

[Jeffrey Hugo]

From: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>

->size in struct qaic_attach_slice_hdr is redundant since we have BO handle
and its size can be retrieved from base BO structure.

Signed-off-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
---
 drivers/accel/qaic/qaic_data.c | 17 ++++-------------
 include/uapi/drm/qaic_accel.h  | 13 +------------
 2 files changed, 5 insertions(+), 25 deletions(-)

diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
index cf2898eda7ae..0c6f1328df68 100644
--- a/drivers/accel/qaic/qaic_data.c
+++ b/drivers/accel/qaic/qaic_data.c
@@ -830,9 +830,6 @@ static int qaic_prepare_import_bo(struct qaic_bo *bo, struct qaic_attach_slice_h
 	struct sg_table *sgt;
 	int ret;
 
-	if (obj->import_attach->dmabuf->size < hdr->size)
-		return -EINVAL;
-
 	sgt = dma_buf_map_attachment(obj->import_attach, hdr->dir);
 	if (IS_ERR(sgt)) {
 		ret = PTR_ERR(sgt);
@@ -849,9 +846,6 @@ static int qaic_prepare_export_bo(struct qaic_device *qdev, struct qaic_bo *bo,
 {
 	int ret;
 
-	if (bo->base.size < hdr->size)
-		return -EINVAL;
-
 	ret = dma_map_sgtable(&qdev->pdev->dev, bo->sgt, hdr->dir, 0);
 	if (ret)
 		return -EFAULT;
@@ -952,9 +946,6 @@ int qaic_attach_slice_bo_ioctl(struct drm_device *dev, void *data, struct drm_fi
 	if (arg_size / args->hdr.count != sizeof(*slice_ent))
 		return -EINVAL;
 
-	if (args->hdr.size == 0)
-		return -EINVAL;
-
 	if (!(args->hdr.dir == DMA_TO_DEVICE || args->hdr.dir == DMA_FROM_DEVICE))
 		return -EINVAL;
 
@@ -994,16 +985,16 @@ int qaic_attach_slice_bo_ioctl(struct drm_device *dev, void *data, struct drm_fi
 		goto free_slice_ent;
 	}
 
-	ret = qaic_validate_req(qdev, slice_ent, args->hdr.count, args->hdr.size);
-	if (ret)
-		goto free_slice_ent;
-
 	obj = drm_gem_object_lookup(file_priv, args->hdr.handle);
 	if (!obj) {
 		ret = -ENOENT;
 		goto free_slice_ent;
 	}
 
+	ret = qaic_validate_req(qdev, slice_ent, args->hdr.count, obj->size);
+	if (ret)
+		goto put_bo;
+
 	bo = to_qaic_bo(obj);
 	ret = mutex_lock_interruptible(&bo->lock);
 	if (ret)
diff --git a/include/uapi/drm/qaic_accel.h b/include/uapi/drm/qaic_accel.h
index 9dab32316aee..d3ca876a08e9 100644
--- a/include/uapi/drm/qaic_accel.h
+++ b/include/uapi/drm/qaic_accel.h
@@ -242,18 +242,7 @@ struct qaic_attach_slice_entry {
  * @dbc_id: In. Associate the sliced BO with this DBC.
  * @handle: In. GEM handle of the BO to slice.
  * @dir: In. Direction of data flow. 1 = DMA_TO_DEVICE, 2 = DMA_FROM_DEVICE
- * @size: In. Total length of BO being used. This should not exceed base
- *	  size of BO (struct drm_gem_object.base)
- *	  For BOs being allocated using DRM_IOCTL_QAIC_CREATE_BO, size of
- *	  BO requested is PAGE_SIZE aligned then allocated hence allocated
- *	  BO size maybe bigger. This size should not exceed the new
- *	  PAGE_SIZE aligned BO size.
- * @dev_addr: In. Device address this slice pushes to or pulls from.
- * @db_addr: In. Address of the doorbell to ring.
- * @db_data: In. Data to write to the doorbell.
- * @db_len: In. Size of the doorbell data in bits - 32, 16, or 8.  0 is for
- *	    inactive doorbells.
- * @offset: In. Start of this slice as an offset from the start of the BO.
+ * @size: Deprecated. This value is ignored and size of @handle is used instead.
  */
 struct qaic_attach_slice_hdr {
 	__u32 count;
-- 
2.34.1

Re: [PATCH 1/7] accel/qaic: Deprecate ->size field from attach slice IOCTL structure

[Jacek Lawrynowicz]

Reviewed-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>

On 08.12.2023 17:34, Jeffrey Hugo wrote:
> From: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
> 
> ->size in struct qaic_attach_slice_hdr is redundant since we have BO handle
> and its size can be retrieved from base BO structure.
> 
> Signed-off-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> ---
>  drivers/accel/qaic/qaic_data.c | 17 ++++-------------
>  include/uapi/drm/qaic_accel.h  | 13 +------------
>  2 files changed, 5 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
> index cf2898eda7ae..0c6f1328df68 100644
> --- a/drivers/accel/qaic/qaic_data.c
> +++ b/drivers/accel/qaic/qaic_data.c
> @@ -830,9 +830,6 @@ static int qaic_prepare_import_bo(struct qaic_bo *bo, struct qaic_attach_slice_h
>  	struct sg_table *sgt;
>  	int ret;
>  
> -	if (obj->import_attach->dmabuf->size < hdr->size)
> -		return -EINVAL;
> -
>  	sgt = dma_buf_map_attachment(obj->import_attach, hdr->dir);
>  	if (IS_ERR(sgt)) {
>  		ret = PTR_ERR(sgt);
> @@ -849,9 +846,6 @@ static int qaic_prepare_export_bo(struct qaic_device *qdev, struct qaic_bo *bo,
>  {
>  	int ret;
>  
> -	if (bo->base.size < hdr->size)
> -		return -EINVAL;
> -
>  	ret = dma_map_sgtable(&qdev->pdev->dev, bo->sgt, hdr->dir, 0);
>  	if (ret)
>  		return -EFAULT;
> @@ -952,9 +946,6 @@ int qaic_attach_slice_bo_ioctl(struct drm_device *dev, void *data, struct drm_fi
>  	if (arg_size / args->hdr.count != sizeof(*slice_ent))
>  		return -EINVAL;
>  
> -	if (args->hdr.size == 0)
> -		return -EINVAL;
> -
>  	if (!(args->hdr.dir == DMA_TO_DEVICE || args->hdr.dir == DMA_FROM_DEVICE))
>  		return -EINVAL;
>  
> @@ -994,16 +985,16 @@ int qaic_attach_slice_bo_ioctl(struct drm_device *dev, void *data, struct drm_fi
>  		goto free_slice_ent;
>  	}
>  
> -	ret = qaic_validate_req(qdev, slice_ent, args->hdr.count, args->hdr.size);
> -	if (ret)
> -		goto free_slice_ent;
> -
>  	obj = drm_gem_object_lookup(file_priv, args->hdr.handle);
>  	if (!obj) {
>  		ret = -ENOENT;
>  		goto free_slice_ent;
>  	}
>  
> +	ret = qaic_validate_req(qdev, slice_ent, args->hdr.count, obj->size);
> +	if (ret)
> +		goto put_bo;
> +
>  	bo = to_qaic_bo(obj);
>  	ret = mutex_lock_interruptible(&bo->lock);
>  	if (ret)
> diff --git a/include/uapi/drm/qaic_accel.h b/include/uapi/drm/qaic_accel.h
> index 9dab32316aee..d3ca876a08e9 100644
> --- a/include/uapi/drm/qaic_accel.h
> +++ b/include/uapi/drm/qaic_accel.h
> @@ -242,18 +242,7 @@ struct qaic_attach_slice_entry {
>   * @dbc_id: In. Associate the sliced BO with this DBC.
>   * @handle: In. GEM handle of the BO to slice.
>   * @dir: In. Direction of data flow. 1 = DMA_TO_DEVICE, 2 = DMA_FROM_DEVICE
> - * @size: In. Total length of BO being used. This should not exceed base
> - *	  size of BO (struct drm_gem_object.base)
> - *	  For BOs being allocated using DRM_IOCTL_QAIC_CREATE_BO, size of
> - *	  BO requested is PAGE_SIZE aligned then allocated hence allocated
> - *	  BO size maybe bigger. This size should not exceed the new
> - *	  PAGE_SIZE aligned BO size.
> - * @dev_addr: In. Device address this slice pushes to or pulls from.
> - * @db_addr: In. Address of the doorbell to ring.
> - * @db_data: In. Data to write to the doorbell.
> - * @db_len: In. Size of the doorbell data in bits - 32, 16, or 8.  0 is for
> - *	    inactive doorbells.
> - * @offset: In. Start of this slice as an offset from the start of the BO.
> + * @size: Deprecated. This value is ignored and size of @handle is used instead.
>   */
>  struct qaic_attach_slice_hdr {
>  	__u32 count;

[PATCH 2/7] accel/qaic: Remove bo->queued field

[Jeffrey Hugo]

From: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>

->queued field is used to track whether the BO is submitted to hardware for
DMA or not. Since same information can be retrieved using ->xfer_list field
of same structure remove ->queued as it is redundant.

Signed-off-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
---
 drivers/accel/qaic/qaic.h      |  2 --
 drivers/accel/qaic/qaic_data.c | 23 +++++++++++------------
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/accel/qaic/qaic.h b/drivers/accel/qaic/qaic.h
index 582836f9538f..2b3ef588b717 100644
--- a/drivers/accel/qaic/qaic.h
+++ b/drivers/accel/qaic/qaic.h
@@ -191,8 +191,6 @@ struct qaic_bo {
 	u32			nr_slice;
 	/* Number of slice that have been transferred by DMA engine */
 	u32			nr_slice_xfer_done;
-	/* true = BO is queued for execution, true = BO is not queued */
-	bool			queued;
 	/*
 	 * If true then user has attached slicing information to this BO by
 	 * calling DRM_IOCTL_QAIC_ATTACH_SLICE_BO ioctl.
diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
index 0c6f1328df68..89ab8fa19315 100644
--- a/drivers/accel/qaic/qaic_data.c
+++ b/drivers/accel/qaic/qaic_data.c
@@ -141,6 +141,11 @@ struct dbc_rsp {
 	__le16	status;
 } __packed;
 
+static inline bool bo_queued(struct qaic_bo *bo)
+{
+	return !list_empty(&bo->xfer_list);
+}
+
 inline int get_dbc_req_elem_size(void)
 {
 	return sizeof(struct dbc_req);
@@ -648,6 +653,7 @@ static void qaic_init_bo(struct qaic_bo *bo, bool reinit)
 	}
 	complete_all(&bo->xfer_done);
 	INIT_LIST_HEAD(&bo->slices);
+	INIT_LIST_HEAD(&bo->xfer_list);
 }
 
 static struct qaic_bo *qaic_alloc_init_bo(void)
@@ -1166,7 +1172,6 @@ static int send_bo_list_to_device(struct qaic_device *qdev, struct drm_file *fil
 	struct bo_slice *slice;
 	unsigned long flags;
 	struct qaic_bo *bo;
-	bool queued;
 	int i, j;
 	int ret;
 
@@ -1198,9 +1203,7 @@ static int send_bo_list_to_device(struct qaic_device *qdev, struct drm_file *fil
 		}
 
 		spin_lock_irqsave(&dbc->xfer_lock, flags);
-		queued = bo->queued;
-		bo->queued = true;
-		if (queued) {
+		if (bo_queued(bo)) {
 			spin_unlock_irqrestore(&dbc->xfer_lock, flags);
 			ret = -EINVAL;
 			goto unlock_bo;
@@ -1223,7 +1226,6 @@ static int send_bo_list_to_device(struct qaic_device *qdev, struct drm_file *fil
 			else
 				ret = copy_exec_reqs(qdev, slice, dbc->id, head, tail);
 			if (ret) {
-				bo->queued = false;
 				spin_unlock_irqrestore(&dbc->xfer_lock, flags);
 				goto unlock_bo;
 			}
@@ -1246,8 +1248,7 @@ static int send_bo_list_to_device(struct qaic_device *qdev, struct drm_file *fil
 		spin_lock_irqsave(&dbc->xfer_lock, flags);
 		bo = list_last_entry(&dbc->xfer_list, struct qaic_bo, xfer_list);
 		obj = &bo->base;
-		bo->queued = false;
-		list_del(&bo->xfer_list);
+		list_del_init(&bo->xfer_list);
 		spin_unlock_irqrestore(&dbc->xfer_lock, flags);
 		dma_sync_sgtable_for_cpu(&qdev->pdev->dev, bo->sgt, bo->dir);
 		drm_gem_object_put(obj);
@@ -1608,8 +1609,7 @@ irqreturn_t dbc_irq_threaded_fn(int irq, void *data)
 			 */
 			dma_sync_sgtable_for_cpu(&qdev->pdev->dev, bo->sgt, bo->dir);
 			bo->nr_slice_xfer_done = 0;
-			bo->queued = false;
-			list_del(&bo->xfer_list);
+			list_del_init(&bo->xfer_list);
 			bo->perf_stats.req_processed_ts = ktime_get_ns();
 			complete_all(&bo->xfer_done);
 			drm_gem_object_put(&bo->base);
@@ -1868,7 +1868,7 @@ int qaic_detach_slice_bo_ioctl(struct drm_device *dev, void *data, struct drm_fi
 
 	/* Check if BO is committed to H/W for DMA */
 	spin_lock_irqsave(&dbc->xfer_lock, flags);
-	if (bo->queued) {
+	if (bo_queued(bo)) {
 		spin_unlock_irqrestore(&dbc->xfer_lock, flags);
 		ret = -EBUSY;
 		goto unlock_ch_srcu;
@@ -1898,8 +1898,7 @@ static void empty_xfer_list(struct qaic_device *qdev, struct dma_bridge_chan *db
 	spin_lock_irqsave(&dbc->xfer_lock, flags);
 	while (!list_empty(&dbc->xfer_list)) {
 		bo = list_first_entry(&dbc->xfer_list, typeof(*bo), xfer_list);
-		bo->queued = false;
-		list_del(&bo->xfer_list);
+		list_del_init(&bo->xfer_list);
 		spin_unlock_irqrestore(&dbc->xfer_lock, flags);
 		bo->nr_slice_xfer_done = 0;
 		bo->req_id = 0;
-- 
2.34.1

Re: [PATCH 2/7] accel/qaic: Remove bo->queued field

[Jacek Lawrynowicz]

Reviewed-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>

On 08.12.2023 17:34, Jeffrey Hugo wrote:
> From: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
> 
> ->queued field is used to track whether the BO is submitted to hardware for
> DMA or not. Since same information can be retrieved using ->xfer_list field
> of same structure remove ->queued as it is redundant.
> 
> Signed-off-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> ---
>  drivers/accel/qaic/qaic.h      |  2 --
>  drivers/accel/qaic/qaic_data.c | 23 +++++++++++------------
>  2 files changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/accel/qaic/qaic.h b/drivers/accel/qaic/qaic.h
> index 582836f9538f..2b3ef588b717 100644
> --- a/drivers/accel/qaic/qaic.h
> +++ b/drivers/accel/qaic/qaic.h
> @@ -191,8 +191,6 @@ struct qaic_bo {
>  	u32			nr_slice;
>  	/* Number of slice that have been transferred by DMA engine */
>  	u32			nr_slice_xfer_done;
> -	/* true = BO is queued for execution, true = BO is not queued */
> -	bool			queued;
>  	/*
>  	 * If true then user has attached slicing information to this BO by
>  	 * calling DRM_IOCTL_QAIC_ATTACH_SLICE_BO ioctl.
> diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
> index 0c6f1328df68..89ab8fa19315 100644
> --- a/drivers/accel/qaic/qaic_data.c
> +++ b/drivers/accel/qaic/qaic_data.c
> @@ -141,6 +141,11 @@ struct dbc_rsp {
>  	__le16	status;
>  } __packed;
>  
> +static inline bool bo_queued(struct qaic_bo *bo)
> +{
> +	return !list_empty(&bo->xfer_list);
> +}
> +
>  inline int get_dbc_req_elem_size(void)
>  {
>  	return sizeof(struct dbc_req);
> @@ -648,6 +653,7 @@ static void qaic_init_bo(struct qaic_bo *bo, bool reinit)
>  	}
>  	complete_all(&bo->xfer_done);
>  	INIT_LIST_HEAD(&bo->slices);
> +	INIT_LIST_HEAD(&bo->xfer_list);
>  }
>  
>  static struct qaic_bo *qaic_alloc_init_bo(void)
> @@ -1166,7 +1172,6 @@ static int send_bo_list_to_device(struct qaic_device *qdev, struct drm_file *fil
>  	struct bo_slice *slice;
>  	unsigned long flags;
>  	struct qaic_bo *bo;
> -	bool queued;
>  	int i, j;
>  	int ret;
>  
> @@ -1198,9 +1203,7 @@ static int send_bo_list_to_device(struct qaic_device *qdev, struct drm_file *fil
>  		}
>  
>  		spin_lock_irqsave(&dbc->xfer_lock, flags);
> -		queued = bo->queued;
> -		bo->queued = true;
> -		if (queued) {
> +		if (bo_queued(bo)) {
>  			spin_unlock_irqrestore(&dbc->xfer_lock, flags);
>  			ret = -EINVAL;
>  			goto unlock_bo;
> @@ -1223,7 +1226,6 @@ static int send_bo_list_to_device(struct qaic_device *qdev, struct drm_file *fil
>  			else
>  				ret = copy_exec_reqs(qdev, slice, dbc->id, head, tail);
>  			if (ret) {
> -				bo->queued = false;
>  				spin_unlock_irqrestore(&dbc->xfer_lock, flags);
>  				goto unlock_bo;
>  			}
> @@ -1246,8 +1248,7 @@ static int send_bo_list_to_device(struct qaic_device *qdev, struct drm_file *fil
>  		spin_lock_irqsave(&dbc->xfer_lock, flags);
>  		bo = list_last_entry(&dbc->xfer_list, struct qaic_bo, xfer_list);
>  		obj = &bo->base;
> -		bo->queued = false;
> -		list_del(&bo->xfer_list);
> +		list_del_init(&bo->xfer_list);
>  		spin_unlock_irqrestore(&dbc->xfer_lock, flags);
>  		dma_sync_sgtable_for_cpu(&qdev->pdev->dev, bo->sgt, bo->dir);
>  		drm_gem_object_put(obj);
> @@ -1608,8 +1609,7 @@ irqreturn_t dbc_irq_threaded_fn(int irq, void *data)
>  			 */
>  			dma_sync_sgtable_for_cpu(&qdev->pdev->dev, bo->sgt, bo->dir);
>  			bo->nr_slice_xfer_done = 0;
> -			bo->queued = false;
> -			list_del(&bo->xfer_list);
> +			list_del_init(&bo->xfer_list);
>  			bo->perf_stats.req_processed_ts = ktime_get_ns();
>  			complete_all(&bo->xfer_done);
>  			drm_gem_object_put(&bo->base);
> @@ -1868,7 +1868,7 @@ int qaic_detach_slice_bo_ioctl(struct drm_device *dev, void *data, struct drm_fi
>  
>  	/* Check if BO is committed to H/W for DMA */
>  	spin_lock_irqsave(&dbc->xfer_lock, flags);
> -	if (bo->queued) {
> +	if (bo_queued(bo)) {
>  		spin_unlock_irqrestore(&dbc->xfer_lock, flags);
>  		ret = -EBUSY;
>  		goto unlock_ch_srcu;
> @@ -1898,8 +1898,7 @@ static void empty_xfer_list(struct qaic_device *qdev, struct dma_bridge_chan *db
>  	spin_lock_irqsave(&dbc->xfer_lock, flags);
>  	while (!list_empty(&dbc->xfer_list)) {
>  		bo = list_first_entry(&dbc->xfer_list, typeof(*bo), xfer_list);
> -		bo->queued = false;
> -		list_del(&bo->xfer_list);
> +		list_del_init(&bo->xfer_list);
>  		spin_unlock_irqrestore(&dbc->xfer_lock, flags);
>  		bo->nr_slice_xfer_done = 0;
>  		bo->req_id = 0;

[PATCH 3/7] accel/qaic: Fix MHI channel struct field order

[Jeffrey Hugo]

The timesync channels have their struct fields out of order with the rest
of the channels. Fix them so there is a consistent style in the file.

Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
---
 drivers/accel/qaic/mhi_controller.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/qaic/mhi_controller.c b/drivers/accel/qaic/mhi_controller.c
index 832464f2833a..364eede0ac02 100644
--- a/drivers/accel/qaic/mhi_controller.c
+++ b/drivers/accel/qaic/mhi_controller.c
@@ -358,8 +358,8 @@ static struct mhi_channel_config aic100_channels[] = {
 		.wake_capable = false,
 	},
 	{
-		.num = 21,
 		.name = "QAIC_TIMESYNC",
+		.num = 21,
 		.num_elements = 32,
 		.local_elements = 0,
 		.event_ring = 0,
@@ -390,8 +390,8 @@ static struct mhi_channel_config aic100_channels[] = {
 		.wake_capable = false,
 	},
 	{
-		.num = 23,
 		.name = "QAIC_TIMESYNC_PERIODIC",
+		.num = 23,
 		.num_elements = 32,
 		.local_elements = 0,
 		.event_ring = 0,
-- 
2.34.1

Re: [PATCH 3/7] accel/qaic: Fix MHI channel struct field order

[Jacek Lawrynowicz]

Reviewed-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>

On 08.12.2023 17:34, Jeffrey Hugo wrote:
> The timesync channels have their struct fields out of order with the rest
> of the channels. Fix them so there is a consistent style in the file.
> 
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
> ---
>  drivers/accel/qaic/mhi_controller.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/accel/qaic/mhi_controller.c b/drivers/accel/qaic/mhi_controller.c
> index 832464f2833a..364eede0ac02 100644
> --- a/drivers/accel/qaic/mhi_controller.c
> +++ b/drivers/accel/qaic/mhi_controller.c
> @@ -358,8 +358,8 @@ static struct mhi_channel_config aic100_channels[] = {
>  		.wake_capable = false,
>  	},
>  	{
> -		.num = 21,
>  		.name = "QAIC_TIMESYNC",
> +		.num = 21,
>  		.num_elements = 32,
>  		.local_elements = 0,
>  		.event_ring = 0,
> @@ -390,8 +390,8 @@ static struct mhi_channel_config aic100_channels[] = {
>  		.wake_capable = false,
>  	},
>  	{
> -		.num = 23,
>  		.name = "QAIC_TIMESYNC_PERIODIC",
> +		.num = 23,
>  		.num_elements = 32,
>  		.local_elements = 0,
>  		.event_ring = 0,

[PATCH 4/7] accel/qaic: Drop the reference to BO in error path of create BO IOCTL

[Jeffrey Hugo]

From: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>

Do not free BO explicitly in error path, just drop its reference, cleanup
will be taken care by DRM as we have registered for ->free() callback.
This patch makes sure that there is only one code path for BO to be freed.

Signed-off-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
---
 drivers/accel/qaic/qaic_data.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
index 89ab8fa19315..7faa00705c1d 100644
--- a/drivers/accel/qaic/qaic_data.c
+++ b/drivers/accel/qaic/qaic_data.c
@@ -574,6 +574,9 @@ static void qaic_free_sgt(struct sg_table *sgt)
 {
 	struct scatterlist *sg;
 
+	if (!sgt)
+		return;
+
 	for (sg = sgt->sgl; sg; sg = sg_next(sg))
 		if (sg_page(sg))
 			__free_pages(sg_page(sg), get_order(sg->length));
@@ -717,7 +720,7 @@ int qaic_create_bo_ioctl(struct drm_device *dev, void *data, struct drm_file *fi
 
 	ret = drm_gem_handle_create(file_priv, obj, &args->handle);
 	if (ret)
-		goto free_sgt;
+		goto free_bo;
 
 	bo->handle = args->handle;
 	drm_gem_object_put(obj);
@@ -726,10 +729,8 @@ int qaic_create_bo_ioctl(struct drm_device *dev, void *data, struct drm_file *fi
 
 	return 0;
 
-free_sgt:
-	qaic_free_sgt(bo->sgt);
 free_bo:
-	kfree(bo);
+	drm_gem_object_put(obj);
 unlock_dev_srcu:
 	srcu_read_unlock(&qdev->dev_lock, qdev_rcu_id);
 unlock_usr_srcu:
-- 
2.34.1

Re: [PATCH 4/7] accel/qaic: Drop the reference to BO in error path of create BO IOCTL

[Jacek Lawrynowicz]

Reviewed-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>

On 08.12.2023 17:34, Jeffrey Hugo wrote:
> From: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
> 
> Do not free BO explicitly in error path, just drop its reference, cleanup
> will be taken care by DRM as we have registered for ->free() callback.
> This patch makes sure that there is only one code path for BO to be freed.
> 
> Signed-off-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
> Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> ---
>  drivers/accel/qaic/qaic_data.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
> index 89ab8fa19315..7faa00705c1d 100644
> --- a/drivers/accel/qaic/qaic_data.c
> +++ b/drivers/accel/qaic/qaic_data.c
> @@ -574,6 +574,9 @@ static void qaic_free_sgt(struct sg_table *sgt)
>  {
>  	struct scatterlist *sg;
>  
> +	if (!sgt)
> +		return;
> +
>  	for (sg = sgt->sgl; sg; sg = sg_next(sg))
>  		if (sg_page(sg))
>  			__free_pages(sg_page(sg), get_order(sg->length));
> @@ -717,7 +720,7 @@ int qaic_create_bo_ioctl(struct drm_device *dev, void *data, struct drm_file *fi
>  
>  	ret = drm_gem_handle_create(file_priv, obj, &args->handle);
>  	if (ret)
> -		goto free_sgt;
> +		goto free_bo;
>  
>  	bo->handle = args->handle;
>  	drm_gem_object_put(obj);
> @@ -726,10 +729,8 @@ int qaic_create_bo_ioctl(struct drm_device *dev, void *data, struct drm_file *fi
>  
>  	return 0;
>  
> -free_sgt:
> -	qaic_free_sgt(bo->sgt);
>  free_bo:
> -	kfree(bo);
> +	drm_gem_object_put(obj);
>  unlock_dev_srcu:
>  	srcu_read_unlock(&qdev->dev_lock, qdev_rcu_id);
>  unlock_usr_srcu:

[PATCH 5/7] accel/qaic: Call drm_gem_create_mmap_offset() once for each BO

[Jeffrey Hugo]

From: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>

Every time QAIC_MMAP_BO ioctl is called for a BO,
drm_gem_create_mmap_offset() is called. Calling
drm_gem_create_mmap_offset() more then once for a BO seems redundant.

Signed-off-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
---
 drivers/accel/qaic/qaic_data.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
index 7faa00705c1d..f88d925c8001 100644
--- a/drivers/accel/qaic/qaic_data.c
+++ b/drivers/accel/qaic/qaic_data.c
@@ -718,6 +718,10 @@ int qaic_create_bo_ioctl(struct drm_device *dev, void *data, struct drm_file *fi
 	if (ret)
 		goto free_bo;
 
+	ret = drm_gem_create_mmap_offset(obj);
+	if (ret)
+		goto free_bo;
+
 	ret = drm_gem_handle_create(file_priv, obj, &args->handle);
 	if (ret)
 		goto free_bo;
@@ -745,7 +749,7 @@ int qaic_mmap_bo_ioctl(struct drm_device *dev, void *data, struct drm_file *file
 	struct drm_gem_object *obj;
 	struct qaic_device *qdev;
 	struct qaic_user *usr;
-	int ret;
+	int ret = 0;
 
 	usr = file_priv->driver_priv;
 	usr_rcu_id = srcu_read_lock(&usr->qddev_lock);
@@ -767,9 +771,7 @@ int qaic_mmap_bo_ioctl(struct drm_device *dev, void *data, struct drm_file *file
 		goto unlock_dev_srcu;
 	}
 
-	ret = drm_gem_create_mmap_offset(obj);
-	if (ret == 0)
-		args->offset = drm_vma_node_offset_addr(&obj->vma_node);
+	args->offset = drm_vma_node_offset_addr(&obj->vma_node);
 
 	drm_gem_object_put(obj);
 
-- 
2.34.1

Re: [PATCH 5/7] accel/qaic: Call drm_gem_create_mmap_offset() once for each BO

[Jacek Lawrynowicz]

Reviewed-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>

On 08.12.2023 17:34, Jeffrey Hugo wrote:
> From: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
> 
> Every time QAIC_MMAP_BO ioctl is called for a BO,
> drm_gem_create_mmap_offset() is called. Calling
> drm_gem_create_mmap_offset() more then once for a BO seems redundant.
> 
> Signed-off-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> ---
>  drivers/accel/qaic/qaic_data.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
> index 7faa00705c1d..f88d925c8001 100644
> --- a/drivers/accel/qaic/qaic_data.c
> +++ b/drivers/accel/qaic/qaic_data.c
> @@ -718,6 +718,10 @@ int qaic_create_bo_ioctl(struct drm_device *dev, void *data, struct drm_file *fi
>  	if (ret)
>  		goto free_bo;
>  
> +	ret = drm_gem_create_mmap_offset(obj);
> +	if (ret)
> +		goto free_bo;
> +
>  	ret = drm_gem_handle_create(file_priv, obj, &args->handle);
>  	if (ret)
>  		goto free_bo;
> @@ -745,7 +749,7 @@ int qaic_mmap_bo_ioctl(struct drm_device *dev, void *data, struct drm_file *file
>  	struct drm_gem_object *obj;
>  	struct qaic_device *qdev;
>  	struct qaic_user *usr;
> -	int ret;
> +	int ret = 0;
>  
>  	usr = file_priv->driver_priv;
>  	usr_rcu_id = srcu_read_lock(&usr->qddev_lock);
> @@ -767,9 +771,7 @@ int qaic_mmap_bo_ioctl(struct drm_device *dev, void *data, struct drm_file *file
>  		goto unlock_dev_srcu;
>  	}
>  
> -	ret = drm_gem_create_mmap_offset(obj);
> -	if (ret == 0)
> -		args->offset = drm_vma_node_offset_addr(&obj->vma_node);
> +	args->offset = drm_vma_node_offset_addr(&obj->vma_node);
>  
>  	drm_gem_object_put(obj);
>  

[PATCH 6/7] accel/qaic: Leverage DRM managed APIs to release resources

[Jeffrey Hugo]

From: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>

Offload the balancing of init and destroy calls to DRM managed APIs.
mutex destroy for ->cntl_mutex is not called during device release and
destroy workqueue is not called in error path of create_qdev(). So, use
DRM managed APIs to manage the release of resources and avoid such
problems.

Signed-off-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
---
 drivers/accel/qaic/qaic.h     |   1 +
 drivers/accel/qaic/qaic_drv.c | 138 ++++++++++++++++++++++------------
 2 files changed, 89 insertions(+), 50 deletions(-)

diff --git a/drivers/accel/qaic/qaic.h b/drivers/accel/qaic/qaic.h
index 2b3ef588b717..9256653b3036 100644
--- a/drivers/accel/qaic/qaic.h
+++ b/drivers/accel/qaic/qaic.h
@@ -30,6 +30,7 @@
 #define to_qaic_drm_device(dev) container_of(dev, struct qaic_drm_device, drm)
 #define to_drm(qddev) (&(qddev)->drm)
 #define to_accel_kdev(qddev) (to_drm(qddev)->accel->kdev) /* Return Linux device of accel node */
+#define to_qaic_device(dev) (to_qaic_drm_device((dev))->qdev)
 
 enum __packed dev_states {
 	/* Device is offline or will be very soon */
diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
index 2a313eb69b12..10a43d02844f 100644
--- a/drivers/accel/qaic/qaic_drv.c
+++ b/drivers/accel/qaic/qaic_drv.c
@@ -44,6 +44,53 @@ MODULE_PARM_DESC(datapath_polling, "Operate the datapath in polling mode");
 static bool link_up;
 static DEFINE_IDA(qaic_usrs);
 
+static void qaicm_wq_release(struct drm_device *dev, void *res)
+{
+	struct workqueue_struct *wq = res;
+
+	destroy_workqueue(wq);
+}
+
+static struct workqueue_struct *qaicm_wq_init(struct drm_device *dev, const char *fmt)
+{
+	struct workqueue_struct *wq;
+	int ret;
+
+	wq = alloc_workqueue(fmt, WQ_UNBOUND, 0);
+	if (!wq)
+		return ERR_PTR(-ENOMEM);
+	ret = drmm_add_action_or_reset(dev, qaicm_wq_release, wq);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return wq;
+}
+
+static void qaicm_srcu_release(struct drm_device *dev, void *res)
+{
+	struct srcu_struct *lock = res;
+
+	cleanup_srcu_struct(lock);
+}
+
+static int qaicm_srcu_init(struct drm_device *dev, struct srcu_struct *lock)
+{
+	int ret;
+
+	ret = init_srcu_struct(lock);
+	if (ret)
+		return ret;
+
+	return drmm_add_action_or_reset(dev, qaicm_srcu_release, lock);
+}
+
+static void qaicm_pci_release(struct drm_device *dev, void *res)
+{
+	struct qaic_device *qdev = to_qaic_device(dev);
+
+	pci_set_drvdata(qdev->pdev, NULL);
+}
+
 static void free_usr(struct kref *kref)
 {
 	struct qaic_user *usr = container_of(kref, struct qaic_user, ref_count);
@@ -299,74 +346,73 @@ void qaic_dev_reset_clean_local_state(struct qaic_device *qdev)
 		release_dbc(qdev, i);
 }
 
-static void cleanup_qdev(struct qaic_device *qdev)
-{
-	int i;
-
-	for (i = 0; i < qdev->num_dbc; ++i)
-		cleanup_srcu_struct(&qdev->dbc[i].ch_lock);
-	cleanup_srcu_struct(&qdev->dev_lock);
-	pci_set_drvdata(qdev->pdev, NULL);
-	destroy_workqueue(qdev->cntl_wq);
-	destroy_workqueue(qdev->qts_wq);
-}
-
 static struct qaic_device *create_qdev(struct pci_dev *pdev, const struct pci_device_id *id)
 {
+	struct device *dev = &pdev->dev;
 	struct qaic_drm_device *qddev;
 	struct qaic_device *qdev;
-	int i;
+	struct drm_device *drm;
+	int i, ret;
 
-	qdev = devm_kzalloc(&pdev->dev, sizeof(*qdev), GFP_KERNEL);
+	qdev = devm_kzalloc(dev, sizeof(*qdev), GFP_KERNEL);
 	if (!qdev)
 		return NULL;
 
 	qdev->dev_state = QAIC_OFFLINE;
 	if (id->device == PCI_DEV_AIC100) {
 		qdev->num_dbc = 16;
-		qdev->dbc = devm_kcalloc(&pdev->dev, qdev->num_dbc, sizeof(*qdev->dbc), GFP_KERNEL);
+		qdev->dbc = devm_kcalloc(dev, qdev->num_dbc, sizeof(*qdev->dbc), GFP_KERNEL);
 		if (!qdev->dbc)
 			return NULL;
 	}
 
-	qdev->cntl_wq = alloc_workqueue("qaic_cntl", WQ_UNBOUND, 0);
-	if (!qdev->cntl_wq)
+	qddev = devm_drm_dev_alloc(&pdev->dev, &qaic_accel_driver, struct qaic_drm_device, drm);
+	if (IS_ERR(qddev))
+		return NULL;
+
+	drm = to_drm(qddev);
+	pci_set_drvdata(pdev, qdev);
+
+	ret = drmm_mutex_init(drm, &qddev->users_mutex);
+	if (ret)
+		return NULL;
+	ret = drmm_add_action_or_reset(drm, qaicm_pci_release, NULL);
+	if (ret)
+		return NULL;
+	ret = drmm_mutex_init(drm, &qdev->cntl_mutex);
+	if (ret)
 		return NULL;
 
-	qdev->qts_wq = alloc_workqueue("qaic_ts", WQ_UNBOUND, 0);
-	if (!qdev->qts_wq) {
-		destroy_workqueue(qdev->cntl_wq);
+	qdev->cntl_wq = qaicm_wq_init(drm, "qaic_cntl");
+	if (IS_ERR(qdev->cntl_wq))
+		return NULL;
+	qdev->qts_wq = qaicm_wq_init(drm, "qaic_ts");
+	if (IS_ERR(qdev->qts_wq))
 		return NULL;
-	}
 
-	pci_set_drvdata(pdev, qdev);
+	ret = qaicm_srcu_init(drm, &qdev->dev_lock);
+	if (ret)
+		return NULL;
+
+	qdev->qddev = qddev;
 	qdev->pdev = pdev;
+	qddev->qdev = qdev;
 
-	mutex_init(&qdev->cntl_mutex);
 	INIT_LIST_HEAD(&qdev->cntl_xfer_list);
-	init_srcu_struct(&qdev->dev_lock);
+	INIT_LIST_HEAD(&qddev->users);
 
 	for (i = 0; i < qdev->num_dbc; ++i) {
 		spin_lock_init(&qdev->dbc[i].xfer_lock);
 		qdev->dbc[i].qdev = qdev;
 		qdev->dbc[i].id = i;
 		INIT_LIST_HEAD(&qdev->dbc[i].xfer_list);
-		init_srcu_struct(&qdev->dbc[i].ch_lock);
+		ret = qaicm_srcu_init(drm, &qdev->dbc[i].ch_lock);
+		if (ret)
+			return NULL;
 		init_waitqueue_head(&qdev->dbc[i].dbc_release);
 		INIT_LIST_HEAD(&qdev->dbc[i].bo_lists);
 	}
 
-	qddev = devm_drm_dev_alloc(&pdev->dev, &qaic_accel_driver, struct qaic_drm_device, drm);
-	if (IS_ERR(qddev)) {
-		cleanup_qdev(qdev);
-		return NULL;
-	}
-
-	drmm_mutex_init(to_drm(qddev), &qddev->users_mutex);
-	INIT_LIST_HEAD(&qddev->users);
-	qddev->qdev = qdev;
-	qdev->qddev = qddev;
-
 	return qdev;
 }
 
@@ -472,35 +518,28 @@ static int qaic_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	ret = init_pci(qdev, pdev);
 	if (ret)
-		goto cleanup_qdev;
+		return ret;
 
 	for (i = 0; i < qdev->num_dbc; ++i)
 		qdev->dbc[i].dbc_base = qdev->bar_2 + QAIC_DBC_OFF(i);
 
 	mhi_irq = init_msi(qdev, pdev);
-	if (mhi_irq < 0) {
-		ret = mhi_irq;
-		goto cleanup_qdev;
-	}
+	if (mhi_irq < 0)
+		return mhi_irq;
 
 	ret = qaic_create_drm_device(qdev, QAIC_NO_PARTITION);
 	if (ret)
-		goto cleanup_qdev;
+		return ret;
 
 	qdev->mhi_cntrl = qaic_mhi_register_controller(pdev, qdev->bar_0, mhi_irq,
 						       qdev->single_msi);
 	if (IS_ERR(qdev->mhi_cntrl)) {
 		ret = PTR_ERR(qdev->mhi_cntrl);
-		goto cleanup_drm_dev;
+		qaic_destroy_drm_device(qdev, QAIC_NO_PARTITION);
+		return ret;
 	}
 
 	return 0;
-
-cleanup_drm_dev:
-	qaic_destroy_drm_device(qdev, QAIC_NO_PARTITION);
-cleanup_qdev:
-	cleanup_qdev(qdev);
-	return ret;
 }
 
 static void qaic_pci_remove(struct pci_dev *pdev)
@@ -513,7 +552,6 @@ static void qaic_pci_remove(struct pci_dev *pdev)
 	qaic_dev_reset_clean_local_state(qdev);
 	qaic_destroy_drm_device(qdev, QAIC_NO_PARTITION);
 	qaic_mhi_free_controller(qdev->mhi_cntrl, link_up);
-	cleanup_qdev(qdev);
 }
 
 static void qaic_pci_shutdown(struct pci_dev *pdev)
-- 
2.34.1

Re: [PATCH 6/7] accel/qaic: Leverage DRM managed APIs to release resources

[Jeffrey Hugo]

On 12/8/2023 9:34 AM, Jeffrey Hugo wrote:
> From: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
> 
> Offload the balancing of init and destroy calls to DRM managed APIs.
> mutex destroy for ->cntl_mutex is not called during device release and
> destroy workqueue is not called in error path of create_qdev(). So, use
> DRM managed APIs to manage the release of resources and avoid such
> problems.
> 
> Signed-off-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

Jacek, I saw a review from you for 1-5, and 7 but not this patch (6). 
Did I miss something?

-Jeff

Re: [PATCH 6/7] accel/qaic: Leverage DRM managed APIs to release resources

[Jacek Lawrynowicz]

On 15.12.2023 19:06, Jeffrey Hugo wrote:
> On 12/8/2023 9:34 AM, Jeffrey Hugo wrote:
>> From: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
>>
>> Offload the balancing of init and destroy calls to DRM managed APIs.
>> mutex destroy for ->cntl_mutex is not called during device release and
>> destroy workqueue is not called in error path of create_qdev(). So, use
>> DRM managed APIs to manage the release of resources and avoid such
>> problems.
>>
>> Signed-off-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
>> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
>> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> 
> Jacek, I saw a review from you for 1-5, and 7 but not this patch (6). Did I miss something?

Sorry, I was out of office for a couple days and I wasn't able to finish the review.

Reviewed-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>

Re: [PATCH 6/7] accel/qaic: Leverage DRM managed APIs to release resources

[Jeffrey Hugo]

On 12/20/2023 12:02 AM, Jacek Lawrynowicz wrote:
> On 15.12.2023 19:06, Jeffrey Hugo wrote:
>> On 12/8/2023 9:34 AM, Jeffrey Hugo wrote:
>>> From: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
>>>
>>> Offload the balancing of init and destroy calls to DRM managed APIs.
>>> mutex destroy for ->cntl_mutex is not called during device release and
>>> destroy workqueue is not called in error path of create_qdev(). So, use
>>> DRM managed APIs to manage the release of resources and avoid such
>>> problems.
>>>
>>> Signed-off-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
>>> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
>>> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
>>
>> Jacek, I saw a review from you for 1-5, and 7 but not this patch (6). Did I miss something?
> 
> Sorry, I was out of office for a couple days and I wasn't able to finish the review.
> 
> Reviewed-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
> 

No problem.  I thought maybe I had an issue on my end.  I appreciate the 
reviews.  Hopefully your out of office was enjoyable.

Happy Holidays.

-Jeff

[PATCH 7/7] accel/qaic: Order pci_remove() operations in reverse of probe()

[Jeffrey Hugo]

In probe() we create the drm_device, and then register the MHI controller.
In remove(), we should unregister the controller first, then remove the
drm_device.  Update the remove() operations to match.

Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
---
 drivers/accel/qaic/qaic_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
index 10a43d02844f..d1a632dbaec6 100644
--- a/drivers/accel/qaic/qaic_drv.c
+++ b/drivers/accel/qaic/qaic_drv.c
@@ -550,8 +550,8 @@ static void qaic_pci_remove(struct pci_dev *pdev)
 		return;
 
 	qaic_dev_reset_clean_local_state(qdev);
-	qaic_destroy_drm_device(qdev, QAIC_NO_PARTITION);
 	qaic_mhi_free_controller(qdev->mhi_cntrl, link_up);
+	qaic_destroy_drm_device(qdev, QAIC_NO_PARTITION);
 }
 
 static void qaic_pci_shutdown(struct pci_dev *pdev)
-- 
2.34.1

Re: [PATCH 7/7] accel/qaic: Order pci_remove() operations in reverse of probe()

[Jacek Lawrynowicz]

Reviewed-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>

On 08.12.2023 17:34, Jeffrey Hugo wrote:
> In probe() we create the drm_device, and then register the MHI controller.
> In remove(), we should unregister the controller first, then remove the
> drm_device.  Update the remove() operations to match.
> 
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
> Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
> ---
>  drivers/accel/qaic/qaic_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
> index 10a43d02844f..d1a632dbaec6 100644
> --- a/drivers/accel/qaic/qaic_drv.c
> +++ b/drivers/accel/qaic/qaic_drv.c
> @@ -550,8 +550,8 @@ static void qaic_pci_remove(struct pci_dev *pdev)
>  		return;
>  
>  	qaic_dev_reset_clean_local_state(qdev);
> -	qaic_destroy_drm_device(qdev, QAIC_NO_PARTITION);
>  	qaic_mhi_free_controller(qdev->mhi_cntrl, link_up);
> +	qaic_destroy_drm_device(qdev, QAIC_NO_PARTITION);
>  }
>  
>  static void qaic_pci_shutdown(struct pci_dev *pdev)

Re: [PATCH 0/7] qaic cleanups for 6.8

[Jeffrey Hugo]

On 12/8/2023 9:34 AM, Jeffrey Hugo wrote:
> A set of cleanups to the driver to improve error cases and reduce some
> code duplication.
> 
> Jeffrey Hugo (2):
>    accel/qaic: Fix MHI channel struct field order
>    accel/qaic: Order pci_remove() operations in reverse of probe()
> 
> Pranjal Ramajor Asha Kanojiya (5):
>    accel/qaic: Deprecate ->size field from attach slice IOCTL structure
>    accel/qaic: Remove bo->queued field
>    accel/qaic: Drop the reference to BO in error path of create BO IOCTL
>    accel/qaic: Call drm_gem_create_mmap_offset() once for each BO
>    accel/qaic: Leverage DRM managed APIs to release resources
> 
>   drivers/accel/qaic/mhi_controller.c |   4 +-
>   drivers/accel/qaic/qaic.h           |   3 +-
>   drivers/accel/qaic/qaic_data.c      |  59 ++++++------
>   drivers/accel/qaic/qaic_drv.c       | 140 ++++++++++++++++++----------
>   include/uapi/drm/qaic_accel.h       |  13 +--
>   5 files changed, 119 insertions(+), 100 deletions(-)
> 

1-5 pushed to drm-misc-next

-Jeff

Re: [PATCH 0/7] qaic cleanups for 6.8

[Jeffrey Hugo]

On 12/15/2023 11:05 AM, Jeffrey Hugo wrote:
> On 12/8/2023 9:34 AM, Jeffrey Hugo wrote:
>> A set of cleanups to the driver to improve error cases and reduce some
>> code duplication.
>>
>> Jeffrey Hugo (2):
>>    accel/qaic: Fix MHI channel struct field order
>>    accel/qaic: Order pci_remove() operations in reverse of probe()
>>
>> Pranjal Ramajor Asha Kanojiya (5):
>>    accel/qaic: Deprecate ->size field from attach slice IOCTL structure
>>    accel/qaic: Remove bo->queued field
>>    accel/qaic: Drop the reference to BO in error path of create BO IOCTL
>>    accel/qaic: Call drm_gem_create_mmap_offset() once for each BO
>>    accel/qaic: Leverage DRM managed APIs to release resources
>>
>>   drivers/accel/qaic/mhi_controller.c |   4 +-
>>   drivers/accel/qaic/qaic.h           |   3 +-
>>   drivers/accel/qaic/qaic_data.c      |  59 ++++++------
>>   drivers/accel/qaic/qaic_drv.c       | 140 ++++++++++++++++++----------
>>   include/uapi/drm/qaic_accel.h       |  13 +--
>>   5 files changed, 119 insertions(+), 100 deletions(-)
>>
> 
> 1-5 pushed to drm-misc-next
> 
> -Jeff

6 and 7 pushed to drm-misc-next

-Jeff