Re: [PATCH v7 1/8] drm/panthor: Add performance counter uAPI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/panthor: missing output handle field in PERF_SESSION_CREATE UAPI
--

commit 94c846a97bea171800cf5afb9d2812fd54ce5a02
Author: Lukas Zapolskas <lukas.zapolskas@arm.com>

drm/panthor: Add performance counter uAPI

This commit extends the DEV_QUERY ioctl and introduces the new
DRM_PANTHOR_PERF_SESSION ioctls to allow for the sampling of performance
counters. It adds several structures to map user buffers and configure
the counter sessions.

> diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> --- a/include/uapi/drm/panthor_drm.h
> +++ b/include/uapi/drm/panthor_drm.h
[ ... ]
> +/**
> + * struct drm_panthor_perf_session_create - Arguments passed to DRM_PANTHOR_PERF_SESSION_CREATE.
> + */
> +struct drm_panthor_perf_session_create {
> +	/**
> +	 * @block_set: Set of performance counter blocks, member of
> +	 *             enum drm_panthor_perf_counter_set.
> +	 *
> +	 * This is a global configuration and only one set can be active at a time. If
> +	 * another client has already requested a counter set, any further requests
> +	 * for a different counter set will fail and return an -EBUSY.
> +	 *
> +	 * If the requested set does not exist, the request will fail and return an -EINVAL.
> +	 *
> +	 * Some sets have additional requirements to be enabled, and the setup request will
> +	 * fail with an -EACCES if these requirements are not satisfied.
> +	 */
> +	__u8 block_set;
> +
> +	/** @pad: MBZ. */
> +	__u8 pad[7];
> +
> +	/** @fd: eventfd for signalling the availability of a new sample. */
> +	__u32 fd;
> +
> +	/** @ringbuf_handle: Handle to the BO to write perf counter sample to. */
> +	__u32 ringbuf_handle;
> +
> +	/**
> +	 * @control_handle: Handle to the BO containing a contiguous 16 byte range, used for the
> +	 * insert and extract indices for the ringbuffer.
> +	 */
> +	__u32 control_handle;

[Severity: High]
Does this structure need a __u32 handle output field to return the created
session identifier to userspace?

It appears that this UAPI returns the newly allocated session ID directly via
the ioctl's integer return value rather than writing it back to an output
field in this struct.

Returning positive values from ioctls can cause standard DRM wrappers, such as
libdrm's drmIoctl(), to misinterpret the positive return value as an error
code, which might render the API unusable for those clients.

Additionally, the core drm_ioctl() function prints a debug warning for any
non-zero ioctl return value. Could this cause every successful session
creation to spam the kernel debug log with false errors?

> +
> +	/**
> +	 * @sample_slots: The number of slots available in the userspace-provided BO. Must be
> +	 * a power of 2.
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011-panthor-performance-counters-v7-0-12c32495062f@arm.com?part=1