Re: [PATCH 6/6] remoteproc: qcom: Add notification types to SSR

[Mathieu Poirier <mathieu.poirier@linaro.org>]

On Wed, 1 Apr 2020 at 19:01, Siddharth Gupta <sidgup@codeaurora.org> wrote:
>
> On 3/9/2020 10:34 AM, Mathieu Poirier wrote:
>
> > On Tue, 3 Mar 2020 at 16:30, <rishabhb@codeaurora.org> wrote:
> >> On 2020-03-03 10:05, Mathieu Poirier wrote:
> >>> On Mon, 2 Mar 2020 at 13:54, <rishabhb@codeaurora.org> wrote:
> >>>> On 2020-02-28 10:38, Mathieu Poirier wrote:
> >>>>> On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org
> >>>>> wrote:
> >>>>>> On 2020-02-27 13:59, Mathieu Poirier wrote:
> >>>>>>> On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote:
> >>>>>>>> The SSR subdevice only adds callback for the unprepare event. Add
> >>>>>>>> callbacks
> >>>>>>>> for unprepare, start and prepare events. The client driver for a
> >>>>>>>> particular
> >>>>>>>> remoteproc might be interested in knowing the status of the remoteproc
> >>>>>>>> while undergoing SSR, not just when the remoteproc has finished
> >>>>>>>> shutting
> >>>>>>>> down.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> >>>>>>>> ---
> >>>>>>>>   drivers/remoteproc/qcom_common.c | 39
> >>>>>>>> +++++++++++++++++++++++++++++++++++----
> >>>>>>>>   include/linux/remoteproc.h       | 15 +++++++++++++++
> >>>>>>>>   2 files changed, 50 insertions(+), 4 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/remoteproc/qcom_common.c
> >>>>>>>> b/drivers/remoteproc/qcom_common.c
> >>>>>>>> index 6714f27..6f04a5b 100644
> >>>>>>>> --- a/drivers/remoteproc/qcom_common.c
> >>>>>>>> +++ b/drivers/remoteproc/qcom_common.c
> >>>>>>>> @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
> >>>>>>>>    *
> >>>>>>>>    * Returns pointer to srcu notifier head on success, ERR_PTR on
> >>>>>>>> failure.
> >>>>>>>>    *
> >>>>>>>> - * This registers the @notify function as handler for restart
> >>>>>>>> notifications. As
> >>>>>>>> - * remote processors are stopped this function will be called, with
> >>>>>>>> the rproc
> >>>>>>>> - * pointer passed as a parameter.
> >>>>>>>> + * This registers the @notify function as handler for
> >>>>>>>> powerup/shutdown
> >>>>>>>> + * notifications. This function will be invoked inside the
> >>>>>>>> callbacks registered
> >>>>>>>> + * for the ssr subdevice, with the rproc pointer passed as a
> >>>>>>>> parameter.
> >>>>>>>>    */
> >>>>>>>>   void *qcom_register_ssr_notifier(struct rproc *rproc, struct
> >>>>>>>> notifier_block *nb)
> >>>>>>>>   {
> >>>>>>>> @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify,
> >>>>>>>> struct notifier_block *nb)
> >>>>>>>>   }
> >>>>>>>>   EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
> >>>>>>>>
> >>>>>>>> +static int ssr_notify_prepare(struct rproc_subdev *subdev)
> >>>>>>>> +{
> >>>>>>>> +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >>>>>>>> +
> >>>>>>>> +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >>>>>>>> +                                 RPROC_BEFORE_POWERUP, (void *)ssr->name);
> >>>>>>>> +        return 0;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +static int ssr_notify_start(struct rproc_subdev *subdev)
> >>>>>>>> +{
> >>>>>>>> +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >>>>>>>> +
> >>>>>>>> +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >>>>>>>> +                                 RPROC_AFTER_POWERUP, (void *)ssr->name);
> >>>>>>>> +        return 0;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +static void ssr_notify_stop(struct rproc_subdev *subdev, bool
> >>>>>>>> crashed)
> >>>>>>>> +{
> >>>>>>>> +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >>>>>>>> +
> >>>>>>>> +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >>>>>>>> +                                 RPROC_BEFORE_SHUTDOWN, (void *)ssr->name);
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +
> >>>>>>>>   static void ssr_notify_unprepare(struct rproc_subdev *subdev)
> >>>>>>>>   {
> >>>>>>>>           struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >>>>>>>>
> >>>>>>>> -        srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void
> >>>>>>>> *)ssr->name);
> >>>>>>>> +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >>>>>>>> +                                 RPROC_AFTER_SHUTDOWN, (void *)ssr->name);
> >>>>>>>>   }
> >>>>>>>>
> >>>>>>>>   /**
> >>>>>>>> @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc,
> >>>>>>>> struct qcom_rproc_ssr *ssr,
> >>>>>>>>   {
> >>>>>>>>           ssr->name = ssr_name;
> >>>>>>>>           ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
> >>>>>>>> +        ssr->subdev.prepare = ssr_notify_prepare;
> >>>>>>>> +        ssr->subdev.start = ssr_notify_start;
> >>>>>>>> +        ssr->subdev.stop = ssr_notify_stop;
> >>>>>>> Now that I have a better understanding of what this patchset is doing, I
> >>>>>>> realise
> >>>>>>> my comments in patch 04 won't work.  To differentiate the subdevs of an
> >>>>>>> rproc I
> >>>>>>> suggest to wrap them in a generic structure with a type and an enum.
> >>>>>>> That way
> >>>>>>> you can differenciate between subdevices without having to add to the
> >>>>>>> core.
>
> While creating a new revision of the patchset we tried to implement
> this, but a similar issue comes
> up. If at a later point we wish to utilize the functionality of some
> common subdevice (not the case
> right now, but potentially), we might run into a similar problem of
> accessing illegal memory using
> container_of. I think it might be a better idea to introduce the name in
> the subdevice structure over
> having a potential security bug. What do you think?

I trust that you have given this an honest try but found potential
problems that I can't foresee due to the lack of insight on your
operating environment.  Please move forward with the addition of a new
"name" field to the rproc_subdev structure.

>
> Thanks,
> Siddharth
>
> >>>>>> Ok. I can try that.
> >>>>>>> That being said, I don't understand what patches 5 and 6 are doing...
> >>>>>>> Registering with the global ssr_notifiers allowed to gracefully shutdown
> >>>>>>> all the
> >>>>>>> MCUs in the system when one of them would go down.  But now that we are
> >>>>>>> using
> >>>>>>> the notifier on a per MCU, I really don't see why each subdev couldn't
> >>>>>>> implement
> >>>>>>> the right prepare/start/stop functions.
> >>>>>>>
> >>>>>>> Am I missing something here?
> >>>>>> We only want kernel clients to be notified when the Remoteproc they
> >>>>>> are
> >>>>>> interested
> >>>>>> in changes state. For e.g. audio kernel driver should be notified when
> >>>>>> audio
> >>>>>> processor goes down but it does not care about any other remoteproc.
> >>>>>> If you are suggesting that these kernel clients be added as subdevices
> >>>>>> then
> >>>>>> we will end up having many subdevices registered to each remoteproc.
> >>>>>> So we
> >>>>>> implemented a notifier chain per Remoteproc. This keeps the SSR
> >>>>>> notifications as
> >>>>>> the subdevice per remoteproc, and all interested clients can register
> >>>>>> to it.
> >>>>> It seems like I am missing information...  Your are referring to
> >>>>> "kernel
> >>>>> clients" and as such I must assume some drivers that are not part of
> >>>>> the
> >>>>> remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier().
> >>>>> I must
> >>>> Yes these are not part of remoteproc framework and they will register
> >>>> for notifications.
> >>>>> also assume these drivers (or that functionality) are not yet upsream
> >>>>> because
> >>>>> all I can see calling qcom_register_ssr_notifier() is
> >>>>> qcom_glink_ssr_probe().
> >>>> Correct.These are not upstreamed.
> >>> Ok, things are starting to make sense.
> >>>
> >>>>> Speaking of which, what is the role of the qcom_glink_ssr_driver?  Is
> >>>>> the glink
> >>>>> device that driver is handling the same as the glink device registed in
> >>>>> adsp_probe() and q6v5_probe()?
> >>>> glink ssr driver will send out notifications to remoteprocs that have
> >>>> opened the
> >>>> "glink_ssr" channel that some subsystem has gone down or booted up.
> >>>> This
> >>>> helps notify
> >>>> neighboring subsystems about change in state of any other subsystem.
> >>> I am still looking for an answer to my second question.
> >> Yes its the subdevice of the glink device that is registered in
> >> adsp_probe.
> >> It uses the "glink_ssr" glink channel.
> > Since this is confining events to a single MCU, I was mostly worried
> > about opening the "glink_ssr" channel for nothing but taking a step
> > back and thinking further on this, there might be other purposes for
> > the channel than only receiving notifications of other MCUs in the
> > system going down.
> >
> > Please spin off a new revision of this set and I will take another look.
> >
> > Thanks,
> > Mathieu
> >
> >>>>>>>
> >>>>>>>>           ssr->subdev.unprepare = ssr_notify_unprepare;
> >>>>>>>>           ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
> >>>>>>>>                                                                   GFP_KERNEL);
> >>>>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >>>>>>>> index e2f60cc..4be4478 100644
> >>>>>>>> --- a/include/linux/remoteproc.h
> >>>>>>>> +++ b/include/linux/remoteproc.h
> >>>>>>>> @@ -449,6 +449,21 @@ struct rproc_dump_segment {
> >>>>>>>>   };
> >>>>>>>>
> >>>>>>>>   /**
> >>>>>>>> + * enum rproc_notif_type - Different stages of remoteproc
> >>>>>>>> notifications
> >>>>>>>> + * @RPROC_BEFORE_SHUTDOWN:      unprepare stage of  remoteproc
> >>>>>>>> + * @RPROC_AFTER_SHUTDOWN:       stop stage of  remoteproc
> >>>>>>>> + * @RPROC_BEFORE_POWERUP:       prepare stage of  remoteproc
> >>>>>>>> + * @RPROC_AFTER_POWERUP:        start stage of  remoteproc
> >>>>>>>> + */
> >>>>>>>> +enum rproc_notif_type {
> >>>>>>>> +        RPROC_BEFORE_SHUTDOWN,
> >>>>>>>> +        RPROC_AFTER_SHUTDOWN,
> >>>>>>>> +        RPROC_BEFORE_POWERUP,
> >>>>>>>> +        RPROC_AFTER_POWERUP,
> >>>>>>>> +        RPROC_MAX
> >>>>>>>> +};
> >>>>>>>> +
> >>>>>>>> +/**
> >>>>>>>>    * struct rproc - represents a physical remote processor device
> >>>>>>>>    * @node: list node of this rproc object
> >>>>>>>>    * @domain: iommu domain
> >>>>>>>> --
> >>>>>>>> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> >>>>>>>> a Linux Foundation Collaborative Project
> >>>>>>>>
> >>>>>>>> _______________________________________________
> >>>>>>>> linux-arm-kernel mailing list
> >>>>>>>> linux-arm-kernel@lists.infradead.org
> >>>>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>>> _______________________________________________
> >>>> linux-arm-kernel mailing list
> >>>> linux-arm-kernel@lists.infradead.org
> >>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel