lkml.org 
[lkml]   [2017]   [Dec]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v4 3/5] remoteproc: Pass type of shutdown to subdev remove
    From
    Date


    On 12/06/2017 10:53 PM, Bjorn Andersson wrote:
    > On Wed 06 Dec 00:55 PST 2017, Arnaud Pouliquen wrote:
    >
    >> Hello,
    >>
    >> I saw your new version but i 'm answering to this one to continue
    >> discussion.
    >>
    >
    > That's fine.
    >
    >> On 12/05/2017 06:17 PM, Bjorn Andersson wrote:
    >>> On Tue 05 Dec 02:54 PST 2017, Arnaud Pouliquen wrote:
    >>>> On 12/05/2017 07:46 AM, Bjorn Andersson wrote:
    >>>>> On Fri 01 Dec 06:50 PST 2017, Arnaud Pouliquen wrote:
    >>>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
    >>>>>>> index 44e630eb3d94..20a9467744ea 100644
    >>>>>>> --- a/include/linux/remoteproc.h
    >>>>>>> +++ b/include/linux/remoteproc.h
    >>>>>>> @@ -456,7 +456,7 @@ struct rproc_subdev {
    >>>>>>> struct list_head node;
    >>>>>>>
    >>>>>>> int (*probe)(struct rproc_subdev *subdev);
    >>>>>>> - void (*remove)(struct rproc_subdev *subdev);
    >>>>>>> + void (*remove)(struct rproc_subdev *subdev, bool graceful);
    >>>>>> What about adding a new ops instead of a parameter, like a recovery
    >>>>>> callback?
    >>>>>>
    >>>>>
    >>>>> I think that for symmetry purposes it should be probe/remove in both
    >>>>> code paths. A possible alternative to the proposal would be to introduce
    >>>>> an operation "request_shutdown()" the would be called in the proposed
    >>>>> graceful code path.
    >>>>>
    >>>>>
    >>>>> However, in the Qualcomm SMD and GLINK (conceptually equivalent to
    >>>>> virtio-rpmsg) it is possible to open and close communication channels
    >>>>> and it's conceivable to see that the graceful case would close all
    >>>>> channels cleanly while the non-graceful case would just remove the rpmsg
    >>>>> devices (and leave the channel states/memory as is).
    >>>>>
    >>>>> In this case a "request_shutdown()" would complicate things, compared to
    >>>>> the boolean.
    >>>>>
    >>>> I would be more for a specific ops that inform sub-dev on a crash. This
    >>>> would allow sub-dev to perform specific action (for instance dump) and
    >>>> store crash information, then on remove, sub_dev would perform specific
    >>>> action.
    >>>
    >>> There is a separate discussion (although dormant) on how to gather core
    >>> dumps, which should cover these cases.
    >>>
    >>>> This could be something like "trigger_recovery" that is propagated to
    >>>> the sub-dev.
    >>>>
    >>>
    >>> Right, this step does make sense, but is the opposite of what I need -
    >>> i.e. a means to trigger a clean shutdown.
    >> Could you clarify this point? i do not see my proposal as the opposite.
    >> In your proposal:
    >> - rproc_trigger_recovery: graceful is set to false
    >> - rproc_shutdown: Graceful is set to true
    >>
    >
    > Correct
    >
    >> My proposal is to call an new ops (if defined) before the stop in
    >> rproc_trigger_recovery. If you set a local variable in Qualcomm subdev
    >> drivers this should do the job for your need.
    >>
    >
    > In all use cases that comes to mind the gracefulness makes one step of
    > the teardown operation kick in/be optional, it's not a separate
    > operation. I don't see the benefit of enforcing the subdev to keep this
    > state.
    >
    >> I tried to have a look in Qualcomm part to understand implementation.
    >> but seems that you just add the parameter for time being.
    >>
    >
    > The following patch, adding sysmon, make use of this. But I have yet to
    > post patches that affects the SMD and GLINK implementations.
    >
    >> I think that main point that bother me here, is the that the "graceful"
    >> mode should be the normal mode. And in your implementation this look
    >> like the exception mode.
    >
    > I agree, I consider the recovery path to be the exception, but I'm not
    > seeing the necessity of making the "true" state of this parameter mean
    > "yes we have an exception".
    >
    > Regardless of the value here, the remove() function's purpose is to
    > clean up resources/state. But in the case of a graceful shutdown (i.e.
    > not recovery path) the subdevices can be expected to tear things down in
    > a fashion that permits the remote side to act, so if anything this
    > "true" would imply that this extra steps should be taken.
    >
    >> Perhaps more a feeling than anything else...but if you decide to keep
    >> argument i would propose to inverse logic using something like
    >> "rproc_crashed" instead of "graceful".
    >>
    >
    > The difference between "this is a graceful shutdown, let's inform the
    > remote" and "this is not a crash, let's inform the remote". The prior
    > sounds much more to the point in my view.

    On the other hand, i consider "graceful" as a normal mode that is
    implemented in kernel. By informing sub-dev that "this is a graceful
    shutdown", you just precise a behavior that is already implemented by
    default.
    And in case of recovery, you inform sub-dev that "this is not a graceful
    shutdown". What does it means "this is not a graceful shutdown"?...This
    is confusing, from my point of view.

    for this reason, I still don't like the "graceful" wording too much, but
    this is a personal feeling, not a strong argument. :)
    So, if you are not convince by my last argument and nobody else
    comments, consider it as OK for me.

    >
    >>>
    >>>> That would sound more flexible from my point of view.
    >>>>
    >>>
    >>> At this point I see this flexibility as unnecessary complexity, if such
    >>> need show up (beyond the core dump gathering) we should bring this up
    >>> again.
    >>
    >> I let you decide what is the best solution.
    >> My concerns is to be sure that your solution is enough generic and not
    >> too Qualcomm platform oriented.
    >
    > That's a fair concern. I'm very interested in finding more complex use
    > cases that requires this type of logic, to see if this is generic
    > enough.
    >
    > Note that the discussions that we've had related to e.g. clocks that
    > should be on during the life of the remote would not fit into the
    > current subdev life cycle anyways, so this either needs to be
    > complemented or extended.
    >
    Today the only other use case, i would have in mind (except dump) is the
    management of a "hot" restart on a crash... but no concrete example.

    >> As you mentioned in OpenAMP meeting it is quite difficult to come back
    >> on an implementation , especially if it impacts the API.
    >>
    >
    > No, what I said is that it's impossible to go back once we have changed
    > the file format, the interface towards the remote or the interface
    > towards user space. All it takes to change an interface within the
    > kernel is a single patch.
    >
    Thanks for this clarification.

    Regards,
    Arnaud




    > Regards,
    > Bjorn
    >

    \
     
     \ /
      Last update: 2017-12-07 13:15    [W:3.268 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site