On Tue 28 Apr 15:16 PDT 2020, 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.
Signed-off-by: Siddharth Gupta <sid...@codeaurora.org>
Signed-off-by: Rishabh Bhatnagar <risha...@codeaurora.org>
Thanks Rishabh, design wise I think this looks good now, just some code
style things below.
---
drivers/remoteproc/qcom_common.c | 89
++++++++++++++++++++++++++++++-----
drivers/remoteproc/qcom_common.h | 10 +++-
include/linux/remoteproc/qcom_rproc.h | 20 ++++++--
3 files changed, 99 insertions(+), 20 deletions(-)
diff --git a/drivers/remoteproc/qcom_common.c
b/drivers/remoteproc/qcom_common.c
index 60650bc..7cd17be 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -15,16 +15,18 @@
#include <linux/rpmsg/qcom_glink.h>
#include <linux/rpmsg/qcom_smd.h>
#include <linux/soc/qcom/mdt_loader.h>
+#include <linux/remoteproc/qcom_rproc.h>
Please maintain alphabetical sort order.
#include "remoteproc_internal.h"
#include "qcom_common.h"
+#define MAX_NAME_LEN 20
+DEFINE_MUTEX(rproc_notif_lock);
Please rename this qcom_ssr_subsystem_lock
+
#define to_glink_subdev(d) container_of(d, struct qcom_rproc_glink,
subdev)
#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);
Move the definition of rproc_notif_info, the new rproc_notif_info list
head and move the two lines above here as well.
-
static int glink_subdev_start(struct rproc_subdev *subdev)
{
struct qcom_rproc_glink *glink = to_glink_subdev(subdev);
@@ -174,39 +176,81 @@ void qcom_remove_smd_subdev(struct rproc *rproc,
struct qcom_rproc_subdev *smd)
}
EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
+struct rproc_notif_info *find_notif_info(const char *name)
Please make this qcom_ssr_get_subsystem(const char *name)
+{
+ struct rproc_notif_info *info;
+
+ /* Match in the global rproc_notif_list with name */
+ list_for_each_entry(info, &rproc_notif_list, list) {
+ if (!strncmp(info->name, name, strlen(name)))
strncmp(a, b, strlen(b)) is the same thing as strcmp(a, b), unless a is
shorted than b and not NUL terminated.
+ return info;
+ }
+ return NULL;
Both callers of this function will if NULL is returned allocate a new
subsystem object and attach to the list. If you do that here you can
remove the duplication between these.
+}
+
/**
* qcom_register_ssr_notifier() - register SSR notification handler
+ * @name: pointer to name which will be searched in the global
notif_list
* @nb: notifier_block to notify for restart notifications
*
- * Returns 0 on success, negative errno on failure.
+ * Returns pointer to srcu notifier head on success, ERR_PTR on
failure.
This shouldn't mention that the opaque pointer is of a type standard to
the kernel. Better just say that it returns a "subsystem cookie".
*
- * 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.
+ * 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 particular remote processor is stopped.
*/
-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 rproc_notif_info *info;
+
+ mutex_lock(&rproc_notif_lock);
+ info = find_notif_info(name);
+ if (!info) {
+ info = kzalloc(sizeof(*info), GFP_KERNEL);
+ if (!info) {
+ mutex_unlock(&rproc_notif_lock);
+ return ERR_PTR(-ENOMEM);
+ }
+ info->name = kstrndup(name, MAX_NAME_LEN, GFP_KERNEL);
This is going to be a constant in a lot of cases, so please use
kstrdup_const(). Also what's the purpose of limiting the length of
this?
+ srcu_init_notifier_head(&info->notifier_list);
+
+ /* Add to global notif list */
+ INIT_LIST_HEAD(&info->list);
+ list_add_tail(&info->list, &rproc_notif_list);
+ }
+
+ srcu_notifier_chain_register(&info->notifier_list, nb);
+ mutex_unlock(&rproc_notif_lock);
+ return &info->notifier_list;
}
EXPORT_SYMBOL_GPL(qcom_register_ssr_notifier);
/**
* qcom_unregister_ssr_notifier() - unregister SSR notification
handler
+ * @notify: pointer to srcu notifier head
@subsystem: subsystem cookie returned from
qcom_register_ssr_notifier
* @nb: notifier_block to unregister
*/
-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);
+ if (!notify)
+ return -EINVAL;
qcom_register_ssr_notifier() will return a valid cookie or a ERR_PTR()
so if someone passes NULL here they did something wrong during
development...
So it's better to just remove this check and give the developer a nice
callstack directly pointing out their mistake, than forcing them to
chase where this -EINVAL comes from.
+
+ 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 rproc_notif_data data = {
+ .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);
Did we conclude on why you change blocking to srcu? Can we do it in a
separate patch?
diff --git a/drivers/remoteproc/qcom_common.h
b/drivers/remoteproc/qcom_common.h
index 58de71e..0c1d288 100644
--- a/drivers/remoteproc/qcom_common.h
+++ b/drivers/remoteproc/qcom_common.h
@@ -24,10 +24,16 @@ struct qcom_rproc_subdev {
struct qcom_smd_edge *edge;
};
+struct rproc_notif_info {
Please rename this struct qcom_ssr_subsystem
+ const char *name;
+ struct srcu_notifier_head notifier_list;
+ struct list_head list;
+};
+static LIST_HEAD(rproc_notif_list);
Please rename this list qcom_ssr_subsystem_list and as stated above
move
it into qcom_common.c.
To allow using qcom_ssr_subsystem in the struct below simply forward
declare it here as:
struct qcom_ssr_subsystem;
+
struct qcom_rproc_ssr {
struct rproc_subdev subdev;
-
- const char *name;
+ struct rproc_notif_info *info;
};
Regards,
Bjorn
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..3dc65c0 100644
--- a/include/linux/remoteproc/qcom_rproc.h
+++ b/include/linux/remoteproc/qcom_rproc.h
@@ -5,17 +5,27 @@
#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 rproc_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
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project
