linux-kernel - Re: [PATCH v1 04/10] drm/panthor: Add architecture-specific function operations

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: <91b406f2-7221-49f9-89fd-6f3b6bd1f4f5@arm.com>
Date: Mon, 20 Oct 2025 10:10:47 +0100
From: Steven Price <steven.price@....com>
To: Karunika Choo <karunika.choo@....com>, dri-devel@...ts.freedesktop.org
Cc: nd@....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>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 04/10] drm/panthor: Add architecture-specific function
 operations

On 14/10/2025 10:43, Karunika Choo wrote:
> Introduce architecture-specific function pointers to support
> architecture-dependent behaviours. This patch adds the following
> function pointers and updates their usage accordingly:
> 
> - soft_reset
> - l2_power_on
> - l2_power_off
> 
> Signed-off-by: Karunika Choo <karunika.choo@....com>
> ---
>  drivers/gpu/drm/panthor/panthor_device.c |  4 ++--
>  drivers/gpu/drm/panthor/panthor_fw.c     |  5 +++--
>  drivers/gpu/drm/panthor/panthor_gpu.c    | 13 ++++++++++---
>  drivers/gpu/drm/panthor/panthor_gpu.h    |  1 +
>  drivers/gpu/drm/panthor/panthor_hw.c     |  9 ++++++++-
>  drivers/gpu/drm/panthor/panthor_hw.h     | 23 +++++++++++++++++++++++
>  6 files changed, 47 insertions(+), 8 deletions(-)
> 
<snip>
> diff --git a/drivers/gpu/drm/panthor/panthor_hw.h b/drivers/gpu/drm/panthor/panthor_hw.h
> index 7a191e76aeec..5a4e4aad9099 100644
> --- a/drivers/gpu/drm/panthor/panthor_hw.h
> +++ b/drivers/gpu/drm/panthor/panthor_hw.h
> @@ -20,12 +20,35 @@ enum panthor_hw_feature {
>  };
>  
>  
> +/**
> + * struct panthor_hw_ops - HW operations that are specific to a GPU
> + */
> +struct panthor_hw_ops {
> +	/** @soft_reset: Soft reset function pointer */
> +	int (*soft_reset)(struct panthor_device *ptdev);
> +#define panthor_hw_soft_reset(__ptdev) \
> +	((__ptdev)->hw->ops.soft_reset ? (__ptdev)->hw->ops.soft_reset(__ptdev) : 0)
> +
> +	/** @l2_power_off: L2 power off function pointer */
> +	int (*l2_power_off)(struct panthor_device *ptdev);
> +#define panthor_hw_l2_power_off(__ptdev) \
> +	((__ptdev)->hw->ops.l2_power_off ? (__ptdev)->hw->ops.l2_power_off(__ptdev) : 0)
> +
> +	/** @l2_power_on: L2 power on function pointer */
> +	int (*l2_power_on)(struct panthor_device *ptdev);
> +#define panthor_hw_l2_power_on(__ptdev) \
> +	((__ptdev)->hw->ops.l2_power_on ? (__ptdev)->hw->ops.l2_power_on(__ptdev) : 0)
> +};

Minor comments:

 * You are defining these to have a return value, but you haven't
updated any of the call-sites to deal with a failure (the return value
is ignored). This is actually an existing problem, but AFAICT the new
_pwr_ versions have more error codes which are simply getting thrown away.

 * Is there a good reason why we need to support these functions being
NULL? It seems unlikely to be useful, and TBH I'd prefer to just assign
a dummy (empty) function in those cases.

 * A static inline function would be neater and would avoid any
potential issues from the multiple evaluation of __ptdev.

Thanks,
Steve

> +
>  /**
>   * struct panthor_hw - GPU specific register mapping and functions
>   */
>  struct panthor_hw {
>  	/** @features: Bitmap containing panthor_hw_feature */
>  	DECLARE_BITMAP(features, PANTHOR_HW_FEATURES_END);
> +
> +	/** @ops: Panthor HW specific operations */
> +	struct panthor_hw_ops ops;
>  };
>  
>  int panthor_hw_init(struct panthor_device *ptdev);