linux-kernel - Re: [PATCH v5] drm: Add UAPI for the Asahi driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z-VhSaE8nopGy0e-@blossom>
Date: Thu, 27 Mar 2025 10:31:37 -0400
From: Alyssa Rosenzweig <alyssa@...enzweig.io>
To: Janne Grunau <j@...nau.net>
Cc: David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Faith Ekstrand <faith.ekstrand@...labora.com>,
	Sven Peter <sven@...npeter.dev>, Jonathan Corbet <corbet@....net>,
	Sergio Lopez Pascual <slp@...rega.org>,
	Ryan Houdek <sonicadvance1@...il.com>, linux-kernel@...r.kernel.org,
	dri-devel@...ts.freedesktop.org, rust-for-linux@...r.kernel.org,
	asahi@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
	linux-doc@...r.kernel.org, Asahi Lina <lina@...hilina.net>,
	Simona Vetter <simona.vetter@...ll.ch>
Subject: Re: [PATCH v5] drm: Add UAPI for the Asahi driver

> > +/**
> > + * struct drm_asahi_params_global - Global parameters.
> > + *
> > + * This struct may be queried by drm_asahi_get_params.
> > + */
> > +struct drm_asahi_params_global {
> > +	/** @features: Feature bits from drm_asahi_feature */
> > +	__u64 features;
> > +
> > +	/** @gpu_generation: GPU generation, e.g. 13 for G13G */
> > +	__u32 gpu_generation;
> > +
> > +	/** @gpu_variant: GPU variant as a character, e.g. 'G' for G13G */
> > +	__u32 gpu_variant;
> 
> nit: the example can avoid the duplication of 'G' with "e.g. 'C' for
> G13C"

done in v6, thanks.

> > +/**
> > + * struct drm_asahi_get_params - Arguments passed to DRM_IOCTL_ASAHI_GET_PARAMS
> > + */
> > +struct drm_asahi_get_params {
> > +	/** @param_group: Parameter group to fetch (MBZ) */
> > +	__u32 param_group;
> > +
> > +	/** @pad: MBZ */
> > +	__u32 pad;
> > +
> > +	/** @pointer: User pointer to write parameter struct */
> > +	__u64 pointer;
> > +
> > +	/** @size: Size of user buffer, max size supported on return */
> > +	__u64 size;
> 
> The comment is misleading in the case of newer / extended kernel which
> supports a larger size than supplied. You could change it to "size
> written on return" or clarify that the value on return will not exceed
> the input value.

fixed.

> > +struct drm_asahi_vm_create {
> > +	/**
> > +	 * @kernel_start: Start of the kernel-reserved address range. See
> > +	 * drm_asahi_params_global::vm_kernel_min_size.
> > +	 *
> > +	 * Both @kernel_start and @kernel_end must be within the range of
> > +	 * valid VAs given by drm_asahi_params_global::vm_user_start and
> > +	 * drm_asahi_params_global::vm_user_end. The size of the kernel range
> 
> This reads a little strange. Would it make sense to rename drm_asahi_params_global's
> vm_user_start and vm_user_end to vm_start/vm_end?

Good point, renamed.

> > +	/**
> > +	 * @bind: Union holding the bind request.
> > +	 *
> > +	 * This union is named to make the Rust bindings nicer to work with.
> > +	 */
> 
> This comment could use a short justification why this union does not
> defeat extensibility after the initial statement that "structures should
> not contain unions"

Added.

> > +	/**
> > +	 * @syncs: An optional array of drm_asahi_sync. First @in_sync_count
> > +	 * in-syncs then @out_sync_count out-syncs.
> > +	 */
> > +     __u64 syncs;
> 
> Would it make sense to explictly state that this is a pointer?

Done.

> Reviewed-by: Janne Grunau <j@...nau.net>

Thanks!