[PATCH] drm/panthor: Display priorities of panthor groups over debugfs

[PATCH] drm/panthor: Display priorities of panthor groups over debugfs

[Nicolas Frattaroli]

Analogous to what was added in Commit d41c79838c47 ("drm/panfrost:
Display list of device JM contexts over debugfs") for panfrost, add
similar debugfs information for panthor.

The group priority does not change over the lifetime of the group, so no
effort to synchronise with the scheduler lock is being made.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
Some additional notes: I noticed panthor apparently uses
xa_for_each{_marked} by guarding it with xa_lock/xa_unlock. That appears
to be unnecessary, judging by the documentation of it and that nobody
else does this.
---
 drivers/gpu/drm/panthor/panthor_drv.c   |  1 +
 drivers/gpu/drm/panthor/panthor_sched.c | 89 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/panthor/panthor_sched.h |  5 ++
 3 files changed, 95 insertions(+)

diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
index 46a3080b0b20..8cdba0c1a14b 100644
--- a/drivers/gpu/drm/panthor/panthor_drv.c
+++ b/drivers/gpu/drm/panthor/panthor_drv.c
@@ -1769,6 +1769,7 @@ static void panthor_debugfs_init(struct drm_minor *minor)
 {
 	panthor_mmu_debugfs_init(minor);
 	panthor_gem_debugfs_init(minor);
+	panthor_sched_debugfs_init(minor);
 }
 #endif
 
diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index 5832dccfc093..44b61e946e2d 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0 or MIT
 /* Copyright 2023 Collabora ltd. */
 
+#include <drm/drm_debugfs.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_exec.h>
 #include <drm/drm_file.h>
@@ -4198,3 +4199,91 @@ int panthor_sched_init(struct panthor_device *ptdev)
 	ptdev->scheduler = sched;
 	return 0;
 }
+
+#ifdef CONFIG_DEBUG_FS
+
+static const char *
+panthor_sched_prio_str(enum panthor_csg_priority prio)
+{
+	switch (prio) {
+	case PANTHOR_CSG_PRIORITY_LOW:
+		return "LOW";
+	case PANTHOR_CSG_PRIORITY_MEDIUM:
+		return "MEDIUM";
+	case PANTHOR_CSG_PRIORITY_HIGH:
+		return "HIGH";
+	case PANTHOR_CSG_PRIORITY_RT:
+		return "REAL-TIME";
+	default:
+		return "UNKNOWN";
+	}
+}
+
+static int show_file_groups(struct panthor_file *pfile, struct seq_file *m)
+{
+	struct panthor_group *group;
+	unsigned long i;
+
+	if (IS_ERR_OR_NULL(pfile->groups))
+		return -ENOENT;
+
+	xa_for_each_marked(&pfile->groups->xa, i, group, GROUP_REGISTERED) {
+		seq_printf(m, " Group %lu: priority %s\n", i,
+			   panthor_sched_prio_str(group->priority));
+	}
+
+	return 0;
+}
+
+static int show_each_file(struct seq_file *m, void *arg)
+{
+	struct drm_info_node *node = (struct drm_info_node *)m->private;
+	struct drm_device *ddev = node->minor->dev;
+	int (*show)(struct panthor_file *, struct seq_file *) =
+		node->info_ent->data;
+	struct drm_file *file;
+	int ret = 0;
+
+	scoped_cond_guard(mutex_intr, return -EINTR, &ddev->filelist_mutex) {
+		list_for_each_entry(file, &ddev->filelist, lhead) {
+			struct task_struct *task;
+			struct panthor_file *pfile = file->driver_priv;
+			struct pid *pid;
+
+			/*
+			 * Although we have a valid reference on file->pid, that does
+			 * not guarantee that the task_struct who called get_pid() is
+			 * still alive (e.g. get_pid(current) => fork() => exit()).
+			 * Therefore, we need to protect this ->comm access using RCU.
+			 */
+			rcu_read_lock();
+			pid = rcu_dereference(file->pid);
+			task = pid_task(pid, PIDTYPE_TGID);
+			seq_printf(m, "client_id %8llu pid %8d command %s:\n",
+				file->client_id, pid_nr(pid),
+				task ? task->comm : "<unknown>");
+			rcu_read_unlock();
+
+			ret = show(pfile, m);
+			if (ret < 0)
+				break;
+
+			seq_puts(m, "\n");
+		}
+	}
+
+	return ret;
+}
+
+static struct drm_info_list panthor_sched_debugfs_list[] = {
+	{ "sched_groups", show_each_file, 0, show_file_groups },
+};
+
+void panthor_sched_debugfs_init(struct drm_minor *minor)
+{
+	drm_debugfs_create_files(panthor_sched_debugfs_list,
+				 ARRAY_SIZE(panthor_sched_debugfs_list),
+				 minor->debugfs_root, minor);
+}
+
+#endif /* CONFIG_DEBUG_FS */
diff --git a/drivers/gpu/drm/panthor/panthor_sched.h b/drivers/gpu/drm/panthor/panthor_sched.h
index 9a8692de8ade..d87cbdba4d40 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.h
+++ b/drivers/gpu/drm/panthor/panthor_sched.h
@@ -10,6 +10,7 @@ struct drm_file;
 struct drm_gem_object;
 struct drm_sched_job;
 struct drm_memory_stats;
+struct drm_minor;
 struct drm_panthor_group_create;
 struct drm_panthor_queue_create;
 struct drm_panthor_group_get_state;
@@ -55,4 +56,8 @@ void panthor_sched_report_fw_events(struct panthor_device *ptdev, u32 events);
 
 void panthor_fdinfo_gather_group_samples(struct panthor_file *pfile);
 
+#ifdef CONFIG_DEBUG_FS
+void panthor_sched_debugfs_init(struct drm_minor *minor);
+#endif
+
 #endif

---
base-commit: 0e118b936dc5904cf0d9859882a40c96057b083d
change-id: 20260814-panthor-sched-group-prio-c987f6a15690

Best regards,
--  
Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Re: [PATCH] drm/panthor: Display priorities of panthor groups over debugfs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing `rcu_read_lock()` or `xa_lock()` around `xa_for_each_marked()` iteration leads to a use-after-free on `group`.
--

--- Patch [1]: [PATCH] drm/panthor: Display priorities of panthor groups over debugfs ---
commit 55662349e547cf65d238c177ee7f9c17e8fd597c
Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

drm/panthor: Display priorities of panthor groups over debugfs

This commit adds debugfs information for panthor groups, showing
the priority of each group. The implementation uses xa_for_each_marked
to iterate through registered groups.

> Some additional notes: I noticed panthor apparently uses
> xa_for_each{_marked} by guarding it with xa_lock/xa_unlock. That appears
> to be unnecessary, judging by the documentation of it and that nobody
> else does this.

Is this assumption accurate? While xa_for_each_marked() protects the
XArray's internal state without external locks, the returned object pointers
require either an rcu_read_lock() or xa_lock() around the entire loop to
prevent them from being freed while they are being accessed inside the loop.

> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 5832dccfc0932..44b61e946e2d4 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
[ ... ]
> @@ -4198,3 +4199,91 @@ int panthor_sched_init(struct panthor_device *ptdev)
[ ... ]
> +static int show_file_groups(struct panthor_file *pfile, struct seq_file *m)
> +{
> +	struct panthor_group *group;
> +	unsigned long i;
> +
> +	if (IS_ERR_OR_NULL(pfile->groups))
> +		return -ENOENT;
> +
> +	xa_for_each_marked(&pfile->groups->xa, i, group, GROUP_REGISTERED) {
> +		seq_printf(m, " Group %lu: priority %s\n", i,
> +			   panthor_sched_prio_str(group->priority));
> +	}

[Severity: High]
Does this code risk a use-after-free on the group pointer?

Because xa_for_each_marked() only takes the RCU read lock internally
during the lookup, the returned group pointer has no lifetime protection
once the macro yields it to the loop body.

If a user reads the sched_groups debugfs file concurrently with a group
being destroyed via the DRM_IOCTL_PANTHOR_GROUP_DESTROY ioctl, the group
could be removed and freed before its priority is accessed here.

Should this iteration be protected by an rcu_read_lock() or xa_lock() to
ensure the group pointer remains valid during the loop?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-panthor-sched-group-prio-v1-1-1836857c53e9@collabora.com?part=1

Re: [PATCH] drm/panthor: Display priorities of panthor groups over debugfs

[Boris Brezillon]

Hello Nicolas,

On Tue, 18 Aug 2026 21:35:24 +0200
Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:

> Analogous to what was added in Commit d41c79838c47 ("drm/panfrost:
> Display list of device JM contexts over debugfs") for panfrost, add
> similar debugfs information for panthor.
> 
> The group priority does not change over the lifetime of the group, so no
> effort to synchronise with the scheduler lock is being made.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
> Some additional notes: I noticed panthor apparently uses
> xa_for_each{_marked} by guarding it with xa_lock/xa_unlock. That appears
> to be unnecessary, judging by the documentation of it and that nobody
> else does this.
> ---
>  drivers/gpu/drm/panthor/panthor_drv.c   |  1 +
>  drivers/gpu/drm/panthor/panthor_sched.c | 89 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/panthor/panthor_sched.h |  5 ++
>  3 files changed, 95 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
> index 46a3080b0b20..8cdba0c1a14b 100644
> --- a/drivers/gpu/drm/panthor/panthor_drv.c
> +++ b/drivers/gpu/drm/panthor/panthor_drv.c
> @@ -1769,6 +1769,7 @@ static void panthor_debugfs_init(struct drm_minor *minor)
>  {
>  	panthor_mmu_debugfs_init(minor);
>  	panthor_gem_debugfs_init(minor);
> +	panthor_sched_debugfs_init(minor);
>  }
>  #endif
>  
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 5832dccfc093..44b61e946e2d 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0 or MIT
>  /* Copyright 2023 Collabora ltd. */
>  
> +#include <drm/drm_debugfs.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_exec.h>
>  #include <drm/drm_file.h>
> @@ -4198,3 +4199,91 @@ int panthor_sched_init(struct panthor_device *ptdev)
>  	ptdev->scheduler = sched;
>  	return 0;
>  }
> +
> +#ifdef CONFIG_DEBUG_FS
> +
> +static const char *
> +panthor_sched_prio_str(enum panthor_csg_priority prio)
> +{
> +	switch (prio) {
> +	case PANTHOR_CSG_PRIORITY_LOW:
> +		return "LOW";
> +	case PANTHOR_CSG_PRIORITY_MEDIUM:
> +		return "MEDIUM";
> +	case PANTHOR_CSG_PRIORITY_HIGH:
> +		return "HIGH";
> +	case PANTHOR_CSG_PRIORITY_RT:
> +		return "REAL-TIME";
> +	default:
> +		return "UNKNOWN";
> +	}
> +}
> +
> +static int show_file_groups(struct panthor_file *pfile, struct seq_file *m)
> +{
> +	struct panthor_group *group;
> +	unsigned long i;
> +
> +	if (IS_ERR_OR_NULL(pfile->groups))
> +		return -ENOENT;
> +
> +	xa_for_each_marked(&pfile->groups->xa, i, group, GROUP_REGISTERED) {
> +		seq_printf(m, " Group %lu: priority %s\n", i,
> +			   panthor_sched_prio_str(group->priority));
> +	}

Could this race with the GROUP_DESTROY ioctl and open the door for
potential UAFs on the group object? Seems
panthor_fdinfo_gather_group_samples() has this xa_for_each_marked()
inside an xa_lock()-ed section to cover for this.

> +
> +	return 0;
> +}
> +
> +static int show_each_file(struct seq_file *m, void *arg)
> +{
> +	struct drm_info_node *node = (struct drm_info_node *)m->private;
> +	struct drm_device *ddev = node->minor->dev;
> +	int (*show)(struct panthor_file *, struct seq_file *) =
> +		node->info_ent->data;
> +	struct drm_file *file;
> +	int ret = 0;
> +
> +	scoped_cond_guard(mutex_intr, return -EINTR, &ddev->filelist_mutex) {
> +		list_for_each_entry(file, &ddev->filelist, lhead) {
> +			struct task_struct *task;
> +			struct panthor_file *pfile = file->driver_priv;
> +			struct pid *pid;
> +
> +			/*
> +			 * Although we have a valid reference on file->pid, that does
> +			 * not guarantee that the task_struct who called get_pid() is
> +			 * still alive (e.g. get_pid(current) => fork() => exit()).
> +			 * Therefore, we need to protect this ->comm access using RCU.
> +			 */
> +			rcu_read_lock();
> +			pid = rcu_dereference(file->pid);
> +			task = pid_task(pid, PIDTYPE_TGID);
> +			seq_printf(m, "client_id %8llu pid %8d command %s:\n",
> +				file->client_id, pid_nr(pid),
> +				task ? task->comm : "<unknown>");
> +			rcu_read_unlock();

Don't we have this piece of information stored in
panthor_group::task_info already? Unless you really want to reflect
clients that have no groups, or have groups from a given client clearly
outlined in the debugfs output, I'd flatten things out and have these
client-related info printed along the group info in show_file_groups()
(pass a drm_file instead of a panthor_file, so you can get the
client_id from there).

Regards,

Boris

> +
> +			ret = show(pfile, m);
> +			if (ret < 0)
> +				break;
> +
> +			seq_puts(m, "\n");
> +		}
> +	}
> +
> +	return ret;

Re: [PATCH] drm/panthor: Display priorities of panthor groups over debugfs

[Nicolas Frattaroli]

On Monday, 24 August 2026 10:14:20 Central European Summer Time Boris Brezillon wrote:
> Hello Nicolas,
> 
> On Tue, 18 Aug 2026 21:35:24 +0200
> Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:
> 
> > Analogous to what was added in Commit d41c79838c47 ("drm/panfrost:
> > Display list of device JM contexts over debugfs") for panfrost, add
> > similar debugfs information for panthor.
> > 
> > The group priority does not change over the lifetime of the group, so no
> > effort to synchronise with the scheduler lock is being made.
> > 
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> > Some additional notes: I noticed panthor apparently uses
> > xa_for_each{_marked} by guarding it with xa_lock/xa_unlock. That appears
> > to be unnecessary, judging by the documentation of it and that nobody
> > else does this.
> > ---
> >  drivers/gpu/drm/panthor/panthor_drv.c   |  1 +
> >  drivers/gpu/drm/panthor/panthor_sched.c | 89 +++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/panthor/panthor_sched.h |  5 ++
> >  3 files changed, 95 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
> > index 46a3080b0b20..8cdba0c1a14b 100644
> > --- a/drivers/gpu/drm/panthor/panthor_drv.c
> > +++ b/drivers/gpu/drm/panthor/panthor_drv.c
> > @@ -1769,6 +1769,7 @@ static void panthor_debugfs_init(struct drm_minor *minor)
> >  {
> >  	panthor_mmu_debugfs_init(minor);
> >  	panthor_gem_debugfs_init(minor);
> > +	panthor_sched_debugfs_init(minor);
> >  }
> >  #endif
> >  
> > diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> > index 5832dccfc093..44b61e946e2d 100644
> > --- a/drivers/gpu/drm/panthor/panthor_sched.c
> > +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0 or MIT
> >  /* Copyright 2023 Collabora ltd. */
> >  
> > +#include <drm/drm_debugfs.h>
> >  #include <drm/drm_drv.h>
> >  #include <drm/drm_exec.h>
> >  #include <drm/drm_file.h>
> > @@ -4198,3 +4199,91 @@ int panthor_sched_init(struct panthor_device *ptdev)
> >  	ptdev->scheduler = sched;
> >  	return 0;
> >  }
> > +
> > +#ifdef CONFIG_DEBUG_FS
> > +
> > +static const char *
> > +panthor_sched_prio_str(enum panthor_csg_priority prio)
> > +{
> > +	switch (prio) {
> > +	case PANTHOR_CSG_PRIORITY_LOW:
> > +		return "LOW";
> > +	case PANTHOR_CSG_PRIORITY_MEDIUM:
> > +		return "MEDIUM";
> > +	case PANTHOR_CSG_PRIORITY_HIGH:
> > +		return "HIGH";
> > +	case PANTHOR_CSG_PRIORITY_RT:
> > +		return "REAL-TIME";
> > +	default:
> > +		return "UNKNOWN";
> > +	}
> > +}
> > +
> > +static int show_file_groups(struct panthor_file *pfile, struct seq_file *m)
> > +{
> > +	struct panthor_group *group;
> > +	unsigned long i;
> > +
> > +	if (IS_ERR_OR_NULL(pfile->groups))
> > +		return -ENOENT;
> > +
> > +	xa_for_each_marked(&pfile->groups->xa, i, group, GROUP_REGISTERED) {
> > +		seq_printf(m, " Group %lu: priority %s\n", i,
> > +			   panthor_sched_prio_str(group->priority));
> > +	}
> 
> Could this race with the GROUP_DESTROY ioctl and open the door for
> potential UAFs on the group object? Seems
> panthor_fdinfo_gather_group_samples() has this xa_for_each_marked()
> inside an xa_lock()-ed section to cover for this.

Hm, right, I was only considering the case where a group destruction
would happen when the drm handle is being closed anyway, which is
mutually excluded with the caller's file lock.

That being said, I'm not sure I'm in love with locking groups->xa
as a way to protect against this, but it's more fine-grained than
taking the scheduler mutex so I guess it's an improvement over that.

Looks like xa_for_each_marked only takes the RCU read lock in the
functions it calls, and I was hoping we could make a panthor-specific
variant that group_get()s the group before unlocking, but that would
be a write.

With the additional locking around the iteration, we can use
xas_for_each_marked though, but I don't think the rcu overhead for
the repeated unlocks is a problem.

This is debugfs so I shouldn't even worry.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int show_each_file(struct seq_file *m, void *arg)
> > +{
> > +	struct drm_info_node *node = (struct drm_info_node *)m->private;
> > +	struct drm_device *ddev = node->minor->dev;
> > +	int (*show)(struct panthor_file *, struct seq_file *) =
> > +		node->info_ent->data;
> > +	struct drm_file *file;
> > +	int ret = 0;
> > +
> > +	scoped_cond_guard(mutex_intr, return -EINTR, &ddev->filelist_mutex) {
> > +		list_for_each_entry(file, &ddev->filelist, lhead) {
> > +			struct task_struct *task;
> > +			struct panthor_file *pfile = file->driver_priv;
> > +			struct pid *pid;
> > +
> > +			/*
> > +			 * Although we have a valid reference on file->pid, that does
> > +			 * not guarantee that the task_struct who called get_pid() is
> > +			 * still alive (e.g. get_pid(current) => fork() => exit()).
> > +			 * Therefore, we need to protect this ->comm access using RCU.
> > +			 */
> > +			rcu_read_lock();
> > +			pid = rcu_dereference(file->pid);
> > +			task = pid_task(pid, PIDTYPE_TGID);
> > +			seq_printf(m, "client_id %8llu pid %8d command %s:\n",
> > +				file->client_id, pid_nr(pid),
> > +				task ? task->comm : "<unknown>");
> > +			rcu_read_unlock();
> 
> Don't we have this piece of information stored in
> panthor_group::task_info already? Unless you really want to reflect
> clients that have no groups, or have groups from a given client clearly
> outlined in the debugfs output, I'd flatten things out and have these
> client-related info printed along the group info in show_file_groups()
> (pass a drm_file instead of a panthor_file, so you can get the
> client_id from there).

I basically copied this over from panfrost's debugfs output. I do agree
that doing it in a flat listing without grouping them by file->pid is
simpler, since it saves us the trouble of the rcu fun here.

> 
> Regards,
> 
> Boris
> 
> > +
> > +			ret = show(pfile, m);
> > +			if (ret < 0)
> > +				break;
> > +
> > +			seq_puts(m, "\n");
> > +		}
> > +	}
> > +
> > +	return ret;
>