Extend SSR notifications framework

PATCH Superseded Rishabh Bhatnagar 3 participants 7 messages Apr 28, 2020 – May 26, 2020

Subsystems (2)

Arm Qualcomm mailing list Remote processor (remoteproc)

lore

Share & tools

lore

Rishabh Bhatnagar

author

2020-04-28 22:18 UTC #0 lore raw

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

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

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

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

 drivers/remoteproc/qcom_common.c      | 133 ++++++++++++++++++++++++++++++----
 drivers/remoteproc/qcom_common.h      |  11 ++-
 include/linux/remoteproc/qcom_rproc.h |  34 +++++++--
 3 files changed, 157 insertions(+), 21 deletions(-)

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Rishabh Bhatnagar

author

2020-04-28 22:18 UTC #1 lore raw

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

Signed-off-by Siddharth Gupta <[email protected]>

Signed-off-by Rishabh Bhatnagar <[email protected]>

---
 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(-)

drivers/remoteproc/qcom_common.c +76 ~~-13~~

index 60650bc..7cd17be 100644
@@ -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>
 
 #include "remoteproc_internal.h"
 #include "qcom_common.h"
 
+#define MAX_NAME_LEN	20
+DEFINE_MUTEX(rproc_notif_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);
-
 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)
+{
+	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)))
+			return info;
+	}
+	return NULL;
+}
+
 /**
  * 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 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);
+		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
  * @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;
+
+	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);
 }
 
+
 /**
  * qcom_add_ssr_subdev() - register subdevice as restart notification source
  * @rproc:	rproc handle

@@ -214,12 +258,30 @@ 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 rproc_notif_info *info;
+
+	mutex_lock(&rproc_notif_lock);
+	info = find_notif_info(ssr_name);
+	if (!info) {
+		info = kzalloc(sizeof(*info), GFP_KERNEL);
+		if (!info) {
+			mutex_unlock(&rproc_notif_lock);
+			return;
+		}
+		info->name = ssr_name;
+		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);
+	}
+	mutex_unlock(&rproc_notif_lock);
+	ssr->info = info;
 	ssr->subdev.unprepare = ssr_notify_unprepare;
 
 	rproc_add_subdev(rproc, &ssr->subdev);

@@ -233,6 +295,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)
 {
+	ssr->info = NULL;
 	rproc_remove_subdev(rproc, &ssr->subdev);
 }
 EXPORT_SYMBOL_GPL(qcom_remove_ssr_subdev);

drivers/remoteproc/qcom_common.h +8 -2

index 58de71e..0c1d288 100644
@@ -24,10 +24,16 @@ struct qcom_rproc_subdev {
 	struct qcom_smd_edge *edge;
 };
 
+struct rproc_notif_info {
+	const char *name;
+	struct srcu_notifier_head notifier_list;
+	struct list_head list;
+};
+static LIST_HEAD(rproc_notif_list);
+
 struct qcom_rproc_ssr {
 	struct rproc_subdev subdev;
-
-	const char *name;
+	struct rproc_notif_info *info;
 };
 
 void qcom_add_glink_subdev(struct rproc *rproc, struct qcom_rproc_glink *glink);

include/linux/remoteproc/qcom_rproc.h +15 -5

index fa8e386..3dc65c0 100644
@@ -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

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

kbuild test robot

build bot

2020-04-29 06:15 UTC #2 lore raw

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

Thank you for the patch! Yet something to improve:

Hi Rishabh,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.7-rc3 next-20200428]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Rishabh-Bhatnagar/Extend-SSR-notifications-framework/20200429-071958
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 96c9a7802af7d500a582d89a8b864584fe878c1b
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0

reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=arm64

If you fix the issue, kindly add following tag as appropriate

Reported-by kbuild test robot <[email protected]>

All error/warnings (new ones prefixed by >>):

drivers/soc/qcom/glink_ssr.c: In function 'qcom_glink_ssr_probe':

>> drivers/soc/qcom/glink_ssr.c:128:36: error: passing argument 1 of 'qcom_register_ssr_notifier' from incompatible pointer type [-Werror=incompatible-pointer-types]
     128 |  return qcom_register_ssr_notifier(&ssr->nb);
         |                                    ^~~~~~~~
         |                                    |
         |                                    struct notifier_block *
   In file included from drivers/soc/qcom/glink_ssr.c:11:
   include/linux/remoteproc/qcom_rproc.h:13:7: note: expected 'const char *' but argument is of type 'struct notifier_block *'
      13 | void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb);
         |       ^~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/soc/qcom/glink_ssr.c:128:9: error: too few arguments to function 'qcom_register_ssr_notifier'
     128 |  return qcom_register_ssr_notifier(&ssr->nb);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from drivers/soc/qcom/glink_ssr.c:11:
   include/linux/remoteproc/qcom_rproc.h:13:7: note: declared here
      13 | void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb);
         |       ^~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/soc/qcom/glink_ssr.c: In function 'qcom_glink_ssr_remove':
>> drivers/soc/qcom/glink_ssr.c:135:2: error: too few arguments to function 'qcom_unregister_ssr_notifier'
     135 |  qcom_unregister_ssr_notifier(&ssr->nb);
         |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from drivers/soc/qcom/glink_ssr.c:11:
   include/linux/remoteproc/qcom_rproc.h:14:5: note: declared here
      14 | int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb);
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/soc/qcom/glink_ssr.c: In function 'qcom_glink_ssr_probe':
>> drivers/soc/qcom/glink_ssr.c:129:1: warning: control reaches end of non-void function [-Wreturn-type]
     129 | }
         | ^

cc1: some warnings being treated as errors

vim +/qcom_register_ssr_notifier +128 drivers/soc/qcom/glink_ssr.c

c4d77d5fcd8bae Bjorn Andersson 2017-07-24  111  
c4d77d5fcd8bae Bjorn Andersson 2017-07-24  112  static int qcom_glink_ssr_probe(struct rpmsg_device *rpdev)
c4d77d5fcd8bae Bjorn Andersson 2017-07-24  113  {
c4d77d5fcd8bae Bjorn Andersson 2017-07-24  114  	struct glink_ssr *ssr;
c4d77d5fcd8bae Bjorn Andersson 2017-07-24  115  
c4d77d5fcd8bae Bjorn Andersson 2017-07-24  116  	ssr = devm_kzalloc(&rpdev->dev, sizeof(*ssr), GFP_KERNEL);
c4d77d5fcd8bae Bjorn Andersson 2017-07-24  117  	if (!ssr)
c4d77d5fcd8bae Bjorn Andersson 2017-07-24  118  		return -ENOMEM;
c4d77d5fcd8bae Bjorn Andersson 2017-07-24  119  
c4d77d5fcd8bae Bjorn Andersson 2017-07-24  120  	init_completion(&ssr->completion);
c4d77d5fcd8bae Bjorn Andersson 2017-07-24  121  
c4d77d5fcd8bae Bjorn Andersson 2017-07-24  122  	ssr->dev = &rpdev->dev;
c4d77d5fcd8bae Bjorn Andersson 2017-07-24  123  	ssr->ept = rpdev->ept;
c4d77d5fcd8bae Bjorn Andersson 2017-07-24  124  	ssr->nb.notifier_call = qcom_glink_ssr_notify;
c4d77d5fcd8bae Bjorn Andersson 2017-07-24  125  
c4d77d5fcd8bae Bjorn Andersson 2017-07-24  126  	dev_set_drvdata(&rpdev->dev, ssr);
c4d77d5fcd8bae Bjorn Andersson 2017-07-24  127  
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 @128  	return qcom_register_ssr_notifier(&ssr->nb);
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 @129  }
c4d77d5fcd8bae Bjorn Andersson 2017-07-24  130  
c4d77d5fcd8bae Bjorn Andersson 2017-07-24  131  static void qcom_glink_ssr_remove(struct rpmsg_device *rpdev)
c4d77d5fcd8bae Bjorn Andersson 2017-07-24  132  {
c4d77d5fcd8bae Bjorn Andersson 2017-07-24  133  	struct glink_ssr *ssr = dev_get_drvdata(&rpdev->dev);
c4d77d5fcd8bae Bjorn Andersson 2017-07-24  134

c4d77d5fcd8bae Bjorn Andersson 2017-07-24 @135 qcom_unregister_ssr_notifier(&ssr->nb);
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 136 }
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 137

:::::: The code at line 128 was first introduced by commit
:::::: c4d77d5fcd8baefb358ae921b940e9bbd09a751e soc: qcom: GLINK SSR notifier

:::::: TO: Bjorn Andersson <[email protected]> :::::: CC: Bjorn Andersson <[email protected]>

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Bjorn Andersson

2020-05-19 20:40 UTC #3 lore raw

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

In reply to Rishabh Bhatnagar

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

Quoted 2 lines

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

Thanks Rishabh, design wise I think this looks good now, just some code style things below.

Quoted 11 lines

---
 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(-)

drivers/remoteproc/qcom_common.c +0 -0

index 60650bc..7cd17be 100644

drivers/remoteproc/qcom_common.c line 18

Quoted 4 lines

drivers/remoteproc/qcom_common.c +1 -0

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

drivers/remoteproc/qcom_common.c line 24

Quoted 6 lines

drivers/remoteproc/qcom_common.c +2 -0

 #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

drivers/remoteproc/qcom_common.c line 26

Quoted 6 lines

drivers/remoteproc/qcom_common.c +1 -1

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

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

Quoted 9 lines

drivers/remoteproc/qcom_common.c +1 -1

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

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

Quoted 6 lines

drivers/remoteproc/qcom_common.c +6 -0

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

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

Quoted 3 lines

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

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

Quoted 9 lines

drivers/remoteproc/qcom_common.c +4 -1

+}
+
 /**
  * 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".

Quoted 13 lines

drivers/remoteproc/qcom_common.c +5 -5

  *
- * 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;

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

Quoted 10 lines

drivers/remoteproc/qcom_common.c +10 -0

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

Quoted 6 lines

drivers/remoteproc/qcom_common.c +6 -0

+		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);
+	}

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

Quoted 10 lines

drivers/remoteproc/qcom_common.c +5 -0

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

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

Quoted 8 lines

drivers/remoteproc/qcom_common.c +3 -2

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

Quoted 5 lines

drivers/remoteproc/qcom_common.c +2 -0

+
+	return srcu_notifier_chain_unregister(notify, nb);
 }
 EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);

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

Quoted 10 lines

drivers/remoteproc/qcom_common.c +5 -1

 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?

Quoted 40 lines

drivers/remoteproc/qcom_common.c +21 -2

 }
 
+
 /**
  * qcom_add_ssr_subdev() - register subdevice as restart notification source
  * @rproc:	rproc handle

@@ -214,12 +258,30 @@ 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 rproc_notif_info *info;
+
+	mutex_lock(&rproc_notif_lock);
+	info = find_notif_info(ssr_name);
+	if (!info) {
+		info = kzalloc(sizeof(*info), GFP_KERNEL);
+		if (!info) {
+			mutex_unlock(&rproc_notif_lock);
+			return;
+		}
+		info->name = ssr_name;
+		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);
+	}
+	mutex_unlock(&rproc_notif_lock);
+	ssr->info = info;
 	ssr->subdev.unprepare = ssr_notify_unprepare;
 
 	rproc_add_subdev(rproc, &ssr->subdev);

drivers/remoteproc/qcom_common.c void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr, · line 298

Quoted 4 lines

drivers/remoteproc/qcom_common.c +1 -0

@@ -233,6 +295,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)
 {
+	ssr->info = NULL;

Move this after rproc_remove_subdev() and rely on the core for this not to race with the ssr_notify_unprepare().

drivers/remoteproc/qcom_common.c void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr, · line 301

Quoted 3 lines

drivers/remoteproc/qcom_common.c +0 -0

 	rproc_remove_subdev(rproc, &ssr->subdev);
 }
 EXPORT_SYMBOL_GPL(qcom_remove_ssr_subdev);

It would be nice with a module_exit() that walks the rproc_notif_list and free all the elements, if qcom_common.ko is rmmod'ed. Given that this is uncommon I wouldn't mind to take that as a separate patch though.

drivers/remoteproc/qcom_common.h struct qcom_rproc_subdev { · line 27

Quoted 9 lines

drivers/remoteproc/qcom_common.h +1 -0

index 58de71e..0c1d288 100644
@@ -24,10 +24,16 @@ struct qcom_rproc_subdev {
 	struct qcom_smd_edge *edge;
 };
 
+struct rproc_notif_info {

Please rename this struct qcom_ssr_subsystem

drivers/remoteproc/qcom_common.h struct qcom_rproc_subdev { · line 32

Quoted 5 lines

drivers/remoteproc/qcom_common.h +5 -0

+	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;

drivers/remoteproc/qcom_common.h struct qcom_rproc_subdev { · line 38

Quoted 8 lines

drivers/remoteproc/qcom_common.h +2 -2

+
 struct qcom_rproc_ssr {
 	struct rproc_subdev subdev;
-
-	const char *name;
+	struct rproc_notif_info *info;
 };

Regards,
Bjorn

Quoted 41 lines

drivers/remoteproc/qcom_common.h +0 -0

 void qcom_add_glink_subdev(struct rproc *rproc, struct qcom_rproc_glink *glink);

include/linux/remoteproc/qcom_rproc.h +15 -5

index fa8e386..3dc65c0 100644
@@ -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

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[email protected]

author

2020-05-26 18:04 UTC #4 lore raw

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

Quoted 188 lines · Bjorn Andersson

Quoted 10 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.
Quoted 2 lines
Signed-off-by Siddharth Gupta <[email protected]>
Signed-off-by Rishabh Bhatnagar <[email protected]>
Thanks Rishabh, design wise I think this looks good now, just some code style things below.
Quoted 12 lines
---
 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(-)
drivers/remoteproc/qcom_common.c +0 -0
index 60650bc..7cd17be 100644
drivers/remoteproc/qcom_common.c line 18
Quoted 4 lines
drivers/remoteproc/qcom_common.c +1 -0
@@ -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.
drivers/remoteproc/qcom_common.c line 24
Quoted 6 lines
drivers/remoteproc/qcom_common.c +2 -0
 #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
drivers/remoteproc/qcom_common.c line 28
Quoted 9 lines
drivers/remoteproc/qcom_common.c +1 -1
+
 #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.
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, · line 180
Quoted 10 lines
drivers/remoteproc/qcom_common.c +0 -1
-
 static int glink_subdev_start(struct rproc_subdev *subdev)
{
drivers/remoteproc/qcom_common.c +1 -0
 	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)
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 185
Quoted 6 lines
drivers/remoteproc/qcom_common.c +6 -0
+{
+	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.
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 188
Quoted 3 lines
drivers/remoteproc/qcom_common.c +3 -0
+			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.
Quoted 11 lines
drivers/remoteproc/qcom_common.c +3 -0
+}
+
 /**
  * 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".
Quoted 17 lines
*
- * 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. */
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 +1 -1
-	return blocking_notifier_chain_register(&ssr_notifiers, nb);
+	struct rproc_notif_info *info;
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 214
Quoted 10 lines
drivers/remoteproc/qcom_common.c +10 -0
+
+	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?
Quoted 15 lines
drivers/remoteproc/qcom_common.c +10 -0
+		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 
Quoted 2 lines
handler
+ * @notify: pointer to srcu notifier head
@subsystem: subsystem cookie returned from
qcom_register_ssr_notifier
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 236
Quoted 9 lines
* @nb: notifier_block to unregister
*/
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 +2 -1
-	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.
Quoted 13 lines
drivers/remoteproc/qcom_common.c +6 -0
+
+	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,
+	};

Quoted 6 lines

drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 250
Quoted 2 lines
drivers/remoteproc/qcom_common.c +1 -1
-	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?

Since the notifier list needs to be dynamically added for every subsystem using srcu notifier makes sense. I'm not sure if we can use blocking notifier dynamically here.

Quoted 56 lines

Quoted 46 lines
}
+
/**
* qcom_add_ssr_subdev() - register subdevice as restart notification
source
* @rproc: rproc handle
drivers/remoteproc/qcom_common.c +0 -0
@@ -214,12 +258,30 @@ 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,
drivers/remoteproc/qcom_common.c +19 -1
 			 const char *ssr_name)
 {
-	ssr->name = ssr_name;
+	struct rproc_notif_info *info;
+
+	mutex_lock(&rproc_notif_lock);
+	info = find_notif_info(ssr_name);
+	if (!info) {
+		info = kzalloc(sizeof(*info), GFP_KERNEL);
+		if (!info) {
+			mutex_unlock(&rproc_notif_lock);
+			return;
+		}
+		info->name = ssr_name;
+		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);
+	}
+	mutex_unlock(&rproc_notif_lock);
+	ssr->info = info;
 	ssr->subdev.unprepare = ssr_notify_unprepare;
 
 	rproc_add_subdev(rproc, &ssr->subdev);
drivers/remoteproc/qcom_common.c void qcom_add_ssr_subdev(struct rproc *rproc, · line 300
Quoted 6 lines
drivers/remoteproc/qcom_common.c +1 -0
@@ -233,6 +295,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)
 {
+	ssr->info = NULL;
Move this after rproc_remove_subdev() and rely on the core for this not to race with the ssr_notify_unprepare().

Quoted 9 lines

Quoted 3 lines
drivers/remoteproc/qcom_common.c +0 -0
 	rproc_remove_subdev(rproc, &ssr->subdev);
}
EXPORT_SYMBOL_GPL(qcom_remove_ssr_subdev);
It would be nice with a module_exit() that walks the rproc_notif_list and free all the elements, if qcom_common.ko is rmmod'ed. Given that this is uncommon I wouldn't mind to take that as a separate patch though.

Yes i can submit that as a separate patch.

Quoted 85 lines

drivers/remoteproc/qcom_common.h struct qcom_rproc_subdev { · line 27
Quoted 9 lines
drivers/remoteproc/qcom_common.h +1 -0
index 58de71e..0c1d288 100644
@@ -24,10 +24,16 @@ struct qcom_rproc_subdev {
 	struct qcom_smd_edge *edge;
 };
 
+struct rproc_notif_info {
Please rename this struct qcom_ssr_subsystem
drivers/remoteproc/qcom_common.h struct qcom_rproc_subdev { · line 32
Quoted 5 lines
drivers/remoteproc/qcom_common.h +5 -0
+	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;
drivers/remoteproc/qcom_common.h struct qcom_rproc_subdev { · line 38
Quoted 8 lines
drivers/remoteproc/qcom_common.h +2 -2
+
 struct qcom_rproc_ssr {
 	struct rproc_subdev subdev;
-
-	const char *name;
+	struct rproc_notif_info *info;
 };
 
Regards,
Bjorn
Quoted 46 lines
drivers/remoteproc/qcom_common.h +0 -0
 void qcom_add_glink_subdev(struct rproc *rproc, struct 
qcom_rproc_glink *glink);
include/linux/remoteproc/qcom_rproc.h +10 -5
index fa8e386..3dc65c0 100644
@@ -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) {}
include/linux/remoteproc/qcom_rproc.h +5 -1
+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

Rishabh Bhatnagar

author

2020-04-28 22:18 UTC #5 lore raw

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

In reply to Rishabh Bhatnagar

From: Siddharth Gupta <[email protected]>

The SSR subdevice only adds callback for the unprepare event. Add callbacks for unprepare, 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      | 46 +++++++++++++++++++++++++++++++++--
 include/linux/remoteproc/qcom_rproc.h | 14 +++++++++++
 2 files changed, 58 insertions(+), 2 deletions(-)

drivers/remoteproc/qcom_common.c +44 -2

index 7cd17be..0d91cf3 100644
@@ -197,7 +197,7 @@ struct rproc_notif_info *find_notif_info(const char *name)
  *
  * 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.
+ * will be invoked when the particular remote processor is started/stopped.
  */
 void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb)
 {

@@ -239,6 +239,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 rproc_notif_data data = {
+		.name = ssr->info->name,
+		.crashed = false,
+	};
+
+	srcu_notifier_call_chain(&ssr->info->notifier_list,
+				 RPROC_BEFORE_POWERUP, &data);
+	return 0;
+}
+
+static int ssr_notify_start(struct rproc_subdev *subdev)
+{
+	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
+	struct rproc_notif_data data = {
+		.name = ssr->info->name,
+		.crashed = false,
+	};
+
+	srcu_notifier_call_chain(&ssr->info->notifier_list,
+				 RPROC_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 rproc_notif_data data = {
+		.name = ssr->info->name,
+		.crashed = crashed,
+	};
+
+	srcu_notifier_call_chain(&ssr->info->notifier_list,
+				 RPROC_BEFORE_SHUTDOWN, &data);
+}
+
 static void ssr_notify_unprepare(struct rproc_subdev *subdev)
 {
 	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);

@@ -247,7 +285,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,
+				 RPROC_AFTER_SHUTDOWN, &data);
 }

@@ -282,6 +321,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,
 	}
 	mutex_unlock(&rproc_notif_lock);
 	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 +14 -0

index 3dc65c0..567c1f9 100644
@@ -5,6 +5,20 @@
 
 #if IS_ENABLED(CONFIG_QCOM_RPROC_COMMON)
 
+/**
+ * enum rproc_notif_type - Different stages of remoteproc notifications
+ * @RPROC_BEFORE_SHUTDOWN:	unprepare stage of  remoteproc
+ * @RPROC_AFTER_SHUTDOWN:	stop stage of  remoteproc
+ * @RPROC_BEFORE_POWERUP:	prepare stage of  remoteproc
+ * @RPROC_AFTER_POWERUP:	start stage of  remoteproc
+ */
+enum rproc_notif_type {
+	RPROC_BEFORE_SHUTDOWN,
+	RPROC_AFTER_SHUTDOWN,
+	RPROC_BEFORE_POWERUP,
+	RPROC_AFTER_POWERUP,
+};
+
 struct rproc_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

Bjorn Andersson

2020-05-19 20:41 UTC #6 lore raw

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

Quoted 103 lines · Rishabh Bhatnagar

From: Siddharth Gupta <[email protected]>

Signed-off-by Siddharth Gupta <[email protected]>

Signed-off-by Rishabh Bhatnagar <[email protected]>

---
 drivers/remoteproc/qcom_common.c      | 46 +++++++++++++++++++++++++++++++++--
 include/linux/remoteproc/qcom_rproc.h | 14 +++++++++++
 2 files changed, 58 insertions(+), 2 deletions(-)

drivers/remoteproc/qcom_common.c +44 -2

index 7cd17be..0d91cf3 100644
@@ -197,7 +197,7 @@ struct rproc_notif_info *find_notif_info(const char *name)
  *
  * 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.
+ * will be invoked when the particular remote processor is started/stopped.
  */
 void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb)
 {

@@ -239,6 +239,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 rproc_notif_data data = {
+		.name = ssr->info->name,
+		.crashed = false,
+	};
+
+	srcu_notifier_call_chain(&ssr->info->notifier_list,
+				 RPROC_BEFORE_POWERUP, &data);
+	return 0;
+}
+
+static int ssr_notify_start(struct rproc_subdev *subdev)
+{
+	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
+	struct rproc_notif_data data = {
+		.name = ssr->info->name,
+		.crashed = false,
+	};
+
+	srcu_notifier_call_chain(&ssr->info->notifier_list,
+				 RPROC_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 rproc_notif_data data = {
+		.name = ssr->info->name,
+		.crashed = crashed,
+	};
+
+	srcu_notifier_call_chain(&ssr->info->notifier_list,
+				 RPROC_BEFORE_SHUTDOWN, &data);
+}
+
 static void ssr_notify_unprepare(struct rproc_subdev *subdev)
 {
 	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);

@@ -247,7 +285,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,
+				 RPROC_AFTER_SHUTDOWN, &data);
 }

@@ -282,6 +321,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,
 	}
 	mutex_unlock(&rproc_notif_lock);
 	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 +2 -0

index 3dc65c0..567c1f9 100644
@@ -5,6 +5,20 @@
 
 #if IS_ENABLED(CONFIG_QCOM_RPROC_COMMON)
 
+/**
+ * enum rproc_notif_type - Different stages of remoteproc notifications

include/linux/remoteproc/qcom_rproc.h line 19

Quoted 10 lines

include/linux/remoteproc/qcom_rproc.h +10 -0

+ * @RPROC_BEFORE_SHUTDOWN:	unprepare stage of  remoteproc
+ * @RPROC_AFTER_SHUTDOWN:	stop stage of  remoteproc
+ * @RPROC_BEFORE_POWERUP:	prepare stage of  remoteproc
+ * @RPROC_AFTER_POWERUP:	start stage of  remoteproc
+ */
+enum rproc_notif_type {
+	RPROC_BEFORE_SHUTDOWN,
+	RPROC_AFTER_SHUTDOWN,
+	RPROC_BEFORE_POWERUP,
+	RPROC_AFTER_POWERUP,

Given that these are not generic remoteproc things I would like a qcom prefix on them.

How about QCOM_SSR_BEFORE_SHUTDOWN ...

Apart from that, this looks good.

Thanks,
Bjorn

Quoted 8 lines

include/linux/remoteproc/qcom_rproc.h +2 -0

+};
+
 struct rproc_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