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 > >>>> > >> >
