Messages in this thread | | | Date | Tue, 11 Jul 2023 13:30:24 +0530 | From | Vignesh Viswanathan <> | Subject | Re: [PATCH] remoteproc: qcom: Add NOTIFY_FATAL event type to SSR subdevice |
| |
On 6/15/2023 4:10 PM, Vignesh Viswanathan wrote: > > > On 5/29/2023 9:27 AM, Vignesh Viswanathan wrote: >> Gentle Reminder. >> >> On 5/22/2023 3:03 PM, Mukesh Ojha wrote: >>> >>> >>> On 5/3/2023 4:56 PM, Mukesh Ojha wrote: >>>> >>>> >>>> On 5/3/2023 11:51 AM, Vignesh Viswanathan wrote: >>>>> Currently the SSR subdevice notifies the client driver on crash of the >>>>> rproc from the recovery workqueue using the BEFORE_SHUTDOWN event. >>>>> However the client driver might be interested to know that the device >>>>> has crashed immediately to pause any further transactions with the >>>>> rproc. This calls for an event to be sent to the driver in the IRQ >>>>> context as soon as the rproc crashes. >>>>> >>>>> Add NOTIFY_FATAL event to SSR subdevice to atomically notify rproc has >>>>> crashed to the client driver. >>>>> >>>>> Validated the event in IPQ9574 and IPQ5332 by forcing the rproc to >>>>> crash >>>>> and ensuring the registered notifier function receives the >>>>> notification >>>>> in IRQ context. >>>> >>>> This was one of valid use case we encounter in android, We have some >>>> other way of doing the same thing without core kernel change with >>>> something called early notifiers. >>>> >>>> https://git.codelinaro.org/clo/la/kernel/msm-5.15/-/commit/7583d24de337aa1bf7c375a7da706af9b995b9a1#a840754ebb0e24e88adbf48177e1abd0830b72d2 >>>> >>>> https://git.codelinaro.org/clo/la/kernel/msm-5.15/-/commit/257de41c63a5a51a081cc7887cdaa4a46e4d1744 >>>> >>>> But good to address this if possible. >>> >>> Ack the idea of early notifier; >>> But here, atomic does not guarantees it to be atomic. >>> >>> Acked-by: Mukesh Ojha <quic_mojha@quicinc.com> >>> >>> -- Mukesh >>> > Gentle Reminder! > > Thanks, > Vignesh >
Gentle reminder for review!
Thanks, Vignesh
>>> >>>> >>>> -Mukesh >>>>> >>>>> Signed-off-by: Vignesh Viswanathan <quic_viswanat@quicinc.com> >>>>> --- >>>>> drivers/remoteproc/qcom_common.c | 60 >>>>> +++++++++++++++++++++++++++ >>>>> drivers/remoteproc/remoteproc_core.c | 12 ++++++ >>>>> include/linux/remoteproc.h | 3 ++ >>>>> include/linux/remoteproc/qcom_rproc.h | 17 ++++++++ >>>>> 4 files changed, 92 insertions(+) >>>>> >>>>> diff --git a/drivers/remoteproc/qcom_common.c >>>>> b/drivers/remoteproc/qcom_common.c >>>>> index a0d4238492e9..76542229aeb6 100644 >>>>> --- a/drivers/remoteproc/qcom_common.c >>>>> +++ b/drivers/remoteproc/qcom_common.c >>>>> @@ -84,6 +84,7 @@ struct minidump_global_toc { >>>>> struct qcom_ssr_subsystem { >>>>> const char *name; >>>>> struct srcu_notifier_head notifier_list; >>>>> + struct atomic_notifier_head atomic_notifier_list; >>>>> struct list_head list; >>>>> }; >>>>> @@ -366,6 +367,7 @@ static struct qcom_ssr_subsystem >>>>> *qcom_ssr_get_subsys(const char *name) >>>>> } >>>>> info->name = kstrdup_const(name, GFP_KERNEL); >>>>> srcu_init_notifier_head(&info->notifier_list); >>>>> + ATOMIC_INIT_NOTIFIER_HEAD(&info->atomic_notifier_list); >>>>> /* Add to global notification list */ >>>>> list_add_tail(&info->list, &qcom_ssr_subsystem_list); >>>>> @@ -417,6 +419,51 @@ int qcom_unregister_ssr_notifier(void *notify, >>>>> struct notifier_block *nb) >>>>> } >>>>> EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier); >>>>> +/** >>>>> + * qcom_register_ssr_atomic_notifier() - register SSR Atomic >>>>> notification >>>>> + * handler >>>>> + * @name: Subsystem's SSR name >>>>> + * @nb: notifier_block to be invoked upon subsystem's state change >>>>> + * >>>>> + * This registers the @nb notifier block as part the atomic notifier >>>>> + * chain for a remoteproc associated with @name. The notifier >>>>> block's callback >>>>> + * will be invoked when the remote processor crashes in atomic >>>>> context before >>>>> + * the recovery process is queued. >>>>> + * >>>>> + * Return: a subsystem cookie on success, ERR_PTR on failure. >>>>> + */ >>>>> +void *qcom_register_ssr_atomic_notifier(const char *name, >>>>> + struct notifier_block *nb) >>>>> +{ >>>>> + struct qcom_ssr_subsystem *info; >>>>> + >>>>> + info = qcom_ssr_get_subsys(name); >>>>> + if (IS_ERR(info)) >>>>> + return info; >>>>> + >>>>> + atomic_notifier_chain_register(&info->atomic_notifier_list, nb); >>>>> + >>>>> + return &info->atomic_notifier_list; >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(qcom_register_ssr_atomic_notifier); >>>>> + >>>>> +/** >>>>> + * qcom_unregister_ssr_atomic_notifier() - unregister SSR Atomic >>>>> notification >>>>> + * handler >>>>> + * @notify: subsystem cookie returned from >>>>> qcom_register_ssr_notifier >>>>> + * @nb: notifier_block to unregister >>>>> + * >>>>> + * This function will unregister the notifier from the atomic >>>>> notifier >>>>> + * chain. >>>>> + * >>>>> + * Return: 0 on success, %ENOENT otherwise. >>>>> + */ >>>>> +int qcom_unregister_ssr_atomic_notifier(void *notify, struct >>>>> notifier_block *nb) >>>>> +{ >>>>> + return atomic_notifier_chain_unregister(notify, nb); >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(qcom_unregister_ssr_atomic_notifier); >>>>> + >>>>> static int ssr_notify_prepare(struct rproc_subdev *subdev) >>>>> { >>>>> struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); >>>>> @@ -467,6 +514,18 @@ static void ssr_notify_unprepare(struct >>>>> rproc_subdev *subdev) >>>>> QCOM_SSR_AFTER_SHUTDOWN, &data); >>>>> } >>>>> +static void ssr_notify_crash(struct rproc_subdev *subdev) >>>>> +{ >>>>> + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); >>>>> + struct qcom_ssr_notify_data data = { >>>>> + .name = ssr->info->name, >>>>> + .crashed = true, >>>>> + }; >>>>> + >>>>> + atomic_notifier_call_chain(&ssr->info->atomic_notifier_list, >>>>> + QCOM_SSR_NOTIFY_CRASH, &data); >>>>> +} >>>>> + >>>>> /** >>>>> * qcom_add_ssr_subdev() - register subdevice as restart >>>>> notification source >>>>> * @rproc: rproc handle >>>>> @@ -493,6 +552,7 @@ void qcom_add_ssr_subdev(struct rproc *rproc, >>>>> struct qcom_rproc_ssr *ssr, >>>>> ssr->subdev.start = ssr_notify_start; >>>>> ssr->subdev.stop = ssr_notify_stop; >>>>> ssr->subdev.unprepare = ssr_notify_unprepare; >>>>> + ssr->subdev.notify_crash = ssr_notify_crash; >>>>> rproc_add_subdev(rproc, &ssr->subdev); >>>>> } >>>>> diff --git a/drivers/remoteproc/remoteproc_core.c >>>>> b/drivers/remoteproc/remoteproc_core.c >>>>> index 695cce218e8c..3de0ece158ea 100644 >>>>> --- a/drivers/remoteproc/remoteproc_core.c >>>>> +++ b/drivers/remoteproc/remoteproc_core.c >>>>> @@ -1139,6 +1139,16 @@ static void >>>>> rproc_unprepare_subdevices(struct rproc *rproc) >>>>> } >>>>> } >>>>> +static void rproc_notify_crash_subdevices(struct rproc *rproc) >>>>> +{ >>>>> + struct rproc_subdev *subdev; >>>>> + >>>>> + list_for_each_entry_reverse(subdev, &rproc->subdevs, node) { >>>>> + if (subdev->notify_crash) >>>>> + subdev->notify_crash(subdev); >>>>> + } >>>>> +} >>>>> + >>>>> /** >>>>> * rproc_alloc_registered_carveouts() - allocate all carveouts >>>>> registered >>>>> * in the list >>>>> @@ -2687,6 +2697,8 @@ void rproc_report_crash(struct rproc *rproc, >>>>> enum rproc_crash_type type) >>>>> dev_err(&rproc->dev, "crash detected in %s: type %s\n", >>>>> rproc->name, rproc_crash_to_string(type)); >>>>> + rproc_notify_crash_subdevices(rproc); >>>>> + >>>>> queue_work(rproc_recovery_wq, &rproc->crash_handler); >>>>> } >>>>> EXPORT_SYMBOL(rproc_report_crash); >>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h >>>>> index fe8978eb69f1..f3c0e0103e81 100644 >>>>> --- a/include/linux/remoteproc.h >>>>> +++ b/include/linux/remoteproc.h >>>>> @@ -596,6 +596,8 @@ struct rproc { >>>>> * @stop: stop function, called before the rproc is stopped; the >>>>> @crashed >>>>> * parameter indicates if this originates from a recovery >>>>> * @unprepare: unprepare function, called after the rprochas >>>>> been stopped >>>>> + * @notify_crash: notify_crash function, called in atomic context >>>>> to notify >>>>> + * rproc has crashed and recovery is about to start >>>>> */ >>>>> struct rproc_subdev { >>>>> struct list_head node; >>>>> @@ -604,6 +606,7 @@ struct rproc_subdev { >>>>> int (*start)(struct rproc_subdev *subdev); >>>>> void (*stop)(struct rproc_subdev *subdev, bool crashed); >>>>> void (*unprepare)(struct rproc_subdev *subdev); >>>>> + void (*notify_crash)(struct rproc_subdev *subdev); >>>>> }; >>>>> /* we currently support only two vrings per rvdev */ >>>>> diff --git a/include/linux/remoteproc/qcom_rproc.h >>>>> b/include/linux/remoteproc/qcom_rproc.h >>>>> index 82b211518136..f3d06900f297 100644 >>>>> --- a/include/linux/remoteproc/qcom_rproc.h >>>>> +++ b/include/linux/remoteproc/qcom_rproc.h >>>>> @@ -11,12 +11,14 @@ struct notifier_block; >>>>> * @QCOM_SSR_AFTER_POWERUP: Remoteproc is running (start stage) >>>>> * @QCOM_SSR_BEFORE_SHUTDOWN: Remoteproc crashed or shutting >>>>> down (stop stage) >>>>> * @QCOM_SSR_AFTER_SHUTDOWN: Remoteprocis down (unprepare stage) >>>>> + * @QCOM_SSR_NOTIFY_CRASH: Remoteproc crashed >>>>> */ >>>>> enum qcom_ssr_notify_type { >>>>> QCOM_SSR_BEFORE_POWERUP, >>>>> QCOM_SSR_AFTER_POWERUP, >>>>> QCOM_SSR_BEFORE_SHUTDOWN, >>>>> QCOM_SSR_AFTER_SHUTDOWN, >>>>> + QCOM_SSR_NOTIFY_CRASH, >>>>> }; >>>>> struct qcom_ssr_notify_data { >>>>> @@ -29,6 +31,10 @@ struct qcom_ssr_notify_data { >>>>> void *qcom_register_ssr_notifier(const char *name, struct >>>>> notifier_block *nb); >>>>> int qcom_unregister_ssr_notifier(void *notify, struct >>>>> notifier_block *nb); >>>>> +void *qcom_register_ssr_atomic_notifier(const char *name, >>>>> + struct notifier_block *nb); >>>>> +int qcom_unregister_ssr_atomic_notifier(void *notify, >>>>> + struct notifier_block *nb); >>>>> #else >>>>> static inline void *qcom_register_ssr_notifier(const char *name, >>>>> @@ -43,6 +49,17 @@ static inline int >>>>> qcom_unregister_ssr_notifier(void *notify, >>>>> return 0; >>>>> } >>>>> +static inline void *qcom_register_ssr_atomic_notifier(const char >>>>> *name, >>>>> + struct notifier_block *nb) >>>>> +{ >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static inline int qcom_unregister_ssr_atomic_notifier(void *notify, >>>>> + struct notifier_block *nb) >>>>> +{ >>>>> + return 0; >>>>> +} >>>>> #endif >>>>> #endif >>>>
| |