linux-kernel - Re: [PATCH v1 07/10] drm/panthor: Implement soft and fast reset via PWR

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: <7be294e2-e6fe-4c47-8bf3-507443e3b1d5@arm.com>
Date: Mon, 20 Oct 2025 12:24:31 +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 07/10] drm/panthor: Implement soft and fast reset via
 PWR_CONTROL

On 14/10/2025 10:43, Karunika Choo wrote:
> Add helpers to issue reset commands through the PWR_CONTROL interface
> and wait for reset completion using IRQ signaling. This enables support
> for both RESET_SOFT and RESET_FAST operations with timeout handling and
> status verification.
> 
> Signed-off-by: Karunika Choo <karunika.choo@....com>
> ---
>  drivers/gpu/drm/panthor/panthor_pwr.c | 62 ++++++++++++++++++++++++++-
>  drivers/gpu/drm/panthor/panthor_pwr.h |  2 +
>  2 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_pwr.c b/drivers/gpu/drm/panthor/panthor_pwr.c
> index 594181aba847..ecb278824d06 100644
> --- a/drivers/gpu/drm/panthor/panthor_pwr.c
> +++ b/drivers/gpu/drm/panthor/panthor_pwr.c
> @@ -3,6 +3,7 @@
>  
>  #include <linux/platform_device.h>
>  #include <linux/interrupt.h>
> +#include <linux/cleanup.h>
>  #include <linux/iopoll.h>
>  #include <linux/wait.h>
>  
> @@ -31,6 +32,8 @@
>  
>  #define PWR_RETRACT_TIMEOUT_US		2000
>  
> +#define PWR_RESET_TIMEOUT_MS		500
> +
>  /**
>   * struct panthor_pwr - PWR_CONTROL block management data.
>   */
> @@ -80,6 +83,42 @@ static void panthor_pwr_write_command(struct panthor_device *ptdev, u32 command,
>  	gpu_write(ptdev, PWR_COMMAND, command);
>  }
>  
> +static bool reset_irq_raised(struct panthor_device *ptdev)
> +{
> +	return gpu_read(ptdev, PWR_INT_RAWSTAT) & PWR_IRQ_RESET_COMPLETED;
> +}
> +
> +static bool reset_completed(struct panthor_device *ptdev)
> +{
> +	return (ptdev->pwr->pending_reqs & PWR_IRQ_RESET_COMPLETED);
> +}
> +
> +static int panthor_pwr_reset(struct panthor_device *ptdev, u32 reset_cmd)
> +{
> +	scoped_guard(spinlock_irqsave, &ptdev->pwr->reqs_lock) {
> +		if (!drm_WARN_ON(&ptdev->base, !reset_completed(ptdev))) {
> +			ptdev->pwr->pending_reqs |= PWR_IRQ_RESET_COMPLETED;
> +			gpu_write(ptdev, PWR_INT_CLEAR, PWR_IRQ_RESET_COMPLETED);
> +			panthor_pwr_write_command(ptdev, reset_cmd, 0);
> +		}

This would be easier to read as:

if (reset_completed(ptdev)) {
	....
} else {
	drm_WARN(&ptdev->base, 1, "Hey, we're already resetting?");
}

[Message modified to taste ;) ]

I'm also wondering if things would be easier to read if you switched
from reset_completed() to reset_pending(). Certainly here it's the
'pending' test you are trying to do.

> +	}
> +
> +	if (!wait_event_timeout(ptdev->pwr->reqs_acked, reset_completed(ptdev),
> +				msecs_to_jiffies(PWR_RESET_TIMEOUT_MS))) {
> +		guard(spinlock_irqsave)(&ptdev->pwr->reqs_lock);
> +
> +		if (!reset_completed(ptdev) && !reset_irq_raised(ptdev)) {
> +			drm_err(&ptdev->base, "RESET_%s timed out",
> +				reset_cmd == PWR_COMMAND_RESET_SOFT ? "SOFT" : "FAST");
> +			return -ETIMEDOUT;
> +		}
> +
> +		ptdev->pwr->pending_reqs &= ~PWR_IRQ_RESET_COMPLETED;
> +	}
> +
> +	return 0;
> +}
> +
>  static const char *get_domain_name(u8 domain)
>  {
>  	switch (domain) {
> @@ -407,9 +446,30 @@ int panthor_pwr_init(struct panthor_device *ptdev)
>  	return 0;
>  }
>  
> +int panthor_pwr_reset_fast(struct panthor_device *ptdev)
> +{
> +	if (!ptdev->pwr)
> +		return 0;
> +
> +	if (!(panthor_pwr_read_status(ptdev) & PWR_STATUS_ALLOW_FAST_RESET)) {
> +		drm_err(&ptdev->base, "RESET_SOFT not allowed");

Copy/paste mistake on the error message.

> +		return -EOPNOTSUPP;
> +	}
> +
> +	return panthor_pwr_reset(ptdev, PWR_COMMAND_RESET_FAST);
> +}

I can't actually find a caller of this function within the series.

> +
>  int panthor_pwr_reset_soft(struct panthor_device *ptdev)
>  {
> -	return 0;
> +	if (!ptdev->pwr)
> +		return 0;

When would this happen? Is this not a programming error?

Thanks,
Steve

> +
> +	if (!(panthor_pwr_read_status(ptdev) & PWR_STATUS_ALLOW_SOFT_RESET)) {
> +		drm_err(&ptdev->base, "RESET_SOFT not allowed");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return panthor_pwr_reset(ptdev, PWR_COMMAND_RESET_SOFT);
>  }
>  
>  int panthor_pwr_l2_power_off(struct panthor_device *ptdev)
> diff --git a/drivers/gpu/drm/panthor/panthor_pwr.h b/drivers/gpu/drm/panthor/panthor_pwr.h
> index a4042c125448..2301c26dab86 100644
> --- a/drivers/gpu/drm/panthor/panthor_pwr.h
> +++ b/drivers/gpu/drm/panthor/panthor_pwr.h
> @@ -10,6 +10,8 @@ void panthor_pwr_unplug(struct panthor_device *ptdev);
>  
>  int panthor_pwr_init(struct panthor_device *ptdev);
>  
> +int panthor_pwr_reset_fast(struct panthor_device *ptdev);
> +
>  int panthor_pwr_reset_soft(struct panthor_device *ptdev);
>  
>  int panthor_pwr_l2_power_on(struct panthor_device *ptdev);