[PATCH] accel/rocket: search every core slot when a core is removed

[PATCH] accel/rocket: search every core slot when a core is removed

[Igor Paunovic]

rocket_remove() decrements rdev->num_cores for each core it removes,
while find_core_for_dev() searches slots 0 to num_cores - 1. Unbinding
the cores in the order they were bound therefore loses the last one: by
the time it is removed the search range has already shrunk past its
slot, so find_core_for_dev() returns -1 and rocket_remove() gives up
without doing anything.

num_cores never reaches zero, rocket_device_fini() never runs, and the
file-scoped rdev keeps pointing at a device that is going away. Binding
the cores again starts from that stale count, because rocket_probe()
takes rdev->num_cores as the slot to fill. On an RK3588, which describes
three cores, the second round lands on slots 1, 2 and 3 while rdev->cores
was allocated with room for three:

  rocket fdab0000.npu: Rockchip NPU core 1 version: 1179210309
  rocket fdac0000.npu: Rockchip NPU core 2 version: 1179210309
  rocket fdad0000.npu: Rockchip NPU core 3 version: 1179210309

The write to rdev->cores[3] is past the end of the array.

Nothing in tree reads the core array often enough to notice, so the
overrun is silent today. It turned up while testing a devfreq series on
top of this, where a worker walks every core a few times a second, and
UBSAN caught the first bool it read out of the overrun entry:

  UBSAN: invalid-load in drivers/accel/rocket/rocket_devfreq.c:47:10
  load of value 5 is not a valid value for type '_Bool'
  Workqueue: devfreq_wq devfreq_monitor

Record how many slots were allocated and search all of them. Every core
is then found on removal, num_cores reaches zero, the device is torn down
and a later bind starts from a clean rdev.

This does not make unbinding a single core out of several work. probe
still takes num_cores as the slot to fill, so rebinding one core while
its siblings stay bound would write over a slot that is already in use,
and rocket_open() still reaches for cores[0] whether or not anything is
there. Both of those want more thought than a fix should carry.

Found by unbinding and rebinding all three cores on an Orange Pi 5 Plus.
With this applied, 25 unbind/rebind rounds and 5 module unload/reload
rounds run clean there: the cores land in slots 0, 1 and 2 every time,
whichever order they are bound in, and the shared supply goes back to a
single user after each round.

Fixes: ed98261b4168 ("accel/rocket: Add a new driver for Rockchip's NPU")
Cc: stable@vger.kernel.org
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
Assisted-by: LLM sparse checkpatch
---
 drivers/accel/rocket/rocket_device.c | 2 ++
 drivers/accel/rocket/rocket_device.h | 1 +
 drivers/accel/rocket/rocket_drv.c    | 2 +-
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/rocket/rocket_device.c b/drivers/accel/rocket/rocket_device.c
index 46e6ee1e72c5f..efd004194c1af 100644
--- a/drivers/accel/rocket/rocket_device.c
+++ b/drivers/accel/rocket/rocket_device.c
@@ -31,6 +31,8 @@ struct rocket_device *rocket_device_init(struct platform_device *pdev,
 		if (of_device_is_available(core_node))
 			num_cores++;
 
+	rdev->max_cores = num_cores;
+
 	rdev->cores = devm_kcalloc(dev, num_cores, sizeof(*rdev->cores), GFP_KERNEL);
 	if (!rdev->cores)
 		return ERR_PTR(-ENOMEM);
diff --git a/drivers/accel/rocket/rocket_device.h b/drivers/accel/rocket/rocket_device.h
index ce662abc01d3d..c62d567010696 100644
--- a/drivers/accel/rocket/rocket_device.h
+++ b/drivers/accel/rocket/rocket_device.h
@@ -19,6 +19,7 @@ struct rocket_device {
 
 	struct rocket_core *cores;
 	unsigned int num_cores;
+	unsigned int max_cores;
 };
 
 struct rocket_device *rocket_device_init(struct platform_device *pdev,
diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
index 8bbbce594883e..2bcfe4ab3c68f 100644
--- a/drivers/accel/rocket/rocket_drv.c
+++ b/drivers/accel/rocket/rocket_drv.c
@@ -223,7 +223,7 @@ static int find_core_for_dev(struct device *dev)
 {
 	struct rocket_device *rdev = dev_get_drvdata(dev);
 
-	for (unsigned int core = 0; core < rdev->num_cores; core++) {
+	for (unsigned int core = 0; core < rdev->max_cores; core++) {
 		if (dev == rdev->cores[core].dev)
 			return core;
 	}
-- 
2.43.0


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

Re: [PATCH] accel/rocket: search every core slot when a core is removed

[Igor Paunovic]

Answering the one question that is about this patch rather than about the
state it leaves behind, and confirming the rest.

> Because rocket_remove() doesn't clear the dev pointer or compact the
> array, wouldn't subsequent out-of-order unbinds match stale pointers
> since find_core_for_dev() now searches up to max_cores?

There are three callers: rocket_remove() and the two runtime PM callbacks.
The driver core calls remove once per device, and the PM callbacks cannot
run for an unbound one, because rocket_core_fini() has already called
pm_runtime_disable() on it. The slots the widened search adds are either
never filled, where .dev is NULL and matches nothing, or held by a device
that has been unbound - and nothing asks after such a device again.

What the narrower search did do was lose live cores. Unbind the core in
slot 0 of three: num_cores drops to two, so find_core_for_dev() stops
before slot 2. The core sitting there is still bound and still running,
but its own runtime suspend and resume callbacks start returning -ENODEV.
Searching every allocated slot fixes that as well.

An earlier version of this patch did clear .dev on removal and take a free
slot on probe. I dropped both. Clearing .dev turns rocket_open()'s
unconditional cores[0] into a NULL dereference whenever the core in slot 0
is unbound while its siblings stay bound, which is a worse failure than
the one it cures - and it is the same rocket_open() issue listed further
down. That is why the commit message says out-of-order unbind wants more
thought than a fix should carry, rather than quietly half-fixing it.

On the rest: all eight are pre-existing and I agree with all eight. Two of
them have names already.

The ERR_PTR left in the file-scoped rdev is fixed by Guangshuo Li's
"accel/rocket: clear rdev on device init failure", posted in July and
still unapplied:

  https://lore.kernel.org/dri-devel/20260708062845.716487-1-lgs201920130244@gmail.com/

It carries my Reviewed-by. It would be good to see that one land.

The devm point may explain something I measured this week and could not
account for. Unbinding and rebinding all three cores walks the DRM minor
upwards - 1 through 10 over ten rounds in one run - and only a module
reload puts it back to 0. I have not shown that the allocations are
leaked, only that something survives a rebind that should not, which is
consistent with what you describe.

Igor

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

Re: [PATCH] accel/rocket: search every core slot when a core is removed

[Jiaxing Hu]

Hi Igor,

Two unbind and bind rounds on a Radxa ROCK 4D: bound: 0, then bound: 2
with the cores numbered 0 and 1 both times, and nine models decoded
identically afterwards. Moved here from the DVFS thread so that b4 can
collect it.

Tested-by: Jiaxing Hu <gahing@gahingwoo.com> # RK3576, two cores

Jiaxing

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

Re: [PATCH] accel/rocket: search every core slot when a core is removed

[Igor Paunovic]

Hi Jiaxing,

> Moved here from the DVFS thread so that b4 can collect it.
>
> Tested-by: Jiaxing Hu <gahing@gahingwoo.com> # RK3576, two cores

Thank you, and it works: b4 picks it up with the comment attached and
the DKIM check passes. Two cores on a different SoC is worth more to
this patch than anything I can produce on my own board.

Your test also found the edge of something else, and I would rather
tell you before you spend more rounds on it.

You wrote that the cores came back "numbered 0 and 1 both times". That
is the case that works. I rebound the three cores here in all six
orders and only the devicetree order ran clean:

  bind order (slot 0 first)   NPU job timed out         inference
  fdab fdac fdad (devicetree) none                      129.4 inf/s, oracle ok
  fdab fdad fdac              27 fdad                   1.95 inf/s, oracle ok
  fdac fdab fdad              135 fdac, 5 fdab, 1 fdad  oracle rejected output
  fdac fdad fdab              135 fdac, 5 fdad          oracle rejected output
  fdad fdab fdac              136 fdad, 5 fdab          oracle rejected output
  fdad fdac fdab              135 fdad, 1 fdab          oracle rejected output

Counted from the journal since each bind, one six-second inference per
row. The single fdad timeout in row three is on a correctly numbered
core and I cannot account for it; the most likely explanation is a job
still draining from the previous row, since the count starts before the
unbind. Everything else lands on a core whose slot is not its hardware
number, in proportion to the tasks the scheduler gave it, and in both
directions of the mismatch: 5 timeouts on the core in slot 1 whether its
hardware number is above it (row four) or below it (row five).

The cause is in rocket_job_hw_submit():

  extra_bit = 0x10000000 * core->index;

core->index is the slot the core takes in rdev->cores[], which is bind
order. The vendor driver computes that same bit from the hardware
number of the core (rknpu_job.c: REG_WRITE((0xe + 0x10000000 * i), ...)
with i indexing rknpu_dev->base[]). On a normal boot the two agree and
nothing shows. Bind out of that order and every task submitted to a
core whose slot is not its hardware number times out after 500 ms, the
reset does not help, and the inference finishes with wrong output. No
error, no warning, no regulator or clock message - the rail sat at
700000 uV in the runs that passed and in the runs that failed. Only the
bit-exact oracle and the UART log showed it.

The second row is the one I would have missed. The oracle passed there,
because the tensors it checks are computed by the core in slot 0, which
happened to be correct. Throughput was 66 times lower and 27 jobs had
timed out. A single assertion does not catch this.

Two cores make it cheaper to test than three. If you bind the second
core before the first on your ROCK 4D, on your current kernel, I expect
the jobs that land on slot 0 to time out and your nine models to stop
decoding identically. If they do, that is the bug reproduced on a second
SoC by someone who is not me, which is worth more than my six rows.
If they do not, I have the cause wrong and I would like to know that -
please say so on the patch thread rather than here, so it lands where
b4 will pick it up.

The fix is on the list now:

  https://lore.kernel.org/dri-devel/20260905135612.7324-1-royalnet026@gmail.com/

It numbers the cores by their position among the core nodes in the
devicetree. It resolves that through dev->driver->of_match_table
rather than a compatible string, so it works both before and after your
12/14, which moves that enumeration to for_each_matching_node() and
renames the table. All six orders pass afterwards, 124 to 136 inf/s,
zero timeouts. It is a separate patch with Fixes: and Cc: stable, and
it goes out before v2 of the DVFS series.

One more thing your report is useful for: it is the same class as what
you found on RK3576 last week. Wrong result, no complaint from the
hardware. I am starting to think this driver needs a bit-exact check in
whatever test people run against it, because throughput alone will
happily report a healthy number while the output is garbage.

Igor

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

Re: [PATCH] accel/rocket: search every core slot when a core is removed

[Sidong Yang]

On Fri, Sep 04, 2026 at 02:59:36PM +0200, Igor Paunovic wrote:
> rocket_remove() decrements rdev->num_cores for each core it removes,
> while find_core_for_dev() searches slots 0 to num_cores - 1. Unbinding
> the cores in the order they were bound therefore loses the last one: by
> the time it is removed the search range has already shrunk past its
> slot, so find_core_for_dev() returns -1 and rocket_remove() gives up
> without doing anything.
> 
> num_cores never reaches zero, rocket_device_fini() never runs, and the
> file-scoped rdev keeps pointing at a device that is going away. Binding
> the cores again starts from that stale count, because rocket_probe()
> takes rdev->num_cores as the slot to fill. On an RK3588, which describes
> three cores, the second round lands on slots 1, 2 and 3 while rdev->cores
> was allocated with room for three:
> 
>   rocket fdab0000.npu: Rockchip NPU core 1 version: 1179210309
>   rocket fdac0000.npu: Rockchip NPU core 2 version: 1179210309
>   rocket fdad0000.npu: Rockchip NPU core 3 version: 1179210309
> 
> The write to rdev->cores[3] is past the end of the array.
> 
> Nothing in tree reads the core array often enough to notice, so the
> overrun is silent today. It turned up while testing a devfreq series on
> top of this, where a worker walks every core a few times a second, and
> UBSAN caught the first bool it read out of the overrun entry:
> 
>   UBSAN: invalid-load in drivers/accel/rocket/rocket_devfreq.c:47:10
>   load of value 5 is not a valid value for type '_Bool'
>   Workqueue: devfreq_wq devfreq_monitor
> 
> Record how many slots were allocated and search all of them. Every core
> is then found on removal, num_cores reaches zero, the device is torn down
> and a later bind starts from a clean rdev.
> 
> This does not make unbinding a single core out of several work. probe
> still takes num_cores as the slot to fill, so rebinding one core while
> its siblings stay bound would write over a slot that is already in use,
> and rocket_open() still reaches for cores[0] whether or not anything is
> there. Both of those want more thought than a fix should carry.
> 
> Found by unbinding and rebinding all three cores on an Orange Pi 5 Plus.
> With this applied, 25 unbind/rebind rounds and 5 module unload/reload
> rounds run clean there: the cores land in slots 0, 1 and 2 every time,
> whichever order they are bound in, and the shared supply goes back to a
> single user after each round.
> 
> Fixes: ed98261b4168 ("accel/rocket: Add a new driver for Rockchip's NPU")
> Cc: stable@vger.kernel.org
> Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
> Assisted-by: LLM sparse checkpatch
> ---
>  drivers/accel/rocket/rocket_device.c | 2 ++
>  drivers/accel/rocket/rocket_device.h | 1 +
>  drivers/accel/rocket/rocket_drv.c    | 2 +-
>  3 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/accel/rocket/rocket_device.c b/drivers/accel/rocket/rocket_device.c
> index 46e6ee1e72c5f..efd004194c1af 100644
> --- a/drivers/accel/rocket/rocket_device.c
> +++ b/drivers/accel/rocket/rocket_device.c
> @@ -31,6 +31,8 @@ struct rocket_device *rocket_device_init(struct platform_device *pdev,
>  		if (of_device_is_available(core_node))
>  			num_cores++;
>  
> +	rdev->max_cores = num_cores;
> +
>  	rdev->cores = devm_kcalloc(dev, num_cores, sizeof(*rdev->cores), GFP_KERNEL);
>  	if (!rdev->cores)
>  		return ERR_PTR(-ENOMEM);
> diff --git a/drivers/accel/rocket/rocket_device.h b/drivers/accel/rocket/rocket_device.h
> index ce662abc01d3d..c62d567010696 100644
> --- a/drivers/accel/rocket/rocket_device.h
> +++ b/drivers/accel/rocket/rocket_device.h
> @@ -19,6 +19,7 @@ struct rocket_device {
>  
>  	struct rocket_core *cores;
>  	unsigned int num_cores;
> +	unsigned int max_cores;

Hi Igor,

I've tested this patch in Radxa Rock 5 B+ and it works. The test is that unbinding all
cores in sequence 0,1,2 and rebind all. It seems that there is other issue about num_core.
For example, sched_to_core() finds core for sched with num_core and it could make same
error like find_core_for_dev().

Tested-by: Sidong Yang <sidong.yang@furiosa.ai> # RK3588, 3 cores

Thanks,
Sidong

>  };
>  
>  struct rocket_device *rocket_device_init(struct platform_device *pdev,
> diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
> index 8bbbce594883e..2bcfe4ab3c68f 100644
> --- a/drivers/accel/rocket/rocket_drv.c
> +++ b/drivers/accel/rocket/rocket_drv.c
> @@ -223,7 +223,7 @@ static int find_core_for_dev(struct device *dev)
>  {
>  	struct rocket_device *rdev = dev_get_drvdata(dev);
>  
> -	for (unsigned int core = 0; core < rdev->num_cores; core++) {
> +	for (unsigned int core = 0; core < rdev->max_cores; core++) {
>  		if (dev == rdev->cores[core].dev)
>  			return core;
>  	}
> -- 
> 2.43.0
> 

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

Re: [PATCH] accel/rocket: search every core slot when a core is removed

[Igor Paunovic]

Hi Sidong,

> I've tested this patch in Radxa Rock 5 B+ and it works.

Thank you. That is the first test of it on an RK3588 that is not mine, and
b4 collects your tag with the comment attached.

> It seems that there is other issue about num_core. For example,
> sched_to_core() finds core for sched with num_core and it could make
> same error like find_core_for_dev().

You were right, and it is worse than a failed lookup: neither caller checks
what sched_to_core() returns.

I built a KASAN kernel and unbound the middle of the three cores while three
clients were submitting to all of them. It faults twice, once from the
surviving core's job queue and once from its reset work:

  KASAN: null-ptr-deref in range [0x0000000000000220-0x0000000000000227]
  Workqueue: fdad0000.npu drm_sched_run_job_work [gpu_sched]
  pc : rocket_job_run+0x234/0x838 [rocket]

  KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
  Workqueue: rocket-reset-2 drm_sched_job_timedout [gpu_sched]
  pc : rocket_job_timedout+0xf0/0x1e0 [rocket]

Both are the third core. The workqueue names are its device and its
core->index, and it had been left at slot 2 while num_cores was down to 2.

The fix is one word, and it carries your Reported-by:

  https://lore.kernel.org/dri-devel/20260905150432.7477-1-royalnet026@gmail.com/

Same test on a kernel with it applied: no faults, journal clean. It applies
on top of the patch you tested, since max_cores comes from that one.

What it does not fix, and the patch says so: an open client keeps an entity
pointing at the scheduler of the core that went away. drm_sched then logs
"not ready, skipping" for every job that lands on it - 25006 of them in my
run - and the client waits in dma_fence_default_wait for a fence that will
never signal. The board stays up and the client hangs. Making one core of
several safe to unbind while a client is open needs more than a fix, and I
did not want to hide that behind a patch that only stops the oops.

Thanks for reading it closely enough to spot the second one.

Igor

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