[PATCH] accel/rocket: number the cores by devicetree position, not bind order

[PATCH] accel/rocket: number the cores by devicetree position, not bind order

[Igor Paunovic]

rocket_job_hw_submit() programs the S_POINTER registers of a core with an
extra bit derived from core->index, the way the vendor driver derives it
from the hardware number of the core. rocket_probe() sets core->index to
the slot the core takes in rdev->cores[], which is the order the cores
bind in.

The two agree only while the cores that bind are a prefix of the core
nodes in the devicetree, in devicetree order. Unbind them and bind them
back with a different core first, have one core's probe deferred behind a
sibling's, or disable a core other than the last one, and every task
submitted to a core whose slot is not its hardware number times out after
500 ms. The reset that follows does not help, and the inference finishes
with wrong output.

Observed on an Orange Pi 5 Plus, over all six bind orders of the three
cores: only the devicetree order ran clean. The other five produced 27 to
140 "NPU job timed out" within a single six-second inference, with a
bit-exact oracle rejecting the output, or throughput falling from 130 to
1.95 inferences per second. The timeouts land on the cores whose slot is
not their hardware number, in proportion to the tasks the scheduler hands
them, and in both directions of the mismatch.

Number the cores by their position among the core nodes in the devicetree
instead, which is what the hardware number is.

The wrong value has been assigned since the driver was added, but it only
reached the hardware once the extra bit was introduced, hence the Fixes
tag below.

Fixes: 0810d5ad88a1 ("accel/rocket: Add job submission IOCTL")
Cc: stable@vger.kernel.org
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
Assisted-by: LLM sparse checkpatch
---
 drivers/accel/rocket/rocket_core.h |  5 +++++
 drivers/accel/rocket/rocket_drv.c  | 31 +++++++++++++++++++++++++++++-
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/rocket/rocket_core.h b/drivers/accel/rocket/rocket_core.h
index f6d7382854ca9..46ed8352a79d2 100644
--- a/drivers/accel/rocket/rocket_core.h
+++ b/drivers/accel/rocket/rocket_core.h
@@ -30,6 +30,11 @@
 struct rocket_core {
 	struct device *dev;
 	struct rocket_device *rdev;
+	/*
+	 * Hardware number of the core: its position among the core nodes in
+	 * the devicetree. Not an index into rdev->cores[] - that slot is what
+	 * find_core_for_dev() returns.
+	 */
 	unsigned int index;
 
 	int irq;
diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
index 8bbbce594883e..e2cde31443ffa 100644
--- a/drivers/accel/rocket/rocket_drv.c
+++ b/drivers/accel/rocket/rocket_drv.c
@@ -157,10 +157,39 @@ static const struct drm_driver rocket_drm_driver = {
 	.desc			= "rocket DRM",
 };
 
+/*
+ * The extra bit that rocket_job_hw_submit() sets in the S_POINTER registers
+ * is the hardware number of the core, which is its position among the core
+ * nodes in the devicetree: a disabled core keeps its number. The slot a core
+ * takes in rdev->cores[] is the order the cores happened to bind in, and the
+ * two only agree while the cores that bind are a prefix of those nodes, in
+ * devicetree order. Every task submitted to a core whose slot is not its
+ * hardware number then times out.
+ */
+static int rocket_core_hw_index(struct device *dev)
+{
+	struct device_node *np;
+	int index = 0;
+
+	for_each_matching_node(np, dev->driver->of_match_table) {
+		if (np == dev->of_node) {
+			of_node_put(np);
+			return index;
+		}
+		index++;
+	}
+
+	return -ENODEV;
+}
+
 static int rocket_probe(struct platform_device *pdev)
 {
+	int index = rocket_core_hw_index(&pdev->dev);
 	int ret;
 
+	if (index < 0)
+		return index;
+
 	if (rdev == NULL) {
 		/* First core probing, initialize DRM device. */
 		rdev = rocket_device_init(drm_dev, &rocket_drm_driver);
@@ -176,7 +205,7 @@ static int rocket_probe(struct platform_device *pdev)
 
 	rdev->cores[core].rdev = rdev;
 	rdev->cores[core].dev = &pdev->dev;
-	rdev->cores[core].index = core;
+	rdev->cores[core].index = index;
 
 	rdev->num_cores++;
 

base-commit: a9f09b5ea0c3db1e2d4c0f8d3ebdd612d8aa0366
-- 
2.43.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH] accel/rocket: number the cores by devicetree position, not bind order

[Igor Paunovic]

rocket_job_hw_submit() programs the S_POINTER registers of a core with an
extra bit derived from core->index, the way the vendor driver derives it
from the hardware number of the core. rocket_probe() sets core->index to
the slot the core takes in rdev->cores[], which is the order the cores
bind in.

The two agree only while the cores that bind are a prefix of the core
nodes in the devicetree, in devicetree order. Unbind them and bind them
back with a different core first, have one core's probe deferred behind a
sibling's, or disable a core other than the last one, and every task
submitted to a core whose slot is not its hardware number times out after
500 ms. The reset that follows does not help, and the inference finishes
with wrong output.

Observed on an Orange Pi 5 Plus, over all six bind orders of the three
cores: only the devicetree order ran clean. The other five produced 27 to
140 "NPU job timed out" within a single six-second inference, with a
bit-exact oracle rejecting the output, or throughput falling from 130 to
1.95 inferences per second. The timeouts land on the cores whose slot is
not their hardware number, in proportion to the tasks the scheduler hands
them, and in both directions of the mismatch.

Number the cores by their position among the core nodes in the devicetree
instead, which is what the hardware number is.

The wrong value has been assigned since the driver was added, but it only
reached the hardware once the extra bit was introduced, hence the Fixes
tag below.

Fixes: 0810d5ad88a1 ("accel/rocket: Add job submission IOCTL")
Cc: stable@vger.kernel.org
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
Assisted-by: LLM sparse checkpatch
---
 drivers/accel/rocket/rocket_core.h |  5 +++++
 drivers/accel/rocket/rocket_drv.c  | 31 +++++++++++++++++++++++++++++-
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/rocket/rocket_core.h b/drivers/accel/rocket/rocket_core.h
index f6d7382854ca9..46ed8352a79d2 100644
--- a/drivers/accel/rocket/rocket_core.h
+++ b/drivers/accel/rocket/rocket_core.h
@@ -30,6 +30,11 @@
 struct rocket_core {
 	struct device *dev;
 	struct rocket_device *rdev;
+	/*
+	 * Hardware number of the core: its position among the core nodes in
+	 * the devicetree. Not an index into rdev->cores[] - that slot is what
+	 * find_core_for_dev() returns.
+	 */
 	unsigned int index;
 
 	int irq;
diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
index 8bbbce594883e..e2cde31443ffa 100644
--- a/drivers/accel/rocket/rocket_drv.c
+++ b/drivers/accel/rocket/rocket_drv.c
@@ -157,10 +157,39 @@ static const struct drm_driver rocket_drm_driver = {
 	.desc			= "rocket DRM",
 };
 
+/*
+ * The extra bit that rocket_job_hw_submit() sets in the S_POINTER registers
+ * is the hardware number of the core, which is its position among the core
+ * nodes in the devicetree: a disabled core keeps its number. The slot a core
+ * takes in rdev->cores[] is the order the cores happened to bind in, and the
+ * two only agree while the cores that bind are a prefix of those nodes, in
+ * devicetree order. Every task submitted to a core whose slot is not its
+ * hardware number then times out.
+ */
+static int rocket_core_hw_index(struct device *dev)
+{
+	struct device_node *np;
+	int index = 0;
+
+	for_each_matching_node(np, dev->driver->of_match_table) {
+		if (np == dev->of_node) {
+			of_node_put(np);
+			return index;
+		}
+		index++;
+	}
+
+	return -ENODEV;
+}
+
 static int rocket_probe(struct platform_device *pdev)
 {
+	int index = rocket_core_hw_index(&pdev->dev);
 	int ret;
 
+	if (index < 0)
+		return index;
+
 	if (rdev == NULL) {
 		/* First core probing, initialize DRM device. */
 		rdev = rocket_device_init(drm_dev, &rocket_drm_driver);
@@ -176,7 +205,7 @@ static int rocket_probe(struct platform_device *pdev)
 
 	rdev->cores[core].rdev = rdev;
 	rdev->cores[core].dev = &pdev->dev;
-	rdev->cores[core].index = core;
+	rdev->cores[core].index = index;
 
 	rdev->num_cores++;
 

base-commit: a9f09b5ea0c3db1e2d4c0f8d3ebdd612d8aa0366
-- 
2.43.0

Re: [PATCH] accel/rocket: number the cores by devicetree position, not bind order

[sashiko-bot]

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

Pre-existing issues:
- [High] Global `rdev` pointer is left containing an `ERR_PTR` if `rocket_device_init()` fails, leading to a kernel panic on subsequent probes.
- [High] Unbinding a core (other than the last one) breaks `rdev->cores` array tracking, leading to resource leaks, missing remaining cores during lookup, and overwriting active cores on rebind.
- [High] Memory allocated via `devm_*` on `drm_dev->dev` in `rocket_device_init()` permanently leaks when all cores are unbound.
- [High] Global static variable `rdev` and its state (`rdev->num_cores`) are accessed and initialized without synchronization, leading to TOCTOU races during concurrent probing.
- [High] Premature `drm_dev_register()` inside `rocket_device_init()` exposes the device node to userspace before core initialization completes, leading to NULL pointer dereference on open.
- [High] The `scheds` array allocated in `rocket_job_open()` leaks if there is only 1 core or if `drm_sched_entity_init()` fails.
- [High] Unhandled errors in `rocket_ioctl_submit()` silently drop user jobs, causing implicit synchronization failures.
--

commit 536d84c08923eb208c65c6e48a6e81dd4a63e6f5
Author: Igor Paunovic <royalnet026@gmail.com>

accel/rocket: number the cores by devicetree position, not bind order

rocket_job_hw_submit() programs the S_POINTER registers of a core with an
extra bit derived from core->index, the way the vendor driver derives it
from the hardware number of the core. rocket_probe() sets core->index to
the slot the core takes in rdev->cores[], which is the order the cores
bind in.

> diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
> index 8bbbce594883e..e2cde31443ffa 100644
> --- a/drivers/accel/rocket/rocket_drv.c
> +++ b/drivers/accel/rocket/rocket_drv.c
[ ... ]
> @@ -157,10 +157,39 @@ static const struct drm_driver rocket_drm_driver = {
[ ... ]
>  static int rocket_probe(struct platform_device *pdev)
>  {
> +	int index = rocket_core_hw_index(&pdev->dev);
>  	int ret;
>  
> +	if (index < 0)
> +		return index;
> +
>  	if (rdev == NULL) {
>  		/* First core probing, initialize DRM device. */
>  		rdev = rocket_device_init(drm_dev, &rocket_drm_driver);

[Severity: High]
This is a pre-existing issue, but I noticed that memory allocated via devm_* 
on drm_dev->dev in rocket_device_init() might leak. 

When the last core is unbound, rocket_remove() calls rocket_device_fini() 
and sets rdev to NULL, but drm_dev is not unregistered, so the attached 
devm resources are not freed. 

Will a subsequent bind cause a permanent memory leak?

Also, another pre-existing issue: is this global rdev accessed and modified 
without synchronization? Concurrent probes might evaluate rdev == NULL and 
race to double-initialize the DRM device.

> @@ -176,7 +205,7 @@ static int rocket_probe(struct platform_device *pdev)
>  
>  	rdev->cores[core].rdev = rdev;
>  	rdev->cores[core].dev = &pdev->dev;

[Severity: High]
This is a pre-existing issue, but how does this array assignment handle 
unbinds? 

When rocket_remove() is called, it decrements rdev->num_cores without 
compacting the array:

drivers/accel/rocket/rocket_drv.c:rocket_remove() {
	...
	rocket_core_fini(&rdev->cores[core]);
	rdev->num_cores--;
	...
}

If a core other than the last one is unbound, won't this overwrite an active 
core when a new core probes and takes the slot at num_cores?

Additionally, find_core_for_dev() uses num_cores as an upper bound:

drivers/accel/rocket/rocket_drv.c:find_core_for_dev() {
	for (unsigned int core = 0; core < rdev->num_cores; core++) {
		if (dev == rdev->cores[core].dev)
			return core;
	}
	...
}

Will it fail to find remaining active cores located beyond the shrunk 
boundary?

> -	rdev->cores[core].index = core;
> +	rdev->cores[core].index = index;
>  
>  	rdev->num_cores++;

[Severity: High]
This is a pre-existing issue, but might this unprotected increment of the 
shared state lead to desynchronization and array bounds violations during 
concurrent probing?

[ ... ]

Some other pre-existing issues were noticed during review:

[Severity: High]
If rocket_device_init() fails, it returns an ERR_PTR which is left in the 
global rdev pointer:

drivers/accel/rocket/rocket_drv.c:rocket_probe() {
	...
	if (rdev == NULL) {
		rdev = rocket_device_init(drm_dev, &rocket_drm_driver);
		if (IS_ERR(rdev)) {
			...
			return PTR_ERR(rdev);
		}
	}
	
	unsigned int core = rdev->num_cores;
	...
}

Could this leave rdev holding a poisoned pointer instead of NULL? If rdev 
holds a poisoned ERR_PTR from a previous probe failure, will the subsequent 
access to rdev->num_cores dereference it and cause a kernel panic?


[Severity: High]
Does rocket_device_init() expose the device node to userspace prematurely? 

drivers/accel/rocket/rocket_device.c:rocket_device_init() {
	...
	err = drm_dev_register(ddev, 0);
	if (err)
		return ERR_PTR(err);
	...
}

If userspace intercepts the registration and immediately invokes open(), 
could rocket_open() access an uninitialized core dev pointer?

drivers/accel/rocket/rocket_drv.c:rocket_open() {
	...
	rocket_priv->domain = rocket_iommu_domain_create(rdev->cores[0].dev);
	...
}

Since the core properties are not assigned until later in rocket_probe(), 
will this panic when trying to allocate a domain?


[Severity: High]
Does the scheds array allocated in rocket_job_open() leak if 
rdev->num_cores == 1?

drivers/accel/rocket/rocket_job.c:rocket_job_open() {
	...
	struct drm_gpu_scheduler **scheds = kmalloc_objs(*scheds,
							 rdev->num_cores);
	...
	ret = drm_sched_entity_init(&rocket_priv->sched_entity,
				    DRM_SCHED_PRIORITY_NORMAL,
				    scheds,
				    rdev->num_cores, NULL);
	...
}

If num_cores is 1, drm_sched_entity_init() intentionally sets 
entity->sched_list to NULL instead of storing the array. Then during close:

drivers/accel/rocket/rocket_job.c:rocket_job_close() {
	...
	kfree(entity->sched_list);
	...
}

Doesn't this incorrectly assume entity->sched_list always holds the pointer, 
causing a leak when num_cores == 1 or if drm_sched_entity_init() fails?


[Severity: High]
Are job submission errors ignored in rocket_ioctl_submit()?

drivers/accel/rocket/rocket_job.c:rocket_ioctl_submit() {
	...
	for (i = 0; i < args->job_count; i++)
		rocket_ioctl_submit_job(dev, file, &jobs[i]);
	...
}

If rocket_ioctl_submit_job() fails, the loop blindly discards its error 
return value. Will this silently drop user jobs and cause implicit 
synchronization failures since the output fences are never injected?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905135612.7324-1-royalnet026@gmail.com?part=1