[PATCH v4 0/2] Extend SSR notifications framework

[PATCH v4 0/2] Extend SSR notifications framework

[Rishabh Bhatnagar]

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:
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      | 126 ++++++++++++++++++++++++++++++----
 drivers/remoteproc/qcom_common.h      |   5 +-
 include/linux/remoteproc/qcom_rproc.h |  34 +++++++--
 3 files changed, 146 insertions(+), 19 deletions(-)

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

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

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

Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
---
 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);
 
 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)
+{
+	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;
+	}
+	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 */
+	INIT_LIST_HEAD(&info->list);
+	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
  * @nb:		notifier_block to notify for restart notifications
  *
- * Returns 0 on success, negative errno on failure.
+ * Returns a subsystem cookie 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 qcom_ssr_subsystem *info;
+
+	mutex_lock(&qcom_ssr_subsys_lock);
+	info = qcom_ssr_get_subsys(name);
+	if (IS_ERR(info)) {
+		mutex_unlock(&qcom_ssr_subsys_lock);
+		return info;
+	}
+
+	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
  */
-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 (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;
+};
+
+void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb);
+int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb);
 
 #else
 
-static inline int qcom_register_ssr_notifier(struct notifier_block *nb)
+static inline void *qcom_register_ssr_notifier(const char *name,
+					       struct notifier_block *nb)
 {
-	return 0;
+	return NULL;
 }
 
-static inline void qcom_unregister_ssr_notifier(struct notifier_block *nb) {}
+static inline int qcom_unregister_ssr_notifier(void *notify,
+					       struct notifier_block *nb)
+{
+	return 0;
+}
 
 #endif
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Rishabh Bhatnagar]

From: Siddharth Gupta <sidgup@codeaurora.org>

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 <sidgup@codeaurora.org>
Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
---
 drivers/remoteproc/qcom_common.c      | 46 +++++++++++++++++++++++++++++++++--
 include/linux/remoteproc/qcom_rproc.h | 14 +++++++++++
 2 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 61ff2dd..5c5a1eb 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -228,7 +228,7 @@ struct qcom_ssr_subsystem *qcom_ssr_get_subsys(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)
 {
@@ -258,6 +258,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);
@@ -266,7 +304,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,
 
 	mutex_unlock(&qcom_ssr_subsys_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);
diff --git a/include/linux/remoteproc/qcom_rproc.h b/include/linux/remoteproc/qcom_rproc.h
index 58422b1..a558183 100644
--- a/include/linux/remoteproc/qcom_rproc.h
+++ b/include/linux/remoteproc/qcom_rproc.h
@@ -5,6 +5,20 @@
 
 #if IS_ENABLED(CONFIG_QCOM_RPROC_COMMON)
 
+/**
+ * enum qcom_ssr_notif_type - Different stages of remoteproc notifications
+ * @QCOM_SSR_BEFORE_SHUTDOWN:	unprepare stage of  remoteproc
+ * @QCOM_SSR_AFTER_SHUTDOWN:	stop stage of  remoteproc
+ * @QCOM_SSR_BEFORE_POWERUP:	prepare stage of  remoteproc
+ * @QCOM_SSR_AFTER_POWERUP:	start stage of  remoteproc
+ */
+enum qcom_ssr_notif_type {
+	QCOM_SSR_BEFORE_SHUTDOWN,
+	QCOM_SSR_AFTER_SHUTDOWN,
+	QCOM_SSR_BEFORE_POWERUP,
+	QCOM_SSR_AFTER_POWERUP,
+};
+
 struct qcom_ssr_notif_data {
 	const char *name;
 	bool crashed;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2382 bytes --]

Hi Rishabh,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20200526]
[also build test WARNING on v5.7-rc7]
[cannot apply to linus/master linux/master v5.7-rc7 v5.7-rc6 v5.7-rc5]
[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/20200528-115948
base:    b0523c7b1c9d0edcd6c0fe6d2cb558a9ad5c60a8
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        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 COMPILER=gcc-9.3.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> drivers/remoteproc/qcom_common.c:200:28: warning: no previous prototype for 'qcom_ssr_get_subsys' [-Wmissing-prototypes]
200 | struct qcom_ssr_subsystem *qcom_ssr_get_subsys(const char *name)
|                            ^~~~~~~~~~~~~~~~~~~

vim +/qcom_ssr_get_subsys +200 drivers/remoteproc/qcom_common.c

   199	
 > 200	struct qcom_ssr_subsystem *qcom_ssr_get_subsys(const char *name)
   201	{
   202		struct qcom_ssr_subsystem *info;
   203	
   204		/* Match in the global qcom_ssr_subsystem_list with name */
   205		list_for_each_entry(info, &qcom_ssr_subsystem_list, list) {
   206			if (!strcmp(info->name, name))
   207				return info;
   208		}
   209		info = kzalloc(sizeof(*info), GFP_KERNEL);
   210		if (!info)
   211			return ERR_PTR(-ENOMEM);
   212		info->name = kstrdup_const(name, GFP_KERNEL);
   213		srcu_init_notifier_head(&info->notifier_list);
   214	
   215		/* Add to global notif list */
   216		INIT_LIST_HEAD(&info->list);
   217		list_add_tail(&info->list, &qcom_ssr_subsystem_list);
   218	
   219		return info;
   220	}
   221	

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 72782 bytes --]

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

[Alex Elder]

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@codeaurora.org>
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> ---
>   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
>   
> 

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

[Alex Elder]

On 5/27/20 10:34 PM, Rishabh Bhatnagar wrote:
> From: Siddharth Gupta <sidgup@codeaurora.org>
> 
> The SSR subdevice only adds callback for the unprepare event. Add callbacks
> for unprepare, start and prepare events. The client driver for a particular

   for prepare, start, and stop events

> 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 <sidgup@codeaurora.org>
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> ---
>   drivers/remoteproc/qcom_common.c      | 46 +++++++++++++++++++++++++++++++++--
>   include/linux/remoteproc/qcom_rproc.h | 14 +++++++++++
>   2 files changed, 58 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
> index 61ff2dd..5c5a1eb 100644
> --- a/drivers/remoteproc/qcom_common.c
> +++ b/drivers/remoteproc/qcom_common.c
> @@ -228,7 +228,7 @@ struct qcom_ssr_subsystem *qcom_ssr_get_subsys(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.

Maybe something more like:
   will be invoked when an SSR event occurs for the named
   remote processor.

>    */
>   void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb)
>   {
> @@ -258,6 +258,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);
> @@ -266,7 +304,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,
>   
>   	mutex_unlock(&qcom_ssr_subsys_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);
> diff --git a/include/linux/remoteproc/qcom_rproc.h b/include/linux/remoteproc/qcom_rproc.h
> index 58422b1..a558183 100644
> --- a/include/linux/remoteproc/qcom_rproc.h
> +++ b/include/linux/remoteproc/qcom_rproc.h
> @@ -5,6 +5,20 @@
>   
>   #if IS_ENABLED(CONFIG_QCOM_RPROC_COMMON)
>   
> +/**
> + * enum qcom_ssr_notif_type - Different stages of remoteproc notifications
> + * @QCOM_SSR_BEFORE_SHUTDOWN:	unprepare stage of  remoteproc
> + * @QCOM_SSR_AFTER_SHUTDOWN:	stop stage of  remoteproc
> + * @QCOM_SSR_BEFORE_POWERUP:	prepare stage of  remoteproc
> + * @QCOM_SSR_AFTER_POWERUP:	start stage of  remoteproc

I think your explanations of these symbols are less clear than
the symbol names themselves...  In any case, I wouldn't refer
to these as "stages of notifications" but instead something
more like startup/shutdown related events for a remote processor.

I personally might have ordered them differently too:
So maybe more like:
	BEFORE_POWERUP	Remoteproc about to start (prepare)
	AFTER_POWERUP	Remoteproc is running (start)
	BEFORE_SHUTDOWN	Remoteproc crashed, or shutting down (stop)
	AFTER_SHUTDOWN	Remoteproc is down (unprepare)

					-Alex
	
> + */
> +enum qcom_ssr_notif_type {
> +	QCOM_SSR_BEFORE_SHUTDOWN,
> +	QCOM_SSR_AFTER_SHUTDOWN,
> +	QCOM_SSR_BEFORE_POWERUP,
> +	QCOM_SSR_AFTER_POWERUP,
> +};
> +
>   struct qcom_ssr_notif_data {
>   	const char *name;
>   	bool crashed;
> 

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

[Bjorn Andersson]

On Thu 18 Jun 16:00 PDT 2020, Alex Elder wrote:

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

Thanks for your review Alex, some feedback on the patch and your
response below.

> > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> > Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> > ---
> >   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).
> 

Yes.

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

I agree, that would simplify reasoning about the lock.

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

Info is a struct qcom_ssr_subsystem * and that's the function's return
type as well, so Rishabh's approach looks correct to me.

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

No @ in that, but "Return: foo" is the appropriate format.

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

And this description of the function should go above the Return:

See https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation

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

You're right...that shouldn't happen. So a WARN_ON() and early return
would be in order.

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

I looked at something similar for the subdev callbacks, but concluded
that most cases I could find was cleaner if I just passed a bool crashed
than having two separate functions to deal with in the client drivers.

So I'm okay with this.

Regards,
Bjorn

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

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

[rishabhb]

On 2020-06-18 16:35, Bjorn Andersson wrote:
> On Thu 18 Jun 16:00 PDT 2020, Alex Elder wrote:
> 
>> 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
>> 
> 
> Thanks for your review Alex, some feedback on the patch and your
> response below.
> 
>> > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
>> > Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
>> > ---
>> >   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).
>> 
> 
> Yes.
> 
>> 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.
>> 
> 
> I agree, that would simplify reasoning about the lock.
> 
>> > +{
>> > +	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);
> 
> Info is a struct qcom_ssr_subsystem * and that's the function's return
> type as well, so Rishabh's approach looks correct to me.
> 
>> 
>> > +	}
>> 
>> 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.
>> 
> 
> No @ in that, but "Return: foo" is the appropriate format.
> 
>> >    *
>> > - * 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).
>> 
> 
> And this description of the function should go above the Return:
> 
> See
> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation
> 
>> >    */
>> > -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
>> 
> 
> You're right...that shouldn't happen. So a WARN_ON() and early return
> would be in order.
> 
the info structure needs to be embedded in the qcom_rproc_ssr struct in 
case
where clients register for notifications even before that particular ssr
subdevice is registered. Logically add_ssr_subdev shouldn't happen twice 
for
a rproc without doing remove.
>> > +	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.
>> 
> 
> I looked at something similar for the subdev callbacks, but concluded
> that most cases I could find was cleaner if I just passed a bool 
> crashed
> than having two separate functions to deal with in the client drivers.
> 
> So I'm okay with this.
> 
> Regards,
> Bjorn
> 
>> > +};
>> > +
>> > +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
>> >
>> 

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

[Bjorn Andersson]

On Sat 20 Jun 12:48 PDT 2020, rishabhb@codeaurora.org wrote:

> On 2020-06-18 16:35, Bjorn Andersson wrote:
> > On Thu 18 Jun 16:00 PDT 2020, Alex Elder wrote:
> > 
> > > 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
> > > 
> > 
> > Thanks for your review Alex, some feedback on the patch and your
> > response below.
> > 
> > > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> > > > Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> > > > ---
> > > >   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).
> > > 
> > 
> > Yes.
> > 
> > > 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.
> > > 
> > 
> > I agree, that would simplify reasoning about the lock.
> > 
> > > > +{
> > > > +	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);
> > 
> > Info is a struct qcom_ssr_subsystem * and that's the function's return
> > type as well, so Rishabh's approach looks correct to me.
> > 
> > > 
> > > > +	}
> > > 
> > > 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.
> > > 
> > 
> > No @ in that, but "Return: foo" is the appropriate format.
> > 
> > > >    *
> > > > - * 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).
> > > 
> > 
> > And this description of the function should go above the Return:
> > 
> > See
> > https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation
> > 
> > > >    */
> > > > -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
> > > 
> > 
> > You're right...that shouldn't happen. So a WARN_ON() and early return
> > would be in order.
> > 
> the info structure needs to be embedded in the qcom_rproc_ssr struct in case
> where clients register for notifications even before that particular ssr
> subdevice is registered. Logically add_ssr_subdev shouldn't happen twice for
> a rproc without doing remove.

You're right, I forgot that part of it. So if anything we would have a
check to see that the given info isn't already associated with a
remoteproc instance. But I don't see a link in that direction, so I'm
fine with ignoring this.

Regards,
Bjorn