[PATCH 0/2] accel/rocket: fix shared-device lifecycle on probe failure and unbind

[PATCH 0/2] accel/rocket: fix shared-device lifecycle on probe failure and unbind

[Igor Paunovic]

The rocket driver keeps a single shared DRM device on a driverless
"rknn" platform device: the first core to probe initializes it, the
last one to go away tears it down. This series fixes two independent
bugs in that lifecycle. Both were flagged by the Sashiko AI review on
my clks patch; I verified each by hand against the code and then on
hardware before writing the fixes.

Patch 1 releases the devres of the shared device on teardown. Today
every fini/re-init cycle leaks the previous rocket_device and pins its
accel minor - observable as /dev/accel/accel0 coming back as accel1,
then accel2, on unbind/rebind cycles of all cores.

Patch 2 makes the per-core slot bookkeeping stable across unbind and
rebind in any order. Today unbinding a lower-numbered core makes
higher-numbered ones unfindable (their runtime PM callbacks start
returning -ENODEV), a later unbind of such a core is silently skipped,
and a subsequent bind overwrites a slot whose IRQ handler and DRM
scheduler are still live.

Verified on RK3588 (Orange Pi 5 Plus, all three cores): the
unbind/rebind matrix keeps /dev/accel/accel0 stable and every core
findable; single-core operation works from the highest slot alone
(confirmed via the per-core IRQ counters moving to that core); and a
MobileNetV1 inference run via the Teflon TFLite delegate stays
bit-identical to the stock driver throughout.

The series applies on top of Guangshuo Li's pending fix, on which
patch 1 depends textually (reviewed on-list earlier today):
https://lore.kernel.org/dri-devel/20260708062845.716487-1-lgs201920130244@gmail.com/

Igor Paunovic (2):
  accel/rocket: release the shared device's devres on teardown
  accel/rocket: keep core slots stable across unbind and rebind

 drivers/accel/rocket/rocket_device.c |  2 ++
 drivers/accel/rocket/rocket_device.h |  3 +++
 drivers/accel/rocket/rocket_drv.c    | 40 ++++++++++++++++++++++++++++++----
 drivers/accel/rocket/rocket_job.c    | 11 ++++++-----
 4 files changed, 47 insertions(+), 9 deletions(-)

--
2.53.0

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

[PATCH 1/2] accel/rocket: release the shared device's devres on teardown

[Igor Paunovic]

rocket_device_init() attaches its allocations to the shared "rknn"
platform device via devres: devm_drm_dev_alloc(), devm_kcalloc() for
the cores array and devm_mutex_init(). That device is registered at
module init, never binds to a driver, and is only unregistered at
module exit - so its devres list is not released for as long as the
module is loaded.

rocket_device_fini() only calls drm_dev_unregister(): it does not run
the drm_dev_put() devres action or free any of the other entries.
Every fini/re-init cycle therefore leaks the previous rocket_device
(with its embedded drm_device and all drmm state, including the accel
minor number), the cores array and the mutex devres node. The cycle is
easy to trigger: unbind the last bound core and bind one again, or
fail the first core's probe (-EPROBE_DEFER retries included).

Observable symptom, RK3588 (Orange Pi 5 Plus): each unbind/rebind
cycle of all three cores moves the accel node forward -
/dev/accel/accel0 comes back as accel1, then accel2 - because every
leaked drm_device keeps its minor pinned.

Wrap the initialization in a devres group and release exactly that
group wherever the device is torn down: on the rocket_device_init()
error path, when the first core's rocket_core_init() fails, and when
the last core is removed. Each fini now frees what the matching init
allocated, and the accel minor is reusable again.

Fixes: ed98261b4168 ("accel/rocket: Add a new driver for Rockchip's NPU")
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
---
This applies on top of Guangshuo Li's pending fix, which it depends on:
"accel/rocket: clear rdev on device init failure"
https://lore.kernel.org/dri-devel/20260708062845.716487-1-lgs201920130244@gmail.com/

Verified on RK3588 (Orange Pi 5 Plus): with the patch, repeated
unbind/rebind cycles keep /dev/accel/accel0 stable (previously the
minor incremented on every cycle); normal three-core probe, runtime PM
and a MobileNetV1 inference run via the Teflon TFLite delegate are
unaffected.

 drivers/accel/rocket/rocket_drv.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
index 67e7f54..d29c5ee 100644
--- a/drivers/accel/rocket/rocket_drv.c
+++ b/drivers/accel/rocket/rocket_drv.c
@@ -24,6 +24,7 @@
  */
 static struct platform_device *drm_dev;
 static struct rocket_device *rdev;
+static void *rdev_group;
 
 static void
 rocket_iommu_domain_destroy(struct kref *kref)
@@ -163,14 +164,19 @@ static int rocket_probe(struct platform_device *pdev)
 
 	if (rdev == NULL) {
 		/* First core probing, initialize DRM device. */
+		rdev_group = devres_open_group(&drm_dev->dev, NULL, GFP_KERNEL);
+		if (!rdev_group)
+			return -ENOMEM;
 		rdev = rocket_device_init(drm_dev, &rocket_drm_driver);
 		if (IS_ERR(rdev)) {
 			int err = PTR_ERR(rdev);
 
 			dev_err(&pdev->dev, "failed to initialize rocket device\n");
 			rdev = NULL;
+			devres_release_group(&drm_dev->dev, rdev_group);
 			return err;
 		}
+		devres_close_group(&drm_dev->dev, rdev_group);
 	}
 
 	unsigned int core = rdev->num_cores;
@@ -190,6 +196,7 @@ static int rocket_probe(struct platform_device *pdev)
 		if (rdev->num_cores == 0) {
 			rocket_device_fini(rdev);
 			rdev = NULL;
+			devres_release_group(&drm_dev->dev, rdev_group);
 		}
 	}
 
@@ -213,6 +220,7 @@ static void rocket_remove(struct platform_device *pdev)
 		/* Last core removed, deinitialize DRM device. */
 		rocket_device_fini(rdev);
 		rdev = NULL;
+		devres_release_group(&drm_dev->dev, rdev_group);
 	}
 }
 

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

[PATCH 2/2] accel/rocket: keep core slots stable across unbind and rebind

[Igor Paunovic]

The shared rocket_device tracks bound cores with a single counter and
uses it for three different jobs at once: find_core_for_dev() searches
[0, num_cores), rocket_probe() inserts the new core at index num_cores,
and rocket_remove() only decrements the counter without clearing the
slot.

This bookkeeping falls apart as soon as cores are unbound in any order
other than strict reverse bind order:

 - unbinding core 0 shrinks the search range, so the still-bound core
   at the highest index can no longer be found: its runtime PM
   callbacks start failing with -ENODEV and a later unbind of it is
   silently ignored, skipping rocket_core_fini() entirely;

 - a subsequent bind then reuses the index of that still-live core and
   overwrites its slot while its IRQ handler (dev_id points into
   cores[]) and its DRM scheduler are still active;

 - rocket_open() unconditionally uses cores[0].dev, which after an
   unbind of core 0 is a stale pointer to an unbound device.

Give the array a fixed capacity (max_cores, the DT core count already
used to size the allocation) and make .dev the slot-liveness marker:
probe takes the first free slot, remove clears .dev after
rocket_core_fini() and warns if the core cannot be found, lookups
iterate the full capacity, and rocket_open() and rocket_job_open() use
only live slots. num_cores keeps counting bound cores for the
last-core teardown check.

Fixes: ed98261b4168 ("accel/rocket: Add a new driver for Rockchip's NPU")
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
---
Unbinding a core that still has jobs in flight has further
pre-existing issues (scheduler and open-file lifetime) that are out of
scope for this bookkeeping fix.

Verified on RK3588 (Orange Pi 5 Plus): out-of-order unbind/rebind
sequences (including the previously corrupting unbind of core 0 with
cores 1 and 2 still bound, followed by rebind) now keep all three
cores findable and functional, with a MobileNetV1 inference run via
the Teflon TFLite delegate bit-identical to the stock driver
afterwards.

 drivers/accel/rocket/rocket_device.c |  2 ++
 drivers/accel/rocket/rocket_device.h |  3 +++
 drivers/accel/rocket/rocket_drv.c    | 32 ++++++++++++++++++++++++++++----
 drivers/accel/rocket/rocket_job.c    | 11 ++++++-----
 4 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/drivers/accel/rocket/rocket_device.c b/drivers/accel/rocket/rocket_device.c
index 46e6ee1..8303f05 100644
--- a/drivers/accel/rocket/rocket_device.c
+++ b/drivers/accel/rocket/rocket_device.c
@@ -35,6 +35,8 @@ struct rocket_device *rocket_device_init(struct platform_device *pdev,
 	if (!rdev->cores)
 		return ERR_PTR(-ENOMEM);
 
+	rdev->max_cores = num_cores;
+
 	dma_set_max_seg_size(dev, UINT_MAX);
 
 	err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(40));
diff --git a/drivers/accel/rocket/rocket_device.h b/drivers/accel/rocket/rocket_device.h
index ce662ab..7fb6a9d 100644
--- a/drivers/accel/rocket/rocket_device.h
+++ b/drivers/accel/rocket/rocket_device.h
@@ -18,6 +18,9 @@ struct rocket_device {
 	struct mutex sched_lock;
 
 	struct rocket_core *cores;
+	/* Slot capacity (DT core count); slots with a NULL .dev are free. */
+	unsigned int max_cores;
+	/* Number of currently bound cores. */
 	unsigned int num_cores;
 };
 
diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
index d29c5ee..04c1e47 100644
--- a/drivers/accel/rocket/rocket_drv.c
+++ b/drivers/accel/rocket/rocket_drv.c
@@ -69,11 +69,21 @@ rocket_iommu_domain_put(struct rocket_iommu_domain *domain)
 	kref_put(&domain->kref, rocket_iommu_domain_destroy);
 }
 
+static struct rocket_core *rocket_first_live_core(struct rocket_device *rdev)
+{
+	for (unsigned int core = 0; core < rdev->max_cores; core++)
+		if (rdev->cores[core].dev)
+			return &rdev->cores[core];
+
+	return NULL;
+}
+
 static int
 rocket_open(struct drm_device *dev, struct drm_file *file)
 {
 	struct rocket_device *rdev = to_rocket_device(dev);
 	struct rocket_file_priv *rocket_priv;
+	struct rocket_core *core;
 	u64 start, end;
 	int ret;
 
@@ -86,8 +96,14 @@ rocket_open(struct drm_device *dev, struct drm_file *file)
 		goto err_put_mod;
 	}
 
+	core = rocket_first_live_core(rdev);
+	if (!core) {
+		ret = -ENODEV;
+		goto err_free;
+	}
+
 	rocket_priv->rdev = rdev;
-	rocket_priv->domain = rocket_iommu_domain_create(rdev->cores[0].dev);
+	rocket_priv->domain = rocket_iommu_domain_create(core->dev);
 	if (IS_ERR(rocket_priv->domain)) {
 		ret = PTR_ERR(rocket_priv->domain);
 		goto err_free;
@@ -179,10 +195,17 @@ static int rocket_probe(struct platform_device *pdev)
 		devres_close_group(&drm_dev->dev, rdev_group);
 	}
 
-	unsigned int core = rdev->num_cores;
+	unsigned int core;
 
 	dev_set_drvdata(&pdev->dev, rdev);
 
+	/* Take the first free slot: cores can unbind and rebind in any order. */
+	for (core = 0; core < rdev->max_cores; core++)
+		if (!rdev->cores[core].dev)
+			break;
+	if (WARN_ON(core == rdev->max_cores))
+		return -ENXIO;
+
 	rdev->cores[core].rdev = rdev;
 	rdev->cores[core].dev = &pdev->dev;
 	rdev->cores[core].index = core;
@@ -210,10 +233,11 @@ static void rocket_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int core = find_core_for_dev(dev);
 
-	if (core < 0)
+	if (WARN_ON(core < 0))
 		return;
 
 	rocket_core_fini(&rdev->cores[core]);
+	rdev->cores[core].dev = NULL;
 	rdev->num_cores--;
 
 	if (rdev->num_cores == 0) {
@@ -234,7 +258,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;
 	}
diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index ac51bff..680f943 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -276,7 +276,7 @@ static struct rocket_core *sched_to_core(struct rocket_device *rdev,
 {
 	unsigned int core;
 
-	for (core = 0; core < rdev->num_cores; core++) {
+	for (core = 0; core < rdev->max_cores; core++) {
 		if (&rdev->cores[core].sched == sched)
 			return &rdev->cores[core];
 	}
@@ -498,16 +498,17 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv)
 	struct rocket_device *rdev = rocket_priv->rdev;
 	struct drm_gpu_scheduler **scheds = kmalloc_objs(*scheds,
 							 rdev->num_cores);
-	unsigned int core;
+	unsigned int core, n = 0;
 	int ret;
 
-	for (core = 0; core < rdev->num_cores; core++)
-		scheds[core] = &rdev->cores[core].sched;
+	for (core = 0; core < rdev->max_cores; core++)
+		if (rdev->cores[core].dev)
+			scheds[n++] = &rdev->cores[core].sched;
 
 	ret = drm_sched_entity_init(&rocket_priv->sched_entity,
 				    DRM_SCHED_PRIORITY_NORMAL,
 				    scheds,
-				    rdev->num_cores, NULL);
+				    n, NULL);
 	if (WARN_ON(ret))
 		return ret;
 

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

Re: [PATCH 2/2] accel/rocket: keep core slots stable across unbind and rebind

[Jiaxing Hu]

Hi Igor,

I like this one. Making .dev the liveness marker is the right shape, and
the analysis of the three jobs num_cores was doing is exactly right.

But making .dev load-bearing needs one more site than the patch touches:
rocket_probe()'s failure path never clears it.

  rdev->cores[core].dev = &pdev->dev;
  ...
  ret = rocket_core_init(&rdev->cores[core]);
  if (ret) {
          rdev->num_cores--;
          if (rdev->num_cores == 0) {
                  rocket_device_fini(rdev);
                  rdev = NULL;
                  devres_release_group(&drm_dev->dev, rdev_group);
          }
  }

Before this patch that was harmless, because every lookup was bounded by
num_cores and the slot fell outside it. After it, the slot stays marked
live while its rocket_core is half-initialised.

It only bites when the failing core is not the last one bound. If
num_cores drops to zero the whole cores array is freed by the devres
release from patch 1, and the stale .dev goes with it. So the case to
test is core N failing to init while cores 0..N-1 are already up,
easiest with a forced error return in rocket_core_init(), since a real
-EPROBE_DEFER retries rather than failing.

What that leaves behind:

 - find_core_for_dev() finds the slot, so the runtime PM callbacks run
   against a core whose clocks/resets/IRQ were never set up;
 - rocket_first_live_core() can return it, and rocket_open() then builds
   the IOMMU domain against that device;
 - rocket_job_open() adds &cores[core].sched to the scheds array, and
   that scheduler was never drm_sched_init()ed.

That last one also overflows. scheds is sized rdev->num_cores, but the
loop now counts live slots, and after a failed init there is one more
live slot than num_cores, so the last scheds[n++] writes one element
past the allocation.

One line fixes all of it:

  if (ret) {
          rdev->cores[core].dev = NULL;
          rdev->num_cores--;
          ...

Two smaller things, neither blocking:

The slot scan in rocket_probe() is a scan-then-claim with nothing
serialising it against another core's probe. Platform probing is
synchronous today so it cannot bite, and the old code had the same
property via num_cores, so it is no worse. Just worth knowing it is
still there if rocket ever gains async probe.

sched_to_core() scanning max_cores now walks slots whose .sched is not
initialised. Comparing &cores[core].sched against a live scheduler
pointer can never match on those, so it is correct as written; a .dev
check would make it obviously correct to a reader.

I have only RK3576 here, so this is a code review rather than a
Tested-by. I cannot exercise the multi-core unbind matrix.

Thanks,
Jiaxing

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

Re: [PATCH 2/2] accel/rocket: keep core slots stable across unbind and rebind

[Igor Paunovic]

Hi Jiaxing,

Thanks - that is exactly the kind of hole a reviewer is for. You were
right on all three counts: the failure path left the slot marked live,
and the live-slot walk in rocket_job_open() could then write one entry
past its allocation.

v2 is out with all three addressed:
https://lore.kernel.org/dri-devel/20260731064933.12548-1-royalnet026@gmail.com/

I also exercised the path you described on RK3588 - a forced error
return in rocket_core_init() for core 2 with cores 0 and 1 bound: the
slot is released, the device comes up with the two remaining cores,
and inference passes bit-exact with only the two live cores'
interrupts firing.

Thanks again for the careful review,
Igor

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