On 5/27/20 10:34 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.
I'm looking at these patches now, without having reviewed the
previous versions. Forgive me if I contradict or duplicate
previous feedback.
I have a number of suggestions, below.
-Alex
Signed-off-by: Siddharth Gupta <sidgup@xxxxxxxxxxxxxx>
Signed-off-by: Rishabh Bhatnagar <rishabhb@xxxxxxxxxxxxxx>
---
drivers/remoteproc/qcom_common.c | 84 ++++++++++++++++++++++++++++++-----
drivers/remoteproc/qcom_common.h | 5 ++-
include/linux/remoteproc/qcom_rproc.h | 20 ++++++---
3 files changed, 90 insertions(+), 19 deletions(-)
diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 9028cea..61ff2dd 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);
+DEFINE_MUTEX(qcom_ssr_subsys_lock);
There is no need for this mutex to be global.
static int glink_subdev_start(struct rproc_subdev *subdev)
{
@@ -189,39 +197,79 @@ void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd)
}
EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
+struct qcom_ssr_subsystem *qcom_ssr_get_subsys(const char *name)
This function should be made private (static).
I think the mutex should be taken in this function rather than
the caller (more on this below). But if you leave it this way,
please mention something in a comment that indicates the caller
must hold the qcom_ssr_subsys_lock mutex.
+{
+ struct qcom_ssr_subsystem *info;
+
+ /* 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;
This probably isn't strictly necessary, because you are
returning a void pointer, but you could do this here:
return ERR_CAST(info);
+ }
This is purely a style thing, but the curly braces around
the loop body aren't necessary.
+ info = kzalloc(sizeof(*info), GFP_KERNEL);
+ if (!info)
+ return ERR_PTR(-ENOMEM);
+ info->name = kstrdup_const(name, GFP_KERNEL);
+ srcu_init_notifier_head(&info->notifier_list);
+
+ /* Add to global notif list */
s/notif/notification/
+ INIT_LIST_HEAD(&info->list);
No need to initialize the list element when adding it
to a list. Both uts fields will be overwritten anyway.
+ list_add_tail(&info->list, &qcom_ssr_subsystem_list);
+
+ return info;
+}
+
/**
* qcom_register_ssr_notifier() - register SSR notification handler
+ * @name: name that will be searched in global ssr subsystem list
Maybe just "SSR subsystem name".
* @nb: notifier_block to notify for restart notifications
Drop or modify "restart" in the comment line above.
*
- * Returns 0 on success, negative errno on failure.
+ * Returns a subsystem cookie on success, ERR_PTR on failure.
Maybe make the above a @Return: comment.
*
- * 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.
It's not just for stopping, right? Maybe something
more like:
Register to receive notification callbacks when
remoteproc SSR events occur (pre- and post-startup
and pre- and post-shutdown).
*/
-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;
+
+ mutex_lock(&qcom_ssr_subsys_lock);
Can you explain why the mutex is taken here (and in
qcom_add_ssr_subdev()), rather than having the mutex
logic be isolated in qcom_ssr_get_subsys()?
+ info = qcom_ssr_get_subsys(name);
+ if (IS_ERR(info)) {
+ mutex_unlock(&qcom_ssr_subsys_lock);
+ return info;
+ }
I don't think there's any need to have the next function
call be protected by the mutex, but maybe I'm mistaken.
+ srcu_notifier_chain_register(&info->notifier_list, nb);
+ mutex_unlock(&qcom_ssr_subsys_lock);
+ 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
Add a @Return comment (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 = {
+ .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);
}
+
/**
* qcom_add_ssr_subdev() - register subdevice as restart notification source
* @rproc: rproc handle
@@ -229,12 +277,23 @@ 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.
*/
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;
+
+ mutex_lock(&qcom_ssr_subsys_lock);
+ info = qcom_ssr_get_subsys(ssr_name);
If there already exists an SSR subsystem having the given
name, its info structure is returned here. Is that OK?
In practice, I don't expect this to be a problem, but it
would be better to return an error if
+ if (IS_ERR(info)) {
+ dev_err(&rproc->dev, "Failed to add ssr subdevice\n");
+ mutex_unlock(&qcom_ssr_subsys_lock);
+ return;
+ }
+
+ mutex_unlock(&qcom_ssr_subsys_lock);
+ ssr->info = info;
ssr->subdev.unprepare = ssr_notify_unprepare;
rproc_add_subdev(rproc, &ssr->subdev);
@@ -249,6 +308,7 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,
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 @@
#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;
Is the crashed field strictly necessary? Could we instead have
a QCOM_SSR_CRASHED event (in place of QCOM_SSR_BEFORE_SHUTDOWN)?
I don't know, it's possible doing it the way you do ultimately
simplifies the logic... So I'm asking, but not suggesting.
+};
+
+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
