Linux-remoteproc Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3 0/2] Extend SSR notifications framework
@ 2020-04-28 22:16 Rishabh Bhatnagar
  2020-04-28 22:16 ` [PATCH v3 1/2] remoteproc: qcom: Add per subsystem SSR notification Rishabh Bhatnagar
  2020-04-28 22:16 ` [PATCH v3 2/2] remoteproc: qcom: Add notification types to SSR Rishabh Bhatnagar
  0 siblings, 2 replies; 7+ messages in thread
From: Rishabh Bhatnagar @ 2020-04-28 22:16 UTC (permalink / raw)
  To: linux-remoteproc, bjorn.andersson
  Cc: ohad, linux-kernel, tsoni, psodagud, sidgup, 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. 

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

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v3 1/2] remoteproc: qcom: Add per subsystem SSR notification
  2020-04-28 22:16 [PATCH v3 0/2] Extend SSR notifications framework Rishabh Bhatnagar
@ 2020-04-28 22:16 ` Rishabh Bhatnagar
  2020-04-29  6:15   ` kbuild test robot
  2020-05-19 20:38   ` Bjorn Andersson
  2020-04-28 22:16 ` [PATCH v3 2/2] remoteproc: qcom: Add notification types to SSR Rishabh Bhatnagar
  1 sibling, 2 replies; 7+ messages in thread
From: Rishabh Bhatnagar @ 2020-04-28 22:16 UTC (permalink / raw)
  To: linux-remoteproc, bjorn.andersson
  Cc: ohad, linux-kernel, tsoni, psodagud, sidgup, 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.

Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
---
 drivers/remoteproc/qcom_common.c      | 89 ++++++++++++++++++++++++++++++-----
 drivers/remoteproc/qcom_common.h      | 10 +++-
 include/linux/remoteproc/qcom_rproc.h | 20 ++++++--
 3 files changed, 99 insertions(+), 20 deletions(-)

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 60650bc..7cd17be 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -15,16 +15,18 @@
 #include <linux/rpmsg/qcom_glink.h>
 #include <linux/rpmsg/qcom_smd.h>
 #include <linux/soc/qcom/mdt_loader.h>
+#include <linux/remoteproc/qcom_rproc.h>
 
 #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);
diff --git a/drivers/remoteproc/qcom_common.h b/drivers/remoteproc/qcom_common.h
index 58de71e..0c1d288 100644
--- a/drivers/remoteproc/qcom_common.h
+++ b/drivers/remoteproc/qcom_common.h
@@ -24,10 +24,16 @@ struct qcom_rproc_subdev {
 	struct qcom_smd_edge *edge;
 };
 
+struct rproc_notif_info {
+	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);
diff --git a/include/linux/remoteproc/qcom_rproc.h b/include/linux/remoteproc/qcom_rproc.h
index fa8e386..3dc65c0 100644
--- a/include/linux/remoteproc/qcom_rproc.h
+++ b/include/linux/remoteproc/qcom_rproc.h
@@ -5,17 +5,27 @@
 
 #if IS_ENABLED(CONFIG_QCOM_RPROC_COMMON)
 
-int qcom_register_ssr_notifier(struct notifier_block *nb);
-void qcom_unregister_ssr_notifier(struct notifier_block *nb);
+struct rproc_notif_data {
+	const char *name;
+	bool crashed;
+};
+
+void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb);
+int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb);
 
 #else
 
-static inline int qcom_register_ssr_notifier(struct notifier_block *nb)
+static inline void *qcom_register_ssr_notifier(const char *name,
+						struct notifier_block *nb)
 {
-	return 0;
+	return NULL;
 }
 
-static inline void qcom_unregister_ssr_notifier(struct notifier_block *nb) {}
+static inline int qcom_unregister_ssr_notifier(void *notify,
+						struct notifier_block *nb)
+{
+	return 0;
+}
 
 #endif
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v3 2/2] remoteproc: qcom: Add notification types to SSR
  2020-04-28 22:16 [PATCH v3 0/2] Extend SSR notifications framework Rishabh Bhatnagar
  2020-04-28 22:16 ` [PATCH v3 1/2] remoteproc: qcom: Add per subsystem SSR notification Rishabh Bhatnagar
@ 2020-04-28 22:16 ` Rishabh Bhatnagar
  2020-05-19 20:40   ` Bjorn Andersson
  1 sibling, 1 reply; 7+ messages in thread
From: Rishabh Bhatnagar @ 2020-04-28 22:16 UTC (permalink / raw)
  To: linux-remoteproc, bjorn.andersson
  Cc: ohad, linux-kernel, tsoni, psodagud, sidgup, 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 7cd17be..0d91cf3 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -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);
diff --git a/include/linux/remoteproc/qcom_rproc.h b/include/linux/remoteproc/qcom_rproc.h
index 3dc65c0..567c1f9 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 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;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/2] remoteproc: qcom: Add per subsystem SSR notification
  2020-04-28 22:16 ` [PATCH v3 1/2] remoteproc: qcom: Add per subsystem SSR notification Rishabh Bhatnagar
@ 2020-04-29  6:15   ` kbuild test robot
  2020-05-19 20:38   ` Bjorn Andersson
  1 sibling, 0 replies; 7+ messages in thread
From: kbuild test robot @ 2020-04-29  6:15 UTC (permalink / raw)
  To: Rishabh Bhatnagar, linux-remoteproc, bjorn.andersson
  Cc: kbuild-all, ohad, linux-kernel, tsoni, psodagud, sidgup,
	Rishabh Bhatnagar


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

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 <lkp@intel.com>

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 <bjorn.andersson@linaro.org>
:::::: CC: Bjorn Andersson <bjorn.andersson@linaro.org>

---
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: 48802 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/2] remoteproc: qcom: Add per subsystem SSR notification
  2020-04-28 22:16 ` [PATCH v3 1/2] remoteproc: qcom: Add per subsystem SSR notification Rishabh Bhatnagar
  2020-04-29  6:15   ` kbuild test robot
@ 2020-05-19 20:38   ` Bjorn Andersson
  2020-05-26 18:03     ` rishabhb
  1 sibling, 1 reply; 7+ messages in thread
From: Bjorn Andersson @ 2020-05-19 20:38 UTC (permalink / raw)
  To: Rishabh Bhatnagar
  Cc: linux-remoteproc, ohad, linux-kernel, tsoni, psodagud, sidgup

On Tue 28 Apr 15:16 PDT 2020, Rishabh Bhatnagar wrote:

> Currently there is a single notification chain which is called whenever any
> remoteproc shuts down. This leads to all the listeners being notified, and
> is not an optimal design as kernel drivers might only be interested in
> listening to notifications from a particular remoteproc. Create a global
> list of remoteproc notification info data structures. This will hold the
> name and notifier_list information for a particular remoteproc. The API
> to register for notifications will use name argument to retrieve the
> notification info data structure and the notifier block will be added to
> that data structure's notification chain.
> 
> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>

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

> ---
>  drivers/remoteproc/qcom_common.c      | 89 ++++++++++++++++++++++++++++++-----
>  drivers/remoteproc/qcom_common.h      | 10 +++-
>  include/linux/remoteproc/qcom_rproc.h | 20 ++++++--
>  3 files changed, 99 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
> index 60650bc..7cd17be 100644
> --- a/drivers/remoteproc/qcom_common.c
> +++ b/drivers/remoteproc/qcom_common.c
> @@ -15,16 +15,18 @@
>  #include <linux/rpmsg/qcom_glink.h>
>  #include <linux/rpmsg/qcom_smd.h>
>  #include <linux/soc/qcom/mdt_loader.h>
> +#include <linux/remoteproc/qcom_rproc.h>

Please maintain alphabetical sort order.

>  
>  #include "remoteproc_internal.h"
>  #include "qcom_common.h"
>  
> +#define MAX_NAME_LEN	20
> +DEFINE_MUTEX(rproc_notif_lock);

Please rename this qcom_ssr_subsystem_lock

> +
>  #define to_glink_subdev(d) container_of(d, struct qcom_rproc_glink, subdev)
>  #define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, subdev)
>  #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, subdev)
>  
> -static BLOCKING_NOTIFIER_HEAD(ssr_notifiers);

Move the definition of rproc_notif_info, the new rproc_notif_info list
head and move the two lines above here as well.

> -
>  static int glink_subdev_start(struct rproc_subdev *subdev)
>  {
>  	struct qcom_rproc_glink *glink = to_glink_subdev(subdev);
> @@ -174,39 +176,81 @@ void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd)
>  }
>  EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
>  
> +struct rproc_notif_info *find_notif_info(const char *name)

Please make this qcom_ssr_get_subsystem(const char *name)

> +{
> +	struct rproc_notif_info *info;
> +
> +	/* Match in the global rproc_notif_list with name */
> +	list_for_each_entry(info, &rproc_notif_list, list) {
> +		if (!strncmp(info->name, name, strlen(name)))

strncmp(a, b, strlen(b)) is the same thing as strcmp(a, b), unless a is
shorted than b and not NUL terminated.

> +			return info;
> +	}
> +	return NULL;

Both callers of this function will if NULL is returned allocate a new
subsystem object and attach to the list. If you do that here you can
remove the duplication between these.

> +}
> +
>  /**
>   * qcom_register_ssr_notifier() - register SSR notification handler
> + * @name:	pointer to name which will be searched in the global notif_list
>   * @nb:		notifier_block to notify for restart notifications
>   *
> - * Returns 0 on success, negative errno on failure.
> + * Returns pointer to srcu notifier head on success, ERR_PTR on failure.

This shouldn't mention that the opaque pointer is of a type standard to
the kernel. Better just say that it returns a "subsystem cookie".

>   *
> - * This register the @notify function as handler for restart notifications. As
> - * remote processors are stopped this function will be called, with the SSR
> - * name passed as a parameter.
> + * This registers the @nb notifier block as part the notifier chain for a
> + * remoteproc associated with @name. The notifier block's callback
> + * will be invoked when the particular remote processor is stopped.
>   */
> -int qcom_register_ssr_notifier(struct notifier_block *nb)
> +void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb)
>  {
> -	return blocking_notifier_chain_register(&ssr_notifiers, nb);
> +	struct rproc_notif_info *info;
> +
> +	mutex_lock(&rproc_notif_lock);
> +	info = find_notif_info(name);
> +	if (!info) {
> +		info = kzalloc(sizeof(*info), GFP_KERNEL);
> +		if (!info) {
> +			mutex_unlock(&rproc_notif_lock);
> +			return ERR_PTR(-ENOMEM);
> +		}
> +		info->name = kstrndup(name, MAX_NAME_LEN, GFP_KERNEL);

This is going to be a constant in a lot of cases, so please use
kstrdup_const(). Also what's the purpose of limiting the length of this?

> +		srcu_init_notifier_head(&info->notifier_list);
> +
> +		/* Add to global notif list */
> +		INIT_LIST_HEAD(&info->list);
> +		list_add_tail(&info->list, &rproc_notif_list);
> +	}
> +
> +	srcu_notifier_chain_register(&info->notifier_list, nb);
> +	mutex_unlock(&rproc_notif_lock);
> +	return &info->notifier_list;
>  }
>  EXPORT_SYMBOL_GPL(qcom_register_ssr_notifier);
>  
>  /**
>   * qcom_unregister_ssr_notifier() - unregister SSR notification handler
> + * @notify:	pointer to srcu notifier head

      @subsystem: subsystem cookie returned from qcom_register_ssr_notifier

>   * @nb:		notifier_block to unregister
>   */
> -void qcom_unregister_ssr_notifier(struct notifier_block *nb)
> +int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb)
>  {
> -	blocking_notifier_chain_unregister(&ssr_notifiers, nb);
> +	if (!notify)
> +		return -EINVAL;

qcom_register_ssr_notifier() will return a valid cookie or a ERR_PTR()
so if someone passes NULL here they did something wrong during
development...

So it's better to just remove this check and give the developer a nice
callstack directly pointing out their mistake, than forcing them to
chase where this -EINVAL comes from.

> +
> +	return srcu_notifier_chain_unregister(notify, nb);
>  }
>  EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
>  
>  static void ssr_notify_unprepare(struct rproc_subdev *subdev)
>  {
>  	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> +	struct rproc_notif_data data = {
> +		.name = ssr->info->name,
> +		.crashed = false,
> +	};
>  
> -	blocking_notifier_call_chain(&ssr_notifiers, 0, (void *)ssr->name);
> +	srcu_notifier_call_chain(&ssr->info->notifier_list, 0, &data);

Did we conclude on why you change blocking to srcu? Can we do it in a
separate patch?

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

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

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

> diff --git a/drivers/remoteproc/qcom_common.h b/drivers/remoteproc/qcom_common.h
> index 58de71e..0c1d288 100644
> --- a/drivers/remoteproc/qcom_common.h
> +++ b/drivers/remoteproc/qcom_common.h
> @@ -24,10 +24,16 @@ struct qcom_rproc_subdev {
>  	struct qcom_smd_edge *edge;
>  };
>  
> +struct rproc_notif_info {

Please rename this struct qcom_ssr_subsystem

> +	const char *name;
> +	struct srcu_notifier_head notifier_list;
> +	struct list_head list;
> +};
> +static LIST_HEAD(rproc_notif_list);

Please rename this list qcom_ssr_subsystem_list and as stated above move
it into qcom_common.c.


To allow using qcom_ssr_subsystem in the struct below simply forward
declare it here as:

struct qcom_ssr_subsystem;

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

Regards,
Bjorn

>  void qcom_add_glink_subdev(struct rproc *rproc, struct qcom_rproc_glink *glink);
> diff --git a/include/linux/remoteproc/qcom_rproc.h b/include/linux/remoteproc/qcom_rproc.h
> index fa8e386..3dc65c0 100644
> --- a/include/linux/remoteproc/qcom_rproc.h
> +++ b/include/linux/remoteproc/qcom_rproc.h
> @@ -5,17 +5,27 @@
>  
>  #if IS_ENABLED(CONFIG_QCOM_RPROC_COMMON)
>  
> -int qcom_register_ssr_notifier(struct notifier_block *nb);
> -void qcom_unregister_ssr_notifier(struct notifier_block *nb);
> +struct rproc_notif_data {
> +	const char *name;
> +	bool crashed;
> +};
> +
> +void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb);
> +int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb);
>  
>  #else
>  
> -static inline int qcom_register_ssr_notifier(struct notifier_block *nb)
> +static inline void *qcom_register_ssr_notifier(const char *name,
> +						struct notifier_block *nb)
>  {
> -	return 0;
> +	return NULL;
>  }
>  
> -static inline void qcom_unregister_ssr_notifier(struct notifier_block *nb) {}
> +static inline int qcom_unregister_ssr_notifier(void *notify,
> +						struct notifier_block *nb)
> +{
> +	return 0;
> +}
>  
>  #endif
>  
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 2/2] remoteproc: qcom: Add notification types to SSR
  2020-04-28 22:16 ` [PATCH v3 2/2] remoteproc: qcom: Add notification types to SSR Rishabh Bhatnagar
@ 2020-05-19 20:40   ` Bjorn Andersson
  0 siblings, 0 replies; 7+ messages in thread
From: Bjorn Andersson @ 2020-05-19 20:40 UTC (permalink / raw)
  To: Rishabh Bhatnagar
  Cc: linux-remoteproc, ohad, linux-kernel, tsoni, psodagud, sidgup

On Tue 28 Apr 15:16 PDT 2020, 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
> 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 7cd17be..0d91cf3 100644
> --- a/drivers/remoteproc/qcom_common.c
> +++ b/drivers/remoteproc/qcom_common.c
> @@ -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);
> diff --git a/include/linux/remoteproc/qcom_rproc.h b/include/linux/remoteproc/qcom_rproc.h
> index 3dc65c0..567c1f9 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 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,

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

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/2] remoteproc: qcom: Add per subsystem SSR notification
  2020-05-19 20:38   ` Bjorn Andersson
@ 2020-05-26 18:03     ` rishabhb
  0 siblings, 0 replies; 7+ messages in thread
From: rishabhb @ 2020-05-26 18:03 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: linux-remoteproc, ohad, linux-kernel, tsoni, psodagud, sidgup

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-28 22:16 [PATCH v3 0/2] Extend SSR notifications framework Rishabh Bhatnagar
2020-04-28 22:16 ` [PATCH v3 1/2] remoteproc: qcom: Add per subsystem SSR notification Rishabh Bhatnagar
2020-04-29  6:15   ` kbuild test robot
2020-05-19 20:38   ` Bjorn Andersson
2020-05-26 18:03     ` rishabhb
2020-04-28 22:16 ` [PATCH v3 2/2] remoteproc: qcom: Add notification types to SSR Rishabh Bhatnagar
2020-05-19 20:40   ` Bjorn Andersson

Linux-remoteproc Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-remoteproc/0 linux-remoteproc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-remoteproc linux-remoteproc/ https://lore.kernel.org/linux-remoteproc \
		linux-remoteproc@vger.kernel.org
	public-inbox-index linux-remoteproc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-remoteproc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git