linux-kernel - Re: [PATCH v9 4/4] drm/panthor: show device-wide list of DRM GEM objects over DebugFS

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: <tb6267pfhg25zpoaqyzxhw7jxzzmncjgxa5v6czimhdu6xaabu@ygv4z7ibtmxu>
Date: Fri, 18 Apr 2025 21:22:59 +0100
From: Adrián Larumbe <adrian.larumbe@...labora.com>
To: Boris Brezillon <boris.brezillon@...labora.com>
Cc: linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org, 
	kernel@...labora.com, Liviu Dudau <liviu.dudau@....com>, 
	Steven Price <steven.price@....com>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, 
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, 
	Sumit Semwal <sumit.semwal@...aro.org>, Christian König <christian.koenig@....com>, 
	linux-media@...r.kernel.org, linaro-mm-sig@...ts.linaro.org
Subject: Re: [PATCH v9 4/4] drm/panthor: show device-wide list of DRM GEM
 objects over DebugFS

On 18.04.2025 10:26, Boris Brezillon wrote:
On Fri, 18 Apr 2025 10:11:56 +0200
Boris Brezillon <boris.brezillon@...labora.com> wrote:

> On Fri, 18 Apr 2025 03:27:07 +0100
> Adrián Larumbe <adrian.larumbe@...labora.com> wrote:
>
> > +	static const char * const gem_state_flags_names[] = {
> > +		[PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED] = "imported",

> FYI, the compiler seems to be happy with:
>
> 		[ffs(PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED) - 1] = "imported",
>
> but I'm not sure we want to fix it this way. The other
> option would be to define bit pos in the enum and then
> define flags according to these bit pos:
>
> enum panthor_debugfs_gem_state_flags {
> 	PANTHOR_DEBUGFS_GEM_STATE_IMPORTED_BIT = 0,
> 	PANTHOR_DEBUGFS_GEM_STATE_EXPORTED_BIT = 1,
>
> 	/** @PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED: GEM BO is PRIME imported. */
> 	PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED = BIT(PANTHOR_DEBUGFS_GEM_STATE_IMPORTED_BIT),
>
> 	/** @PANTHOR_DEBUGFS_GEM_STATE_FLAG_EXPORTED: GEM BO is PRIME exported. */
> 	PANTHOR_DEBUGFS_GEM_STATE_FLAG_EXPORTED = BIT(PANTHOR_DEBUGFS_GEM_STATE_EXPORTED_BIT),
> };

I'm happy with this solution too. To be frank I thought of something like this
when I realised flag names array indices an bit values were askew by one, but
since the names array is constant and mostly meant to be read in the future by
programmers who want to add new flag meanings to the same table, I didn't think
of having a hole in index 0 as a problem.

> > +		[PANTHOR_DEBUGFS_GEM_STATE_FLAG_EXPORTED] = "exported",
>
> Okay, I think I know where the flag indexing issue comes from:
> PANTHOR_DEBUGFS_GEM_STATE_FLAG_xx are flags, not bit positions, so we
> can't use them as indices here.
>
> > +	};
> > +
> > +	static const char * const gem_usage_flags_names[] = {
> > +		[PANTHOR_DEBUGFS_GEM_USAGE_FLAG_KERNEL] = "kernel",
> > +		[PANTHOR_DEBUGFS_GEM_USAGE_FLAG_FW_MAPPED] = "fw-mapped",
>
> Same problem here.
>
> > +	};
> > +


Adrian Larumbe