On 8/5/21 12:35 PM, Mathieu Poirier wrote: > 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. Yeah, I can make these modification. Let me spin a v3 with this, most probably waiting for you when you come back :) regards Suman > > 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 >>>>>> >>>> >>
