linux-kernel - Re: [PATCH v7] remoteproc: Add device awake calls in rproc boot and shutdown path

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] [day] [month] [year] [list]

Message-ID: <ud4nodcdhxr4pux6nsvqzaclqrnmct6ehjr5bmsxdgjby5xs4k@4c23gunpbf4z>
Date: Sat, 20 Sep 2025 22:00:48 -0500
From: Bjorn Andersson <andersson@...nel.org>
To: Souradeep Chowdhury <souradeep.chowdhury@....qualcomm.com>
Cc: Mathieu Poirier <mathieu.poirier@...aro.org>, 
	linux-remoteproc@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Souradeep Chowdhury <quic_schowdhu@...cnc.com>, Mukesh Ojha <mukesh.ojha@....qualcomm.com>
Subject: Re: [PATCH v7] remoteproc: Add device awake calls in rproc boot and
 shutdown path

On Thu, Aug 28, 2025 at 02:56:15PM +0530, Souradeep Chowdhury wrote:
> If an user stops and starts rproc by using the sysfs interface, then on
> pm suspension the firmware fails to load as the request_firmware call
> under adsp_load relies on usermodehelper process via firmware_fallback_sysfs 
> which gets frozen on pm suspension.

How does it fail? Is the firmware load aborted? Does it time out while
we're suspended?

The usermodehelper is optional, adsp_load() doesn't rely on
usermodehelper, it relies on the firmware class, which might perform
usermodehelper.

Please describe how and why it fail, so that help educate others (me
included) about what the actual problem you're seeing is.

> Currently pm_awake calls are present 
> in the recovery path, add the same in start and stop path of rproc for 
> proper loading of the firmware in non-recovery path.  

I would expect/hope that the git log tells us that the pm_stay_awake()
in the recovery path is there to prevent the system from being suspended
while we're restarting the remotproc, as this is expected to lead to
functional degradation and suboptimal low power state.

"They call this function over there" is not sufficient motivation.


But just to be clear, I'm not saying that this patch is wrong. I'm
saying I don't understand your problem/motivation.

> 
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@...cnc.com>
> Signed-off-by: Souradeep Chowdhury <souradeep.chowdhury@....qualcomm.com>

This is both you, no need to carry both.

> Reviewed-by: Mukesh Ojha <mukesh.ojha@....qualcomm.com>
> ---
> Changes in v7
> 
> *Justify this fix by adding more details in commit message

Please start use b4, so we get proper links to old submissions here.
 
Regards,
Bjorn

> Changes in v6
> 
> *Add some correction to commit message
> 
> Changes in v5
> 
> *Added more details to commit description
> 
> Changes in v4
> 
> *Remove stability from mailing list
> *Remove the extra tab in v3
> *Change the commit description
> 
>  drivers/remoteproc/remoteproc_core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index c2cf0d277729..5d6c4e694b4c 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1917,6 +1917,7 @@ int rproc_boot(struct rproc *rproc)
>  		return -EINVAL;
>  	}
>  
> +	pm_stay_awake(rproc->dev.parent);
>  	dev = &rproc->dev;
>  
>  	ret = mutex_lock_interruptible(&rproc->lock);
> @@ -1961,6 +1962,7 @@ int rproc_boot(struct rproc *rproc)
>  		atomic_dec(&rproc->power);
>  unlock_mutex:
>  	mutex_unlock(&rproc->lock);
> +	pm_relax(rproc->dev.parent);
>  	return ret;
>  }
>  EXPORT_SYMBOL(rproc_boot);
> @@ -1991,6 +1993,7 @@ int rproc_shutdown(struct rproc *rproc)
>  	struct device *dev = &rproc->dev;
>  	int ret = 0;
>  
> +	pm_stay_awake(rproc->dev.parent);
>  	ret = mutex_lock_interruptible(&rproc->lock);
>  	if (ret) {
>  		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
> @@ -2027,6 +2030,7 @@ int rproc_shutdown(struct rproc *rproc)
>  	rproc->table_ptr = NULL;
>  out:
>  	mutex_unlock(&rproc->lock);
> +	pm_relax(rproc->dev.parent);
>  	return ret;
>  }
>  EXPORT_SYMBOL(rproc_shutdown);
> -- 
> 2.34.1
>