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

Siddharth Gupta
2020-04-02T01:02:01Z

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?

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