Messages in this thread | | | Date | Tue, 23 Jun 2020 18:41:37 -0700 | From | rishabhb@codeauro ... | Subject | Re: [PATCH v5 1/2] remoteproc: qcom: Add per subsystem SSR notification |
| |
On 2020-06-23 14:45, Alex Elder wrote: > On 6/22/20 8:04 PM, Rishabh Bhatnagar wrote: >> Currently there is a single notification chain which is called >> whenever any >> remoteproc shuts down. This leads to all the listeners being notified, >> and >> is not an optimal design as kernel drivers might only be interested in >> listening to notifications from a particular remoteproc. Create a >> global >> list of remoteproc notification info data structures. This will hold >> the >> name and notifier_list information for a particular remoteproc. The >> API >> to register for notifications will use name argument to retrieve the >> notification info data structure and the notifier block will be added >> to >> that data structure's notification chain. Also move from blocking >> notifier >> to srcu notifer based implementation to support dynamic notifier head >> creation. >> >> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> >> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org> > > Sorry, a few more comments, but I think your next one will > likely be fine. > > General: > - SSR subsystems are added but never removed. Note that > "qcom_common.o" can be built as a module, and if that > module were ever removed, memory allocated for these > subsystems would be leaked. Hi Alex, Thank you for reviewing this patchset quickly. This point was brought up by Bjorn and it was decided that I will push another patch on top in which I'll do the cleanup during module exit. > - Will a remoteproc subdev (and in particular, an SSR subdev) > ever be removed? What happens to entities that have > registered for SSR notifications in that case? In practice it should never be removed. If it is clients will never get notification about subsystem shutdown/powerup. > > (Maybe these are issues that won't/can't occur in practice?) > >> --- >> drivers/remoteproc/qcom_common.c | 86 >> +++++++++++++++++++++++++++++------ >> drivers/remoteproc/qcom_common.h | 5 +- >> include/linux/remoteproc/qcom_rproc.h | 20 ++++++-- >> 3 files changed, 91 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/remoteproc/qcom_common.c >> b/drivers/remoteproc/qcom_common.c >> index 9028cea..658f2ca 100644 >> --- a/drivers/remoteproc/qcom_common.c >> +++ b/drivers/remoteproc/qcom_common.c >> @@ -12,6 +12,7 @@ >> #include <linux/module.h> >> #include <linux/notifier.h> >> #include <linux/remoteproc.h> >> +#include <linux/remoteproc/qcom_rproc.h> >> #include <linux/rpmsg/qcom_glink.h> >> #include <linux/rpmsg/qcom_smd.h> >> #include <linux/soc/qcom/mdt_loader.h> >> @@ -23,7 +24,14 @@ >> #define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, >> subdev) >> #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, >> subdev) >> -static BLOCKING_NOTIFIER_HEAD(ssr_notifiers); >> +struct qcom_ssr_subsystem { >> + const char *name; >> + struct srcu_notifier_head notifier_list; >> + struct list_head list; >> +}; >> + >> +static LIST_HEAD(qcom_ssr_subsystem_list); >> +static DEFINE_MUTEX(qcom_ssr_subsys_lock); >> static int glink_subdev_start(struct rproc_subdev *subdev) >> { >> @@ -189,37 +197,80 @@ void qcom_remove_smd_subdev(struct rproc *rproc, >> struct qcom_rproc_subdev *smd) >> } >> EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev); >> +static struct qcom_ssr_subsystem *qcom_ssr_get_subsys(const char >> *name) >> +{ >> + struct qcom_ssr_subsystem *info; >> + >> + mutex_lock(&qcom_ssr_subsys_lock); >> + /* Match in the global qcom_ssr_subsystem_list with name */ >> + list_for_each_entry(info, &qcom_ssr_subsystem_list, list) >> + if (!strcmp(info->name, name)) >> + return info; > > You need to unlock the mutex here. You would probably > be better off structuring this with a common exit path > below, for example: > > if (!strcmp(info->name, name)) > goto out_mutex_unlock; > > . . . > > out_mutex_unlock: > mutex_unlock(&qcom_ssr_subsys_lock); > > return info; > } > >> + >> + info = kzalloc(sizeof(*info), GFP_KERNEL); >> + if (!info) >> + return ERR_PTR(-ENOMEM); > > Here too. Perhaps this: > > if (!info) { > info = ERR_PTR(-ENOMEM); > goto out_mutex_unlock; > } > >> + info->name = kstrdup_const(name, GFP_KERNEL); >> + srcu_init_notifier_head(&info->notifier_list); >> + >> + /* Add to global notification list */ >> + list_add_tail(&info->list, &qcom_ssr_subsystem_list); >> + mutex_unlock(&qcom_ssr_subsys_lock); >> + >> + return info; >> +} >> + >> /** >> * qcom_register_ssr_notifier() - register SSR notification handler >> - * @nb: notifier_block to notify for restart notifications >> + * @name: Subsystem's SSR name >> + * @nb: notifier_block to be invoked upon subsystem's state change >> * >> - * Returns 0 on success, negative errno on failure. >> + * This registers the @nb notifier block as part the notifier chain >> for a >> + * remoteproc associated with @name. The notifier block's callback >> + * will be invoked when the remote processor's SSR events occur >> + * (pre/post startup and pre/post shutdown). >> * >> - * This register the @notify function as handler for restart >> notifications. As >> - * remote processors are stopped this function will be called, with >> the SSR >> - * name passed as a parameter. >> + * Return: a subsystem cookie on success, ERR_PTR on failure. >> */ >> -int qcom_register_ssr_notifier(struct notifier_block *nb) >> +void *qcom_register_ssr_notifier(const char *name, struct >> notifier_block *nb) >> { >> - return blocking_notifier_chain_register(&ssr_notifiers, nb); >> + struct qcom_ssr_subsystem *info; >> + >> + info = qcom_ssr_get_subsys(name); >> + if (IS_ERR(info)) >> + return info; >> + >> + srcu_notifier_chain_register(&info->notifier_list, nb); >> + >> + return &info->notifier_list; >> } >> EXPORT_SYMBOL_GPL(qcom_register_ssr_notifier); >> /** >> * qcom_unregister_ssr_notifier() - unregister SSR notification >> handler >> + * @notify: subsystem coookie returned from >> qcom_register_ssr_notifier >> * @nb: notifier_block to unregister >> + * >> + * This function will unregister the notifier from the particular >> notifier >> + * chain. >> + * >> + * Return: 0 on success, %ENOENT otherwise. >> */ >> -void qcom_unregister_ssr_notifier(struct notifier_block *nb) >> +int qcom_unregister_ssr_notifier(void *notify, struct notifier_block >> *nb) >> { >> - blocking_notifier_chain_unregister(&ssr_notifiers, nb); >> + return srcu_notifier_chain_unregister(notify, nb); >> } >> EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier); >> static void ssr_notify_unprepare(struct rproc_subdev *subdev) >> { >> struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); >> + struct qcom_ssr_notif_data data = { > > It's defined in "qcom_rproc.h", but how about naming this type > qcom_ssr_notify_data (or even just qcom_ssr_notify). > Ok "qcom_ssr_notify_data" sounds fine. >> + .name = ssr->info->name, >> + .crashed = false, >> + }; >> - blocking_notifier_call_chain(&ssr_notifiers, 0, (void >> *)ssr->name); >> + srcu_notifier_call_chain(&ssr->info->notifier_list, 0, &data); >> } >> /** >> @@ -229,12 +280,20 @@ static void ssr_notify_unprepare(struct >> rproc_subdev *subdev) >> * @ssr_name: identifier to use for notifications originating from >> @rproc >> * >> * As the @ssr is registered with the @rproc SSR events will be sent >> to all >> - * registered listeners in the system as the remoteproc is shut down. >> + * registered listeners for the particular remoteproc when it is >> shutdown. > > I suggest rewording this comment to make it more general, > considering the events are related to both startup and > shutdown. Scan through the file for other instances > similar to this (I mentioned one previously). > >> */ >> void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr >> *ssr, >> const char *ssr_name) >> { >> - ssr->name = ssr_name; >> + struct qcom_ssr_subsystem *info; >> + >> + info = qcom_ssr_get_subsys(ssr_name); >> + if (IS_ERR(info)) { >> + dev_err(&rproc->dev, "Failed to add ssr subdevice\n"); >> + return; >> + } >> + >> + ssr->info = info; >> ssr->subdev.unprepare = ssr_notify_unprepare; > > Probably all fields should be initialized each time (though > I know you're initializing them in the next patch, so I > guess it's fine...). > > -Alex > >> rproc_add_subdev(rproc, &ssr->subdev); >> @@ -249,6 +308,7 @@ EXPORT_SYMBOL_GPL(qcom_add_ssr_subdev); >> void qcom_remove_ssr_subdev(struct rproc *rproc, struct >> qcom_rproc_ssr *ssr) >> { >> rproc_remove_subdev(rproc, &ssr->subdev); >> + ssr->info = NULL; >> } >> EXPORT_SYMBOL_GPL(qcom_remove_ssr_subdev); >> diff --git a/drivers/remoteproc/qcom_common.h >> b/drivers/remoteproc/qcom_common.h >> index 34e5188..dfc641c 100644 >> --- a/drivers/remoteproc/qcom_common.h >> +++ b/drivers/remoteproc/qcom_common.h >> @@ -26,10 +26,11 @@ struct qcom_rproc_subdev { >> struct qcom_smd_edge *edge; >> }; >> +struct qcom_ssr_subsystem; >> + >> struct qcom_rproc_ssr { >> struct rproc_subdev subdev; >> - >> - const char *name; >> + struct qcom_ssr_subsystem *info; >> }; >> void qcom_add_glink_subdev(struct rproc *rproc, struct >> qcom_rproc_glink *glink, >> diff --git a/include/linux/remoteproc/qcom_rproc.h >> b/include/linux/remoteproc/qcom_rproc.h >> index fa8e386..58422b1 100644 >> --- a/include/linux/remoteproc/qcom_rproc.h >> +++ b/include/linux/remoteproc/qcom_rproc.h >> @@ -5,17 +5,27 @@ struct notifier_block; >> #if IS_ENABLED(CONFIG_QCOM_RPROC_COMMON) >> -int qcom_register_ssr_notifier(struct notifier_block *nb); >> -void qcom_unregister_ssr_notifier(struct notifier_block *nb); >> +struct qcom_ssr_notif_data { >> + const char *name; >> + bool crashed; >> +}; >> + >> +void *qcom_register_ssr_notifier(const char *name, struct >> notifier_block *nb); >> +int qcom_unregister_ssr_notifier(void *notify, struct notifier_block >> *nb); >> #else >> -static inline int qcom_register_ssr_notifier(struct notifier_block >> *nb) >> +static inline void *qcom_register_ssr_notifier(const char *name, >> + struct notifier_block *nb) >> { >> - return 0; >> + return NULL; >> } >> -static inline void qcom_unregister_ssr_notifier(struct >> notifier_block *nb) {} >> +static inline int qcom_unregister_ssr_notifier(void *notify, >> + struct notifier_block *nb) >> +{ >> + return 0; >> +} >> #endif >>
| |