linux-kernel - Re: [PATCH 00/10] drm/panthor: minor AS

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: <04c19e70-d895-45ff-b221-974abaa8dff8@arm.com>
Date: Thu, 2 Oct 2025 11:48:43 +0100
From: Steven Price <steven.price@....com>
To: Chia-I Wu <olvaffe@...il.com>,
 Boris Brezillon <boris.brezillon@...labora.com>,
 Liviu Dudau <liviu.dudau@....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>,
 Grant Likely <grant.likely@...aro.org>, Heiko Stuebner <heiko@...ech.de>,
 dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 00/10] drm/panthor: minor AS_CONTROL clean up

On 16/09/2025 22:08, Chia-I Wu wrote:
> This series performs minor AS_CONTROL clean up.
> 
> Patch 1 to 5 rename and document AS_CONTROL config functions. There is
> no functional change. All functions are now prefixed by mmu_hw_ for
> consistency. All of them also expect locking. I choose not to suffix
> them by _locked, but I can be convinced.
> 
> Patch 6 to 7 eliminiate redundant mmu_hw_wait_ready. This is the main
> functional change of the series. panthor_vm_flush_range no longer waits
> for UNLOCK to complete.
> 
> Patch 8 to 10 give mmu_hw_flush_caches final touches, to improve error
> handling, simplifying code, etc.

I think you need to provide better justification for these changes. Some
of them might make some sense, but in general most of the "cleanup"
patches by themselves seem to make the code harder to read. Which can be
fine if they are a precursor to achieving an improvement in a following
patch, but as things stand I'm having a hard time to figure out what the
benefit is.

The cover letter implies that we have redundant mmu_hw_wait_ready calls
(which I can believe). But we need a proper justification on why they
are redundant, and proper patch descriptions for the precursor patches
so that anyone coming to them in the future can understand why they were
applied (without having to hunt through mail archives for the cover
letter, or guess from the later patches).

Having said the above, I do appreciate the time you took to write the
documentation blocks - we do have a bunch of fairly confusing functions.

Thanks,
Steve

> Chia-I Wu (10):
>   drm/panthor: rename and document wait_ready
>   drm/panthor: rename and document lock_region
>   drm/panthor: add mmu_hw_cmd_unlock
>   drm/panthor: add mmu_hw_cmd_update
>   drm/panthor: rename and document mmu_hw_do_operation_locked
>   drm/panthor: remove write_cmd
>   drm/panthor: remove unnecessary mmu_hw_wait_ready calls
>   drm/panthor: improve error handling for mmu_hw_flush_caches
>   drm/panthor: move size check to mmu_hw_flush_caches
>   drm/panthor: simplify mmu_hw_flush_caches
> 
>  drivers/gpu/drm/panthor/panthor_mmu.c | 157 +++++++++++++++-----------
>  1 file changed, 94 insertions(+), 63 deletions(-)
>