| Message ID | 20210723220248.6554-2-s-anna@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | K3 R5F & DSP IPC-only mode support | expand |
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@ti.com> > --- > 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@ti.com/ > > 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) { This is checked in rproc_stop() where -EINVAL is returned if ops::stop has not been provided. > + 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. > + 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. Conditions such as the above make the core code brittle, difficult to understand and tedious to maintain. 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 >
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@ti.com> >> --- >> 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@ti.com/ >> >> 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. > >> + 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(). 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(). 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 >>
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@ti.com> > >> --- > >> 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@ti.com/ > >> > >> 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. > > > >> + 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. > 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(). > > > 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 > >> >
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@ti.com> >>>> --- >>>> 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@ti.com/ >>>> >>>> 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. 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. 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 >>>> >>
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@ti.com> > >>>> --- > >>>> 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@ti.com/ > >>>> > >>>> 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 > >>>> > >> >
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@ti.com> >>>>>> --- >>>>>> 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@ti.com/ >>>>>> >>>>>> 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 >>>>>> >>>> >>
Hi Suman, Mathieu, On 8/4/21 9:17 PM, 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@ti.com> >>>>> --- >>>>> 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@ti.com/ >>>>> >>>>> 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. Agree With Mathieu. More than that it looks to me that the stop ops is not optional and mandatory to implement the remoteproc shutdown. It should be the role of the platform driver to decide if a stop is a detachment. This behavior may also depend on the project for multi purpose platforms. In this case DT property might be more appropriate than a null stop ops to determine the behavior. >>> >>> 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. > > 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(). In current implementation to be able to detach a remote processor we need to be in RPROC_ATTACHED state. If I well remember the reason is the backup of the resource table to reinitialize it on detach.should we improve this? We could also consider the attach and detach as 2 separate features: - attach is used for a pre loaded firmware (e.g early action use case waiting Linux boot) => can be stopped as a loaded firmware. - boot a remote firmware loaded from sysfs but detach it on Linux shutdown to allow it to property stop it own activities (e.g Linux independent reboot). >> >> 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. I'm not sure to understand what would be behind this rework. What is exactly the issue on the rproc_boot? having a semantic symmetry? In this case do you expect that the user application determines the current state of the remote processor and "start" it or "attach" it, depending on state? Or does it be implemented in rproc_sysfs and rpcroc_cdev (and few platform drivers). Look to me that take rproc_attach() out of rproc_boot() could break or complexify some legacy APIs. Regards, Arnaud > > 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. > > 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 >>>>> >>> >
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 && + !rproc->ops->stop) { + dev_err(&rproc->dev, + "stop not supported for this rproc, use detach\n"); + 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); 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)
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@ti.com> --- 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@ti.com/ 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(-)