Re: [PATCH v2 1/5] remoteproc: Add support for detach-only during shutdown

[Mathieu Poirier]

On Wed, Aug 04, 2021 at 02:17:22PM -0500, Suman Anna wrote:
> Hi Mathieu,
> 
> On 8/3/21 11:23 AM, Mathieu Poirier wrote:
> > Good morning,
> > 
> > On Mon, Aug 02, 2021 at 06:21:38PM -0500, Suman Anna wrote:
> >> Hi Mathieu,
> >>
> >> On 8/2/21 1:44 PM, Mathieu Poirier wrote:
> >>> On Fri, Jul 23, 2021 at 05:02:44PM -0500, Suman Anna wrote:
> >>>> The remoteproc core has support for both stopping and detaching a
> >>>> remote processor that was attached to previously, through both the
> >>>> remoteproc sysfs and cdev interfaces. The rproc_shutdown() though
> >>>> unconditionally only uses the stop functionality at present. This
> >>>> may not be the default desired functionality for all the remoteproc
> >>>> platform drivers.
> >>>>
> >>>> Enhance the remoteproc core logic to key off the presence of the
> >>>> .stop() ops and allow the individual remoteproc drivers to continue
> >>>> to use the standard rproc_add() and rproc_del() API. This allows
> >>>> the remoteproc drivers to only do detach if supported when the driver
> >>>> is uninstalled, and the remote processor continues to run undisturbed
> >>>> even after the driver removal.
> >>>>
> >>>> Signed-off-by: Suman Anna <s-anna@xxxxxx>
> >>>> ---
> >>>> v2: Addressed various review comments from v1
> >>>>  - Reworked the logic to not use remoteproc detach_on_shutdown and
> >>>>    rely only on rproc callback ops
> >>>>  - Updated the last para of the patch description
> >>>> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20210522000309.26134-3-s-anna@xxxxxx/
> >>>>
> >>>>  drivers/remoteproc/remoteproc_cdev.c  | 7 +++++++
> >>>>  drivers/remoteproc/remoteproc_core.c  | 5 ++++-
> >>>>  drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
> >>>>  3 files changed, 17 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> >>>> index 4ad98b0b8caa..16c932beed88 100644
> >>>> --- a/drivers/remoteproc/remoteproc_cdev.c
> >>>> +++ b/drivers/remoteproc/remoteproc_cdev.c
> >>>> @@ -42,6 +42,13 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
> >>>>  		    rproc->state != RPROC_ATTACHED)
> >>>>  			return -EINVAL;
> >>>>  
> >>>> +		if (rproc->state == RPROC_ATTACHED &&
> >>>
> >>> This is already checked just above.
> >>>
> >>>> +		    !rproc->ops->stop) {
> >>
> >> Well, this is checking for both conditions, and not just the stop ops
> >> independently. We expect to have .stop() defined normally for both regular
> >> remoteproc mode and attached mode where you want to stop (and not detach), but
> >> as you can see, I am supporting only detach and so will not have .stop() defined
> >>  with RPROC_ATTACHED.
> >>
> >>>
> >>> This is checked in rproc_stop() where -EINVAL is returned if ops::stop has not
> >>> been provided.
> >>
> >> rproc_shutdown() actually doesn't return any status, so all its internal
> >> checking gets ignored and a success is returned today.
> >>
> > 
> > That is correct, and I have suggested to add a return value in my previous
> > review.
> 
> Yeah ok. I can add a separate patch fixing that, and couple of these checks then
> become redundant.
> 
> > 
> >>>
> >>>> +			dev_err(&rproc->dev,
> >>>> +				"stop not supported for this rproc, use detach\n");
> >>>
> >>> The standard error message from the shell should be enough here, the same way it
> >>> is enough when the "start" and "stop" scenarios fail.
> >>
> >> Thought this was a bit more informative, but sure this trace can be dropped.
> >>
> >>>
> >>>> +			return -EINVAL;
> >>>> +		}
> >>>> +
> >>>>  		rproc_shutdown(rproc);
> >>>>  	} else if (!strncmp(cmd, "detach", len)) {
> >>>>  		if (rproc->state != RPROC_ATTACHED)
> >>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> >>>> index 7de5905d276a..ab9e52180b04 100644
> >>>> --- a/drivers/remoteproc/remoteproc_core.c
> >>>> +++ b/drivers/remoteproc/remoteproc_core.c
> >>>> @@ -2075,7 +2075,10 @@ void rproc_shutdown(struct rproc *rproc)
> >>>>  	if (!atomic_dec_and_test(&rproc->power))
> >>>>  		goto out;
> >>>>  
> >>>> -	ret = rproc_stop(rproc, false);
> >>>> +	if (rproc->state == RPROC_ATTACHED && !rproc->ops->stop)
> >>>> +		ret = __rproc_detach(rproc);
> >>>> +	else
> >>>> +		ret = rproc_stop(rproc, false);
> >>>
> >>> As I indicated in my last review I think rproc_shutdown() and rproc_del() should
> >>> be decoupled and the right call made in the platform drivers based on the state
> >>> of the remote processor.  
> >>
> >> We have various remoteproc API provided in pairs - rproc_alloc()/rproc_free(),
> >> rproc_add()/rproc_del(), rproc_boot()/rproc_shutdown() and
> >> rproc_attach()/rproc_detach(). The drivers are configuring conditions for
> >> auto-boot and RPROC_DETACHED. The reason they are coupled is primarily because
> >> of the auto-boot done during rproc_add(). And we handle the RPROC_DETACHED case
> >> just as well in rproc_boot().
> >>
> > 
> > The difference with rproc_boot() is that we are checking only the state of the
> > remoteproc, everything else related to the remote processor operations is
> > seamlessly handles by the state machine.  It is also tied to the
> > rproc_trigger_auto_boot() mechanic - decoupling that would be messy without
> > bringing any advantages other than keeping with a semantic symmetry.
> 
> Most of this is actually tied to auto_boot if you think about it, not just the
> rproc state. If we have auto_boot set to false, then rproc_add() would not do
> anything, and the decision to start or attach can either be done through the
> sysfs/cdev or a kernel remoteproc or some consumer driver. And the state machine
> is getting influenced by this flag. auto-boot is a very useful feature.
> 
> You are asking is to do things differently between the regular start/stop case
> and attach/detach case ignoring the auto-boot. The semantic symmetry actually
> makes it easier to follow the state machine given that there are some internal
> reference counts as well.

I am definitely not asking to ignore the auto-boot flag.  All I said is that I
did not split the semantic in rproc_boot() because of the auto-boot flag and the
mechanic to handle it.

> 
> Note that we also have the devres API, and rproc_alloc()/rproc_free() and
> rproc_add()/rproc_del() form the main remoteproc subsystem API. The drivers
> would end up using matching calls if we don't have auto_boot.
> 
> > 
> >> While what you have suggested works, but I am not quite convinced on this
> >> asymmetric usage, and why this state-machine logic should be split between the
> >> core and remoteproc drivers differently between attach and detach. To me,
> >> calling rproc_detach() in remoteproc drivers would have made sense only if they
> >> are also calling rproc_attach().
> > 
> > As pointed out above I see rproc_boot() as a special case but if that really
> > concerns you I'm open to consider patches that will take rproc_attach() out of
> > rproc_boot(). 
> > 
> 
> We are talking about a bigger behavioral change to remoteproc core here. So I
> would definitely want to hear from others as well on this before we spend any
> time reworking code.
> 
> Meanwhile, how do I take this series forward? One option I can probably do is
> turn off auto-boot for early-boot case in my drivers and do the matching
> attach/detach.
>

I don't think there is a need to turn off auto-boot for early boot, rproc_boot()
will to the right thing.

As for the way forward, the easiest way I see is to call either rproc_shutdown()
or rproc_detach() based on rproc->state in rproc_del().  That will work with
devm_rproc_remove() and it is still possible for platorm drivers to explicitly
call rproc_shutdown() before rproc_del() to force a remote processor that was
attached to be switched off when the driver is removed.

That is all the time I had for remoteproc - I am officially away for the next two weeks.  

Thanks,
Mathieu

> regards
> Suman
> 
> >>
> >>
> >> Conditions such as the above make the core code
> >>> brittle, difficult to understand and tedious to maintain.
> >>
> >> The logic I have added actually makes rproc_shutdown behavior to be on par with
> >> the rproc_boot().
> >>
> >> regards
> >> Suman
> >>
> >>>
> >>> Thanks,
> >>> Mathieu
> >>>
> >>>>  	if (ret) {
> >>>>  		atomic_inc(&rproc->power);
> >>>>  		goto out;
> >>>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> >>>> index ea8b89f97d7b..133e766f38d4 100644
> >>>> --- a/drivers/remoteproc/remoteproc_sysfs.c
> >>>> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> >>>> @@ -206,6 +206,12 @@ static ssize_t state_store(struct device *dev,
> >>>>  		    rproc->state != RPROC_ATTACHED)
> >>>>  			return -EINVAL;
> >>>>  
> >>>> +		if (rproc->state == RPROC_ATTACHED &&
> >>>> +		    !rproc->ops->stop) {
> >>>> +			dev_err(&rproc->dev, "stop not supported for this rproc, use detach\n");
> >>>> +			return -EINVAL;
> >>>> +		}
> >>>> +
> >>>>  		rproc_shutdown(rproc);
> >>>>  	} else if (sysfs_streq(buf, "detach")) {
> >>>>  		if (rproc->state != RPROC_ATTACHED)
> >>>> -- 
> >>>> 2.32.0
> >>>>
> >>
>