Re: [PATCH v3 1/2] remoteproc: core: full attach detach during recovery

["Shah, Tanmay" <tanmays@amd.com>]

On 2/25/2026 5:42 PM, Bjorn Andersson wrote:
> On Mon, Feb 23, 2026 at 03:43:56PM -0600, Shah, Tanmay wrote:
>> Hello,
>>
>> Thank you for the reviews. My response below:
>>
>> On 2/23/2026 1:27 PM, Bjorn Andersson wrote:
>>> On Mon, Feb 23, 2026 at 10:50:05AM -0800, Tanmay Shah wrote:
>>>> Current attach on recovery mechanism loads the clean resource table
>>>> during recovery, but doesn't re-allocate the resources. RPMsg
>>>> communication will fail after recovery due to this. Fix this
>>>> incorrect behavior by doing the full detach and attach of remote
>>>> processor during the recovery. This will load the clean resource table
>>>> and re-allocate all the resources, which will set up correct vring
>>>> information in the resource table.
>>>>
>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>>> ---
>>>>
>>>> Changes in v3:
>>>>  - both rproc_attach_recovery() and
>>>>    rproc_boot_recovery() are called the same way.
>>>>  - remove unrelated changes
>>>>
>>>> Changes in v2:
>>>>  - use rproc_boot instead of rproc_attach
>>>>  - move debug message early in the function
>>>>
>>>>  drivers/remoteproc/remoteproc_core.c | 33 +++++++++++-----------------
>>>>  1 file changed, 13 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>>>> index aada2780b343..790ad7c6d12e 100644
>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>> @@ -1777,11 +1777,11 @@ static int rproc_attach_recovery(struct rproc *rproc)
>>>>  {
>>>>  	int ret;
>>>>  
>>>> -	ret = __rproc_detach(rproc);
>>>> +	ret = rproc_detach(rproc);
>>>>  	if (ret)
>>>>  		return ret;
>>>>  
>>>> -	return __rproc_attach(rproc);
>>>> +	return rproc_boot(rproc);
>>>>  }
>>>>  
>>>>  static int rproc_boot_recovery(struct rproc *rproc)
>>>> @@ -1790,10 +1790,14 @@ static int rproc_boot_recovery(struct rproc *rproc)
>>>>  	struct device *dev = &rproc->dev;
>>>>  	int ret;
>>>>  
>>>> -	ret = rproc_stop(rproc, true);
>>>> +	ret = mutex_lock_interruptible(&rproc->lock);
>>>>  	if (ret)
>>>>  		return ret;
>>>>  
>>>> +	ret = rproc_stop(rproc, true);
>>>> +	if (ret)
>>>> +		goto unlock_mutex;
>>>> +
>>>>  	/* generate coredump */
>>>>  	rproc->ops->coredump(rproc);
>>>>  
>>>> @@ -1801,7 +1805,7 @@ static int rproc_boot_recovery(struct rproc *rproc)
>>>>  	ret = request_firmware(&firmware_p, rproc->firmware, dev);
>>>>  	if (ret < 0) {
>>>>  		dev_err(dev, "request_firmware failed: %d\n", ret);
>>>> -		return ret;
>>>> +		goto unlock_mutex;
>>>>  	}
>>>>  
>>>>  	/* boot the remote processor up again */
>>>> @@ -1809,6 +1813,8 @@ static int rproc_boot_recovery(struct rproc *rproc)
>>>>  
>>>>  	release_firmware(firmware_p);
>>>>  
>>>> +unlock_mutex:
>>>> +	mutex_unlock(&rproc->lock);
>>>>  	return ret;
>>>>  }
>>>>  
>>>> @@ -1827,26 +1833,13 @@ static int rproc_boot_recovery(struct rproc *rproc)
>>>>  int rproc_trigger_recovery(struct rproc *rproc)
>>>>  {
>>>>  	struct device *dev = &rproc->dev;
>>>> -	int ret;
>>>> -
>>>> -	ret = mutex_lock_interruptible(&rproc->lock);
>>>> -	if (ret)
>>>> -		return ret;
>>>> -
>>>> -	/* State could have changed before we got the mutex */
>>>> -	if (rproc->state != RPROC_CRASHED)
>>>> -		goto unlock_mutex;
>>>>  
>>>>  	dev_err(dev, "recovering %s\n", rproc->name);
>>>>  
>>>>  	if (rproc_has_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY))
>>>> -		ret = rproc_attach_recovery(rproc);
>>>> +		return rproc_attach_recovery(rproc);
>>>
>>> rproc_trigger_recovery() can be called either from scheduled work or
>>> directly from the debugfs/sysfs interface, it doesn't seem safe to me to
>>> call rproc_attach_recovery() without ensuring mutual exclusion between
>>> multiple parallel callers.
>>>
>>
>> I think mutual exclusion is still maintained.
>>
>>> In fact, I can see the relationship between the commit message and the
>>> changes in rproc_attach_recovery() and rproc_detach(), but I'm not sure
>>> why you need to change rproc_boot_recovery() and
>>> rproc_trigger_recovery(). Perhaps you're just missing some explanation
>>> in the commit message?
>>>
>>
>> Here, I am refactoring how lock is used and that is why I have to modify
>> rproc_trigger_recovery() and rproc_boot_recovery().
>>
>> Before:
>>
>> rproc_trigger_recovery() -> lock() -> __rproc_detach() /
>> rproc_boot_recovery() -> unlock()
>>
>> Now, __rproc_detach is replaced with rproc_detach(), which already has
>> mutual exclusion implemented within the call.
>>
>> After:
>>
>> 1) for attach recovery
>> rproc_trigger_recovery() -> rproc_attach_recovery() -> rproc_detach() ->
>> lock() -> ... -> unlock() -> rproc_boot() -> lock() ... -> unlock()
>>
> 
> The concern I had was that we're letting others execute inbetween
> rproc_detach() and rproc_boot(). But perhaps that's okay?
> 
>> 2) To call rproc_attach_recovery() and rproc_boot_recovery() in the same
>> manner, I modified rproc_boot_recovery() and introduced mutual exclusion
>> around it.
>>
> 
> No, in rproc_boot_recovery() you're locking around stop and start. In
> rproc_attach_recovery() you're locking within each part.
> 
> Looking more at this, I dislike the asymmetry between rproc_detach() vs
> rproc_boot(), and how I presume this then relies on rproc_boot() mostly
> just calling rproc_attach().
> 
> This seems to come from the fact that rproc_attach() and rproc_start()
> aren't symmetrical and rproc_detach() and rproc_stop() aren't.
> 
> It would be good if we could get this lined up, so that it's not so hard
> to reason about the different code paths through the core.
> 
> 
> Regardless of this though, the removal of the check for state !=
> RPROC_CRASHED (under a lock) means that inbetween
> rproc_crash_handler_work() and rproc_trigger_recovery() someone can
> write "recover" into the "recovery" debugfs entry and we will recover
> twice.
> 

Okay, to address this concern I will maintain original code in the
rproc_trigger_reocvery().

In, rproc_attach_recovery() I won't be calling rproc_detach() and
rproc_boot(). Instead I will programming sequence needed from
rproc_detach(), and then call rproc_attach() directly. This will help
maintain mutual exclusion same as before during the recovery process.

This will duplicate some code, but that can be refactored later, when
asymmetry of rproc_detach() vs rproc_stop() will be fixed.

I hope that's fine. I am open to any other proposal as well.

If you prefer to see v4, before further reviews let me know.

Thanks,
Tanmay


> Regards,
> Bjorn
> 
>> If you prefer, I can add commit message explaining this change. This is
>> only refactoring of the code and no new feature though.
>> Let me know if something is still missing in the implementation or in
>> the above explanation.
>>
>> Thank You,
>> Tanmay
>>
>>> Regards,
>>> Bjorn
>>>
>>>>  	else
>>>> -		ret = rproc_boot_recovery(rproc);
>>>> -
>>>> -unlock_mutex:
>>>> -	mutex_unlock(&rproc->lock);
>>>> -	return ret;
>>>> +		return rproc_boot_recovery(rproc);
>>>>  }
>>>>  
>>>>  /**
>>>> @@ -2057,7 +2050,7 @@ int rproc_detach(struct rproc *rproc)
>>>>  		return ret;
>>>>  	}
>>>>  
>>>> -	if (rproc->state != RPROC_ATTACHED) {
>>>> +	if (rproc->state != RPROC_ATTACHED && rproc->state != RPROC_CRASHED) {
>>>>  		ret = -EINVAL;
>>>>  		goto out;
>>>>  	}
>>>> -- 
>>>> 2.34.1
>>>>
>>