Currently there is a single notification chain which is called whenever any
remoteproc shuts down. This leads to all the listeners being notified, and
is not an optimal design as kernel drivers might only be interested in
listening to notifications from a particular remoteproc. Create a global
list of remoteproc notification info data structures. This will hold the
name and notifier_list information for a particular remoteproc. The API
to register for notifications will use name argument to retrieve the
notification info data structure and the notifier block will be added to
that data structure's notification chain.
Signed-off-by: Siddharth Gupta <[email protected]>
Signed-off-by: Rishabh Bhatnagar <[email protected]>
---
drivers/remoteproc/qcom_common.c | 89 ++++++++++++++++++++++++++++++-----
drivers/remoteproc/qcom_common.h | 10 +++-
include/linux/remoteproc/qcom_rproc.h | 20 ++++++--
3 files changed, 99 insertions(+), 20 deletions(-)
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
Hi Rishabh,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.7-rc3 next-20200428]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Rishabh-Bhatnagar/Extend-SSR-notifications-framework/20200429-071958
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 96c9a7802af7d500a582d89a8b864584fe878c1b
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <[email protected]>
All error/warnings (new ones prefixed by >>):
drivers/soc/qcom/glink_ssr.c: In function 'qcom_glink_ssr_probe':
>> drivers/soc/qcom/glink_ssr.c:128:36: error: passing argument 1 of 'qcom_register_ssr_notifier' from incompatible pointer type [-Werror=incompatible-pointer-types]
128 | return qcom_register_ssr_notifier(&ssr->nb);
| ^~~~~~~~
| |
| struct notifier_block *
In file included from drivers/soc/qcom/glink_ssr.c:11:
include/linux/remoteproc/qcom_rproc.h:13:7: note: expected 'const char *' but argument is of type 'struct notifier_block *'
13 | void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/soc/qcom/glink_ssr.c:128:9: error: too few arguments to function 'qcom_register_ssr_notifier'
128 | return qcom_register_ssr_notifier(&ssr->nb);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from drivers/soc/qcom/glink_ssr.c:11:
include/linux/remoteproc/qcom_rproc.h:13:7: note: declared here
13 | void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/soc/qcom/glink_ssr.c: In function 'qcom_glink_ssr_remove':
>> drivers/soc/qcom/glink_ssr.c:135:2: error: too few arguments to function 'qcom_unregister_ssr_notifier'
135 | qcom_unregister_ssr_notifier(&ssr->nb);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from drivers/soc/qcom/glink_ssr.c:11:
include/linux/remoteproc/qcom_rproc.h:14:5: note: declared here
14 | int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/soc/qcom/glink_ssr.c: In function 'qcom_glink_ssr_probe':
>> drivers/soc/qcom/glink_ssr.c:129:1: warning: control reaches end of non-void function [-Wreturn-type]
129 | }
| ^
cc1: some warnings being treated as errors
vim +/qcom_register_ssr_notifier +128 drivers/soc/qcom/glink_ssr.c
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 111
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 112 static int qcom_glink_ssr_probe(struct rpmsg_device *rpdev)
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 113 {
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 114 struct glink_ssr *ssr;
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 115
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 116 ssr = devm_kzalloc(&rpdev->dev, sizeof(*ssr), GFP_KERNEL);
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 117 if (!ssr)
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 118 return -ENOMEM;
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 119
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 120 init_completion(&ssr->completion);
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 121
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 122 ssr->dev = &rpdev->dev;
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 123 ssr->ept = rpdev->ept;
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 124 ssr->nb.notifier_call = qcom_glink_ssr_notify;
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 125
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 126 dev_set_drvdata(&rpdev->dev, ssr);
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 127
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 @128 return qcom_register_ssr_notifier(&ssr->nb);
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 @129 }
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 130
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 131 static void qcom_glink_ssr_remove(struct rpmsg_device *rpdev)
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 132 {
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 133 struct glink_ssr *ssr = dev_get_drvdata(&rpdev->dev);
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 134
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 @135 qcom_unregister_ssr_notifier(&ssr->nb);
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 136 }
c4d77d5fcd8bae Bjorn Andersson 2017-07-24 137
:::::: The code at line 128 was first introduced by commit
:::::: c4d77d5fcd8baefb358ae921b940e9bbd09a751e soc: qcom: GLINK SSR notifier
:::::: TO: Bjorn Andersson <[email protected]>
:::::: CC: Bjorn Andersson <[email protected]>
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
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 <[email protected]>
> Signed-off-by: Rishabh Bhatnagar <[email protected]>
Thanks Rishabh, design wise I think this looks good now, just some code
style things below.
> ---
> 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
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 <[email protected]>
>> Signed-off-by: Rishabh Bhatnagar <[email protected]>
>
> Thanks Rishabh, design wise I think this looks good now, just some code
> style things below.
>
>> ---
>> 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