[PATCH] drm/imagination: Remove duplicated CCB control initialisation

[PATCH] drm/imagination: Remove duplicated CCB control initialisation

[Alexandru Dadu]

From: Alessio Belle <alessio.belle@imgtec.com>

The same is already done by the ccb_ctrl_init() callback.

Signed-off-by: Alessio Belle <alessio.belle@imgtec.com>
---
Signed-off-by: Alexandru Dadu <alexandru.dadu@imgtec.com>
---
 drivers/gpu/drm/imagination/pvr_ccb.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_ccb.c b/drivers/gpu/drm/imagination/pvr_ccb.c
index 4accf18e2341..48672a86ca2f 100644
--- a/drivers/gpu/drm/imagination/pvr_ccb.c
+++ b/drivers/gpu/drm/imagination/pvr_ccb.c
@@ -82,11 +82,6 @@ pvr_ccb_init(struct pvr_device *pvr_dev, struct pvr_ccb *pvr_ccb,
 	pvr_fw_object_get_fw_addr(pvr_ccb->ctrl_obj, &pvr_ccb->ctrl_fw_addr);
 	pvr_fw_object_get_fw_addr(pvr_ccb->ccb_obj, &pvr_ccb->ccb_fw_addr);
 
-	WRITE_ONCE(pvr_ccb->ctrl->write_offset, 0);
-	WRITE_ONCE(pvr_ccb->ctrl->read_offset, 0);
-	WRITE_ONCE(pvr_ccb->ctrl->wrap_mask, num_cmds - 1);
-	WRITE_ONCE(pvr_ccb->ctrl->cmd_size, cmd_size);
-
 	return 0;
 
 err_free_ctrl:

---
base-commit: e55fead22ff9ee047ab9f1903860c4b43043514e
change-id: 20260810-b4-upstream-remove-duplicate-ccb-control-init-ed10e641d81b

Best regards,
--  
Alexandru Dadu <alexandru.dadu@imgtec.com>

Re: [PATCH] drm/imagination: Remove duplicated CCB control initialisation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Pre-existing NULL pointer dereference in `pvr_fw_object_destroy()` when handling allocation failures.
- [High] Unprotected accesses to `free_list->current_pages` and `free_list->ready_pages` in `pvr_free_list_process_grow_req`.
- [High] Firmware commands in the FWCCB can be processed out of order due to lock dropping in `pvr_fwccb_process`.
--

--- Patch [1]: [PATCH] drm/imagination: Remove duplicated CCB control initialisation ---
commit f2cf0d89b43f414e7db3141318ab274dbe8aa132
Author: Alessio Belle <alessio.belle@imgtec.com>

drm/imagination: Remove duplicated CCB control initialisation

The same is already done by the ccb_ctrl_init() callback.

> diff --git a/drivers/gpu/drm/imagination/pvr_ccb.c b/drivers/gpu/drm/imagination/pvr_ccb.c
> index 4accf18e23418..48672a86ca2f5 100644
> --- a/drivers/gpu/drm/imagination/pvr_ccb.c
> +++ b/drivers/gpu/drm/imagination/pvr_ccb.c
> @@ -82,11 +82,6 @@ pvr_ccb_init(struct pvr_device *pvr_dev, struct pvr_ccb *pvr_ccb,
>  	pvr_fw_object_get_fw_addr(pvr_ccb->ctrl_obj, &pvr_ccb->ctrl_fw_addr);
>  	pvr_fw_object_get_fw_addr(pvr_ccb->ccb_obj, &pvr_ccb->ccb_fw_addr);
>  
> -	WRITE_ONCE(pvr_ccb->ctrl->write_offset, 0);
> -	WRITE_ONCE(pvr_ccb->ctrl->read_offset, 0);
> -	WRITE_ONCE(pvr_ccb->ctrl->wrap_mask, num_cmds - 1);
> -	WRITE_ONCE(pvr_ccb->ctrl->cmd_size, cmd_size);
> -
>  	return 0;
>  
>  err_free_ctrl:

This isn't a bug introduced by this patch, but I noticed a potential NULL
pointer dereference in pvr_fw_object_destroy() when handling allocation
failures.

If pvr_gem_object_create() fails in pvr_fw_object_create_and_map_common(),
fw_obj->gem is explicitly set to NULL before jumping to the error cleanup
path:

drivers/gpu/drm/imagination/pvr_fw.c:pvr_fw_object_create_and_map_common() {
	...
	fw_obj->gem = pvr_gem_object_create(pvr_dev, size, flags);
	if (IS_ERR(fw_obj->gem)) {
		err = PTR_ERR(fw_obj->gem);
		fw_obj->gem = NULL;
		goto err_put_object;
	}
	...
err_put_object:
	pvr_fw_object_destroy(fw_obj);

	return ERR_PTR(err);
}

The cleanup path then calls pvr_fw_object_destroy():

drivers/gpu/drm/imagination/pvr_fw.c:pvr_fw_object_destroy() {
	struct pvr_gem_object *pvr_obj = fw_obj->gem;
	struct drm_gem_object *gem_obj = gem_from_pvr_gem(pvr_obj);
	struct pvr_device *pvr_dev = to_pvr_device(gem_obj->dev);

[Severity: High]
Will this unconditionally dereference the NULL gem_obj (derived from the NULL
fw_obj->gem) to get the device, causing a kernel panic?


This is a pre-existing issue, but there appears to be an unprotected access
to the free list page accounting variables in
pvr_free_list_process_grow_req().

drivers/gpu/drm/imagination/pvr_free_list.c:pvr_free_list_process_grow_req() {
	...
	/* Since the FW made the request, it has already consumed the ready pages,
	 * update the host struct.
	 */
	free_list->current_pages += free_list->ready_pages;
	free_list->ready_pages = 0;

	/* If the grow succeeds, update the grow_pages argument. */
	if (!pvr_free_list_grow(free_list, free_list->grow_pages))
		grow_pages = free_list->grow_pages;

	/* Now prepare the response and send it back to the FW. */
	pvr_fw_object_get_fw_addr(free_list->fw_obj, &resp->freelist_fw_addr);
	resp->delta_pages = grow_pages;
	resp->new_pages = free_list->current_pages + free_list->ready_pages;
	...
}

[Severity: High]
Since pvr_fwccb_process() can run concurrently (from the IRQ handler and
userspace-triggered free list release), can these unprotected modifications
and reads of free_list->current_pages and free_list->ready_pages lead to
data corruption in the free list page accounting?


This isn't a bug introduced by this patch, but I noticed that firmware commands
in the FWCCB might be processed out of order because the lock is dropped in
pvr_fwccb_process().

drivers/gpu/drm/imagination/pvr_ccb.c:pvr_fwccb_process() {
	...
	while ((read_offset = READ_ONCE(ctrl->read_offset)) != READ_ONCE(ctrl->write_offset)) {
		struct rogue_fwif_fwccb_cmd cmd = fwccb[read_offset];

		WRITE_ONCE(ctrl->read_offset, (read_offset + 1) & READ_ONCE(ctrl->wrap_mask));

		/* Drop FWCCB lock while we process command. */
		mutex_unlock(&pvr_dev->fwccb.lock);

		process_fwccb_command(pvr_dev, &cmd);

		mutex_lock(&pvr_dev->fwccb.lock);
	}
	...
}

[Severity: High]
Since this function can be called both from the IRQ thread and from the
userspace-triggered pvr_free_list_release() path, if one thread dequeues a
command and drops the lock, could a second thread acquire the lock, dequeue
the next command, and process it concurrently, leading to out-of-order
execution of firmware events?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810-b4-upstream-remove-duplicate-ccb-control-init-v1-1-4ea79b116b40@imgtec.com?part=1

Re: [PATCH] drm/imagination: Remove duplicated CCB control initialisation

[Brajesh Gupta]

On Mon, 2026-08-10 at 16:03 +0300, Alexandru Dadu wrote:
Hi Alexandru,
> From: Alessio Belle <alessio.belle@imgtec.com>
> 
> The same is already done by the ccb_ctrl_init() callback.
> 
> Signed-off-by: Alessio Belle <alessio.belle@imgtec.com>
> ---
> Signed-off-by: Alexandru Dadu <alexandru.dadu@imgtec.com>
> ---
>  drivers/gpu/drm/imagination/pvr_ccb.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_ccb.c b/drivers/gpu/drm/imagination/pvr_ccb.c
> index 4accf18e2341..48672a86ca2f 100644
> --- a/drivers/gpu/drm/imagination/pvr_ccb.c
> +++ b/drivers/gpu/drm/imagination/pvr_ccb.c
> @@ -82,11 +82,6 @@ pvr_ccb_init(struct pvr_device *pvr_dev, struct pvr_ccb *pvr_ccb,
>  	pvr_fw_object_get_fw_addr(pvr_ccb->ctrl_obj, &pvr_ccb->ctrl_fw_addr);
>  	pvr_fw_object_get_fw_addr(pvr_ccb->ccb_obj, &pvr_ccb->ccb_fw_addr);
>  
> -	WRITE_ONCE(pvr_ccb->ctrl->write_offset, 0);
> -	WRITE_ONCE(pvr_ccb->ctrl->read_offset, 0);
> -	WRITE_ONCE(pvr_ccb->ctrl->wrap_mask, num_cmds - 1);
> -	WRITE_ONCE(pvr_ccb->ctrl->cmd_size, cmd_size);
> -
>  	return 0;
>  
>  err_free_ctrl:
> 
> ---
> base-commit: e55fead22ff9ee047ab9f1903860c4b43043514e
> change-id: 20260810-b4-upstream-remove-duplicate-ccb-control-init-ed10e641d81b
> 
> Best regards,
> --  
> Alexandru Dadu <alexandru.dadu@imgtec.com>
> 

Reviewed by: Brajesh Gupta <brajesh.gupta@imgtec.com>

Thanks,
Brajesh