Skip to contentSkip to conversation

Extend SSR notifications framework

PATCH Superseded Rishabh Bhatnagar 3 participants 7 messages Jun 23, 2020 – Jun 24, 2020

Subsystems (2)
Arm Qualcomm mailing list Remote processor (remoteproc)

v5 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Rishabh Bhatnagar

author
#0 lore raw

v5 cover Extend SSR notifications framework

This set of patches gives kernel client drivers the ability to register for a particular remoteproc's SSR notifications. Also the notifications are extended to before/after-powerup/shutdown stages.
It also fixes the bug where clients need to register for notifications again if the platform driver is removed. This is done by creating a global list of per-remoteproc notification info data structures which remain static. An API is exported to register for a remoteproc's SSR notifications and uses remoteproc's ssr_name and notifier block as arguments.

Changelog:
v5 -> v4:
- Make qcom_ssr_get_subsys static function
- Fix mutex locking
- Fix function comments

v4 -> v3:
- Fix naming convention

v3 -> v2:
- Create a global list of per remoteproc notification data structure
- Pass ssr_name and crashed information as part of notification data
- Move notification type enum to qcom_rproc.h from remoteproc.h

v2 -> v1:
- Fix commit text

Rishabh Bhatnagar (1):
remoteproc: qcom: Add per subsystem SSR notification

Siddharth Gupta (1):
remoteproc: qcom: Add notification types to SSR

 drivers/remoteproc/qcom_common.c      | 128 ++++++++++++++++++++++++++++++----
 drivers/remoteproc/qcom_common.h      |   5 +-
 include/linux/remoteproc/qcom_rproc.h |  36 ++++++++--
 3 files changed, 149 insertions(+), 20 deletions(-)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#1 lore raw

PATCH v5 1/2 remoteproc: qcom: Add per subsystem SSR notification

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 <[email protected]>
Signed-off-by Rishabh Bhatnagar <[email protected]>
---
 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(-)
drivers/remoteproc/qcom_common.c +73 -13
index 9028cea..658f2ca 100644
@@ -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;
+
+	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 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 = {
+		.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.
  */
 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;
 
 	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);
drivers/remoteproc/qcom_common.h +3 -2
index 34e5188..dfc641c 100644
@@ -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,
include/linux/remoteproc/qcom_rproc.h +15 -5
index fa8e386..58422b1 100644
@@ -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
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#2 lore raw

Re: [PATCH v5 1/2] remoteproc: qcom: Add per subsystem SSR notification

Quoted 12 lines · Rishabh Bhatnagar

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.

Quoted 2 lines
Signed-off-by Siddharth Gupta <[email protected]>
Signed-off-by Rishabh Bhatnagar <[email protected]>

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.
- 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?

(Maybe these are issues that won't/can't occur in practice?)

Quoted 37 lines
---
  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(-)
drivers/remoteproc/qcom_common.c +9 -1
index 9028cea..658f2ca 100644
@@ -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);

drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 208

Quoted 10 lines
drivers/remoteproc/qcom_common.c +9 -0
 
+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;
}

drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 212

Quoted 4 lines
drivers/remoteproc/qcom_common.c +4 -0
+
+	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;
	}
Quoted 55 lines
drivers/remoteproc/qcom_common.c +34 -8
+	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)

drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 268

Quoted 10 lines
drivers/remoteproc/qcom_common.c +2 -1
  {
-	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).

Quoted 10 lines
drivers/remoteproc/qcom_common.c +4 -1
+		.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);
  }
 
  /**

drivers/remoteproc/qcom_common.c static void ssr_notify_unprepare(struct rproc_subdev *subdev) · line 283

Quoted 5 lines
drivers/remoteproc/qcom_common.c +1 -1
@@ -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).

Quoted 5 lines
drivers/remoteproc/qcom_common.c +0 -1
   */
  void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,
  			 const char *ssr_name)
  {
-	ssr->name = ssr_name;

drivers/remoteproc/qcom_common.c static void ssr_notify_unprepare(struct rproc_subdev *subdev) · line 297

Quoted 10 lines
drivers/remoteproc/qcom_common.c +9 -0
+	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

Quoted 65 lines
drivers/remoteproc/qcom_common.c +1 -0
  	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);
drivers/remoteproc/qcom_common.h +3 -2
index 34e5188..dfc641c 100644
@@ -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,
include/linux/remoteproc/qcom_rproc.h +15 -5
index fa8e386..58422b1 100644
@@ -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
author
#3 lore raw

Re: [PATCH v5 1/2] remoteproc: qcom: Add per subsystem SSR notification

Quoted 16 lines · Alex Elder
Quoted 12 lines · Rishabh Bhatnagar

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.

Quoted 2 lines
Signed-off-by Siddharth Gupta <[email protected]>
Signed-off-by Rishabh Bhatnagar <[email protected]>
Quoted 8 lines

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.

Quoted 3 lines

- 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.

Quoted 146 lines

(Maybe these are issues that won't/can't occur in practice?)

Quoted 39 lines
---
  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(-)
drivers/remoteproc/qcom_common.c +9 -0
index 9028cea..658f2ca 100644
@@ -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);

drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, · line 209

Quoted 10 lines
drivers/remoteproc/qcom_common.c +8 -0
  +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;
}

drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 212

Quoted 4 lines
drivers/remoteproc/qcom_common.c +4 -0
+
+	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;
	}
Quoted 66 lines
drivers/remoteproc/qcom_common.c +13 -2
+	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. */

drivers/remoteproc/qcom_common.c +1 -1
-int qcom_register_ssr_notifier(struct notifier_block *nb)
+void *qcom_register_ssr_notifier(const char *name, struct

notifier_block *nb)
{

drivers/remoteproc/qcom_common.c +9 -1
-	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.
*/

drivers/remoteproc/qcom_common.c +1 -1
-void qcom_unregister_ssr_notifier(struct notifier_block *nb)
+int qcom_unregister_ssr_notifier(void *notify, struct notifier_block

*nb)
{

drivers/remoteproc/qcom_common.c +1 -1
-	blocking_notifier_chain_unregister(&ssr_notifiers, nb);
+	return srcu_notifier_chain_unregister(notify, nb);
  }
Quoted 9 lines

drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 268

Quoted 5 lines
  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);
drivers/remoteproc/qcom_common.c +1 -0
+	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.

Quoted 109 lines
Quoted 11 lines
drivers/remoteproc/qcom_common.c +3 -0
+		.name = ssr->info->name,
+		.crashed = false,
+	};
  -	blocking_notifier_call_chain(&ssr_notifiers, 0, (void

*)ssr->name);

drivers/remoteproc/qcom_common.c +1 -0
+	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
Quoted 7 lines

@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).

Quoted 6 lines
   */

void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr
*ssr,
const char *ssr_name)
{

drivers/remoteproc/qcom_common.c +0 -1
-	ssr->name = ssr_name;

drivers/remoteproc/qcom_common.c static void ssr_notify_unprepare(struct rproc_subdev *subdev) · line 297

Quoted 10 lines
drivers/remoteproc/qcom_common.c +9 -0
+	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

Quoted 62 lines
  	rproc_add_subdev(rproc, &ssr->subdev);
drivers/remoteproc/qcom_common.c +1 -0
@@ -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

drivers/remoteproc/qcom_common.h +2 -2
@@ -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,
include/linux/remoteproc/qcom_rproc.h +15 -2
index fa8e386..58422b1 100644
@@ -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
#4 lore raw

Re: [PATCH v5 1/2] remoteproc: qcom: Add per subsystem SSR notification

Quoted 36 lines · On Tue 23 Jun 18:41 PDT 2020, [email protected] wrote:
Quoted 23 lines · Alex Elder
Quoted 19 lines · Rishabh Bhatnagar

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.

Quoted 2 lines
Signed-off-by Siddharth Gupta <[email protected]>
Signed-off-by Rishabh Bhatnagar <[email protected]>
Quoted 8 lines

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.

Quoted 5 lines
Quoted 3 lines

- 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.

Given that clients make direct function calls into qcom_common.ko, qcom_common.ko would not be possible to rmmod until all clients has been rmmod'ed. As such there shouldn't be any remaining listeners, or subdevices, when this happens.

Regards,
Bjorn

author
#5 lore raw

PATCH v5 2/2 remoteproc: qcom: Add notification types to SSR

In reply to Rishabh Bhatnagar

The SSR subdevice only adds callback for the unprepare event. Add callbacks for prepare, 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 <[email protected]>
Signed-off-by Rishabh Bhatnagar <[email protected]>
---
 drivers/remoteproc/qcom_common.c      | 44 ++++++++++++++++++++++++++++++++++-
 include/linux/remoteproc/qcom_rproc.h | 16 +++++++++++++
 2 files changed, 59 insertions(+), 1 deletion(-)
drivers/remoteproc/qcom_common.c +43 -1
index 658f2ca..0848bf1 100644
@@ -262,6 +262,44 @@ 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);
+	struct qcom_ssr_notif_data data = {
+		.name = ssr->info->name,
+		.crashed = false,
+	};
+
+	srcu_notifier_call_chain(&ssr->info->notifier_list,
+				 QCOM_SSR_BEFORE_POWERUP, &data);
+	return 0;
+}
+
+static int ssr_notify_start(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,
+	};
+
+	srcu_notifier_call_chain(&ssr->info->notifier_list,
+				 QCOM_SSR_AFTER_POWERUP, &data);
+	return 0;
+}
+
+static void ssr_notify_stop(struct rproc_subdev *subdev, bool crashed)
+{
+	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
+	struct qcom_ssr_notif_data data = {
+		.name = ssr->info->name,
+		.crashed = crashed,
+	};
+
+	srcu_notifier_call_chain(&ssr->info->notifier_list,
+				 QCOM_SSR_BEFORE_SHUTDOWN, &data);
+}
+
 static void ssr_notify_unprepare(struct rproc_subdev *subdev)
 {
 	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);

@@ -270,7 +308,8 @@ static void ssr_notify_unprepare(struct rproc_subdev *subdev)
 		.crashed = false,
 	};
 
-	srcu_notifier_call_chain(&ssr->info->notifier_list, 0, &data);
+	srcu_notifier_call_chain(&ssr->info->notifier_list,
+				 QCOM_SSR_AFTER_SHUTDOWN, &data);
 }
 
 /**

@@ -294,6 +333,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,
 	}
 
 	ssr->info = info;
+	ssr->subdev.prepare = ssr_notify_prepare;
+	ssr->subdev.start = ssr_notify_start;
+	ssr->subdev.stop = ssr_notify_stop;
 	ssr->subdev.unprepare = ssr_notify_unprepare;
 
 	rproc_add_subdev(rproc, &ssr->subdev);
include/linux/remoteproc/qcom_rproc.h +16 -0
index 58422b1..83ac8e8 100644
@@ -5,6 +5,22 @@ struct notifier_block;
 
 #if IS_ENABLED(CONFIG_QCOM_RPROC_COMMON)
 
+/**
+ * enum qcom_ssr_notif_type - Startup/Shutdown events related to a remoteproc
+ * processor.
+ *
+ * @QCOM_SSR_BEFORE_POWERUP:	Remoteproc about to start (prepare stage)
+ * @QCOM_SSR_AFTER_POWERUP:	Remoteproc is running (start stage)
+ * @QCOM_SSR_BEFORE_SHUTDOWN:	Remoteproc crashed or shutting down (stop stage)
+ * @QCOM_SSR_AFTER_SHUTDOWN:	Remoteproc is down (unprepare stage)
+ */
+enum qcom_ssr_notif_type {
+	QCOM_SSR_BEFORE_POWERUP,
+	QCOM_SSR_AFTER_POWERUP,
+	QCOM_SSR_BEFORE_SHUTDOWN,
+	QCOM_SSR_AFTER_SHUTDOWN,
+};
+
 struct qcom_ssr_notif_data {
 	const char *name;
 	bool crashed;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Reviewed-by on v5 2/2
#6 lore raw

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

Quoted 8 lines · Rishabh Bhatnagar

The SSR subdevice only adds callback for the unprepare event. Add callbacks for prepare, 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 <[email protected]>
Signed-off-by Rishabh Bhatnagar <[email protected]>

This looks good to me.

Reviewed-by Alex Elder <[email protected]>
Quoted 102 lines
---
  drivers/remoteproc/qcom_common.c      | 44 ++++++++++++++++++++++++++++++++++-
  include/linux/remoteproc/qcom_rproc.h | 16 +++++++++++++
  2 files changed, 59 insertions(+), 1 deletion(-)
drivers/remoteproc/qcom_common.c +43 -1
index 658f2ca..0848bf1 100644
@@ -262,6 +262,44 @@ 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);
+	struct qcom_ssr_notif_data data = {
+		.name = ssr->info->name,
+		.crashed = false,
+	};
+
+	srcu_notifier_call_chain(&ssr->info->notifier_list,
+				 QCOM_SSR_BEFORE_POWERUP, &data);
+	return 0;
+}
+
+static int ssr_notify_start(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,
+	};
+
+	srcu_notifier_call_chain(&ssr->info->notifier_list,
+				 QCOM_SSR_AFTER_POWERUP, &data);
+	return 0;
+}
+
+static void ssr_notify_stop(struct rproc_subdev *subdev, bool crashed)
+{
+	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
+	struct qcom_ssr_notif_data data = {
+		.name = ssr->info->name,
+		.crashed = crashed,
+	};
+
+	srcu_notifier_call_chain(&ssr->info->notifier_list,
+				 QCOM_SSR_BEFORE_SHUTDOWN, &data);
+}
+
  static void ssr_notify_unprepare(struct rproc_subdev *subdev)
  {
  	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);

@@ -270,7 +308,8 @@ static void ssr_notify_unprepare(struct rproc_subdev *subdev)
  		.crashed = false,
  	};
 
-	srcu_notifier_call_chain(&ssr->info->notifier_list, 0, &data);
+	srcu_notifier_call_chain(&ssr->info->notifier_list,
+				 QCOM_SSR_AFTER_SHUTDOWN, &data);
  }
 
  /**

@@ -294,6 +333,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,
  	}
 
  	ssr->info = info;
+	ssr->subdev.prepare = ssr_notify_prepare;
+	ssr->subdev.start = ssr_notify_start;
+	ssr->subdev.stop = ssr_notify_stop;
  	ssr->subdev.unprepare = ssr_notify_unprepare;
 
  	rproc_add_subdev(rproc, &ssr->subdev);
include/linux/remoteproc/qcom_rproc.h +16 -0
index 58422b1..83ac8e8 100644
@@ -5,6 +5,22 @@ struct notifier_block;
 
  #if IS_ENABLED(CONFIG_QCOM_RPROC_COMMON)
 
+/**
+ * enum qcom_ssr_notif_type - Startup/Shutdown events related to a remoteproc
+ * processor.
+ *
+ * @QCOM_SSR_BEFORE_POWERUP:	Remoteproc about to start (prepare stage)
+ * @QCOM_SSR_AFTER_POWERUP:	Remoteproc is running (start stage)
+ * @QCOM_SSR_BEFORE_SHUTDOWN:	Remoteproc crashed or shutting down (stop stage)
+ * @QCOM_SSR_AFTER_SHUTDOWN:	Remoteproc is down (unprepare stage)
+ */
+enum qcom_ssr_notif_type {
+	QCOM_SSR_BEFORE_POWERUP,
+	QCOM_SSR_AFTER_POWERUP,
+	QCOM_SSR_BEFORE_SHUTDOWN,
+	QCOM_SSR_AFTER_SHUTDOWN,
+};
+
  struct qcom_ssr_notif_data {
  	const char *name;
  	bool crashed;