Re: [PATCH v5 1/2] remoteproc: qcom: Add per subsystem SSR notification
On Tue 23 Jun 18:41 PDT 2020, risha...@codeaurora.org wrote: > On 2020-06-23 14:45, Alex Elder wrote: > > On 6/22/20 8:04 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. > > > > > > Signed-off-by: Siddharth Gupta > > > Signed-off-by: Rishabh Bhatnagar > > > > Sorry, a few more comments, but I think your next one will > > likely be fine. > > > > General: > > - SSR subsystems are added but never removed. Note that > > "qcom_common.o" can be built as a module, and if that > > module were ever removed, memory allocated for these > > subsystems would be leaked. > Hi Alex, > Thank you for reviewing this patchset quickly. This point was > brought up by Bjorn and it was decided that I will push another patch on > top in which I'll do the cleanup during module exit. > > - Will a remoteproc subdev (and in particular, an SSR subdev) > > ever be removed? What happens to entities that have > > registered for SSR notifications in that case? > In practice it should never be removed. If it is clients will > never get notification about subsystem shutdown/powerup. Given that clients make direct function calls into qcom_common.ko, qcom_common.ko would not be possible to rmmod until all clients has been rmmod'ed. As such there shouldn't be any remaining listeners, or subdevices, when this happens. Regards, Bjorn
Re: [PATCH v5 1/2] remoteproc: qcom: Add per subsystem SSR notification
On 2020-06-23 14:45, Alex Elder wrote:
On 6/22/20 8:04 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.
Signed-off-by: Siddharth Gupta
Signed-off-by: Rishabh Bhatnagar
Sorry, a few more comments, but I think your next one will
likely be fine.
General:
- SSR subsystems are added but never removed. Note that
"qcom_common.o" can be built as a module, and if that
module were ever removed, memory allocated for these
subsystems would be leaked.
Hi Alex,
Thank you for reviewing this patchset quickly. This point was
brought up by Bjorn and it was decided that I will push another patch on
top in which I'll do the cleanup during module exit.
- Will a remoteproc subdev (and in particular, an SSR subdev)
ever be removed? What happens to entities that have
registered for SSR notifications in that case?
In practice it should never be removed. If it is clients will
never get notification about subsystem shutdown/powerup.
(Maybe these are issues that won't/can't occur in practice?)
---
drivers/remoteproc/qcom_common.c | 86
+--
drivers/remoteproc/qcom_common.h | 5 +-
include/linux/remoteproc/qcom_rproc.h | 20 ++--
3 files changed, 91 insertions(+), 20 deletions(-)
diff --git a/drivers/remoteproc/qcom_common.c
b/drivers/remoteproc/qcom_common.c
index 9028cea..658f2ca 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -23,7 +24,14 @@
#define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev,
subdev)
#define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr,
subdev)
-static BLOCKING_NOTIFIER_HEAD(ssr_notifiers);
+struct qcom_ssr_subsystem {
+ const char *name;
+ struct srcu_notifier_head notifier_list;
+ struct list_head list;
+};
+
+static LIST_HEAD(qcom_ssr_subsystem_list);
+static DEFINE_MUTEX(qcom_ssr_subsys_lock);
static int glink_subdev_start(struct rproc_subdev *subdev)
{
@@ -189,37 +197,80 @@ void qcom_remove_smd_subdev(struct rproc *rproc,
struct qcom_rproc_subdev *smd)
}
EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
+static struct qcom_ssr_subsystem *qcom_ssr_get_subsys(const char
*name)
+{
+ struct qcom_ssr_subsystem *info;
+
+ mutex_lock(_ssr_subsys_lock);
+ /* Match in the global qcom_ssr_subsystem_list with name */
+ list_for_each_entry(info, _ssr_subsystem_list, list)
+ if (!strcmp(info->name, name))
+ return info;
You need to unlock the mutex here. You would probably
be better off structuring this with a common exit path
below, for example:
if (!strcmp(info->name, name))
goto out_mutex_unlock;
. . .
out_mutex_unlock:
mutex_unlock(_ssr_subsys_lock);
return info;
}
+
+ info = kzalloc(sizeof(*info), GFP_KERNEL);
+ if (!info)
+ return ERR_PTR(-ENOMEM);
Here too. Perhaps this:
if (!info) {
info = ERR_PTR(-ENOMEM);
goto out_mutex_unlock;
}
+ info->name = kstrdup_const(name, GFP_KERNEL);
+ srcu_init_notifier_head(>notifier_list);
+
+ /* Add to global notification list */
+ list_add_tail(>list, _ssr_subsystem_list);
+ mutex_unlock(_ssr_subsys_lock);
+
+ return info;
+}
+
/**
* qcom_register_ssr_notifier() - register SSR notification handler
- * @nb:notifier_block to notify for restart notifications
+ * @name: Subsystem's SSR name
+ * @nb:notifier_block to be invoked upon subsystem's state
change
*
- * Returns 0 on success, negative errno on failure.
+ * This registers the @nb notifier block as part the notifier chain
for a
+ * remoteproc associated with @name. The notifier block's callback
+ * will be invoked when the remote processor's SSR events occur
+ * (pre/post startup and pre/post shutdown).
*
- * This register the @notify function as handler for restart
notifications. As
- * remote processors are stopped this function will be called, with
the SSR
- * name passed as a parameter.
+ * Return: a subsystem cookie on success,
Re: [PATCH v5 1/2] remoteproc: qcom: Add per subsystem SSR notification
On 6/22/20 8:04 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.
Signed-off-by: Siddharth Gupta
Signed-off-by: Rishabh Bhatnagar
Sorry, a few more comments, but I think your next one will
likely be fine.
General:
- SSR subsystems are added but never removed. Note that
"qcom_common.o" can be built as a module, and if that
module were ever removed, memory allocated for these
subsystems would be leaked.
- Will a remoteproc subdev (and in particular, an SSR subdev)
ever be removed? What happens to entities that have
registered for SSR notifications in that case?
(Maybe these are issues that won't/can't occur in practice?)
---
drivers/remoteproc/qcom_common.c | 86 +--
drivers/remoteproc/qcom_common.h | 5 +-
include/linux/remoteproc/qcom_rproc.h | 20 ++--
3 files changed, 91 insertions(+), 20 deletions(-)
diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 9028cea..658f2ca 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -23,7 +24,14 @@
#define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, subdev)
#define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, subdev)
-static BLOCKING_NOTIFIER_HEAD(ssr_notifiers);
+struct qcom_ssr_subsystem {
+ const char *name;
+ struct srcu_notifier_head notifier_list;
+ struct list_head list;
+};
+
+static LIST_HEAD(qcom_ssr_subsystem_list);
+static DEFINE_MUTEX(qcom_ssr_subsys_lock);
static int glink_subdev_start(struct rproc_subdev *subdev)
{
@@ -189,37 +197,80 @@ void qcom_remove_smd_subdev(struct rproc *rproc, struct
qcom_rproc_subdev *smd)
}
EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
+static struct qcom_ssr_subsystem *qcom_ssr_get_subsys(const char *name)
+{
+ struct qcom_ssr_subsystem *info;
+
+ mutex_lock(_ssr_subsys_lock);
+ /* Match in the global qcom_ssr_subsystem_list with name */
+ list_for_each_entry(info, _ssr_subsystem_list, list)
+ if (!strcmp(info->name, name))
+ return info;
You need to unlock the mutex here. You would probably
be better off structuring this with a common exit path
below, for example:
if (!strcmp(info->name, name))
goto out_mutex_unlock;
. . .
out_mutex_unlock:
mutex_unlock(_ssr_subsys_lock);
return info;
}
+
+ info = kzalloc(sizeof(*info), GFP_KERNEL);
+ if (!info)
+ return ERR_PTR(-ENOMEM);
Here too. Perhaps this:
if (!info) {
info = ERR_PTR(-ENOMEM);
goto out_mutex_unlock;
}
+ info->name = kstrdup_const(name, GFP_KERNEL);
+ srcu_init_notifier_head(>notifier_list);
+
+ /* Add to global notification list */
+ list_add_tail(>list, _ssr_subsystem_list);
+ mutex_unlock(_ssr_subsys_lock);
+
+ return info;
+}
+
/**
* qcom_register_ssr_notifier() - register SSR notification handler
- * @nb:notifier_block to notify for restart notifications
+ * @name: Subsystem's SSR name
+ * @nb:notifier_block to be invoked upon subsystem's state
change
*
- * Returns 0 on success, negative errno on failure.
+ * This registers the @nb notifier block as part the notifier chain for a
+ * remoteproc associated with @name. The notifier block's callback
+ * will be invoked when the remote processor's SSR events occur
+ * (pre/post startup and pre/post shutdown).
*
- * This register the @notify function as handler for restart notifications. As
- * remote processors are stopped this function will be called, with the SSR
- * name passed as a parameter.
+ * Return: a subsystem cookie on success, ERR_PTR on failure.
*/
-int qcom_register_ssr_notifier(struct notifier_block *nb)
+void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb)
{
- return blocking_notifier_chain_register(_notifiers, nb);
+ struct qcom_ssr_subsystem *info;
+
+ info = qcom_ssr_get_subsys(name);
+ if (IS_ERR(info))
+ return info;
+
+
[PATCH v5 1/2] remoteproc: qcom: Add per subsystem SSR notification
Currently there is a single notification chain which is called whenever any
remoteproc shuts down. This leads to all the listeners being notified, and
is not an optimal design as kernel drivers might only be interested in
listening to notifications from a particular remoteproc. Create a global
list of remoteproc notification info data structures. This will hold the
name and notifier_list information for a particular remoteproc. The API
to register for notifications will use name argument to retrieve the
notification info data structure and the notifier block will be added to
that data structure's notification chain. Also move from blocking notifier
to srcu notifer based implementation to support dynamic notifier head
creation.
Signed-off-by: Siddharth Gupta
Signed-off-by: Rishabh Bhatnagar
---
drivers/remoteproc/qcom_common.c | 86 +--
drivers/remoteproc/qcom_common.h | 5 +-
include/linux/remoteproc/qcom_rproc.h | 20 ++--
3 files changed, 91 insertions(+), 20 deletions(-)
diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 9028cea..658f2ca 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -23,7 +24,14 @@
#define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, subdev)
#define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, subdev)
-static BLOCKING_NOTIFIER_HEAD(ssr_notifiers);
+struct qcom_ssr_subsystem {
+ const char *name;
+ struct srcu_notifier_head notifier_list;
+ struct list_head list;
+};
+
+static LIST_HEAD(qcom_ssr_subsystem_list);
+static DEFINE_MUTEX(qcom_ssr_subsys_lock);
static int glink_subdev_start(struct rproc_subdev *subdev)
{
@@ -189,37 +197,80 @@ void qcom_remove_smd_subdev(struct rproc *rproc, struct
qcom_rproc_subdev *smd)
}
EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
+static struct qcom_ssr_subsystem *qcom_ssr_get_subsys(const char *name)
+{
+ struct qcom_ssr_subsystem *info;
+
+ mutex_lock(_ssr_subsys_lock);
+ /* Match in the global qcom_ssr_subsystem_list with name */
+ list_for_each_entry(info, _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(>notifier_list);
+
+ /* Add to global notification list */
+ list_add_tail(>list, _ssr_subsystem_list);
+ mutex_unlock(_ssr_subsys_lock);
+
+ return info;
+}
+
/**
* qcom_register_ssr_notifier() - register SSR notification handler
- * @nb:notifier_block to notify for restart notifications
+ * @name: Subsystem's SSR name
+ * @nb:notifier_block to be invoked upon subsystem's state
change
*
- * Returns 0 on success, negative errno on failure.
+ * This registers the @nb notifier block as part the notifier chain for a
+ * remoteproc associated with @name. The notifier block's callback
+ * will be invoked when the remote processor's SSR events occur
+ * (pre/post startup and pre/post shutdown).
*
- * This register the @notify function as handler for restart notifications. As
- * remote processors are stopped this function will be called, with the SSR
- * name passed as a parameter.
+ * Return: a subsystem cookie on success, ERR_PTR on failure.
*/
-int qcom_register_ssr_notifier(struct notifier_block *nb)
+void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb)
{
- return blocking_notifier_chain_register(_notifiers, nb);
+ struct qcom_ssr_subsystem *info;
+
+ info = qcom_ssr_get_subsys(name);
+ if (IS_ERR(info))
+ return info;
+
+ srcu_notifier_chain_register(>notifier_list, nb);
+
+ return >notifier_list;
}
EXPORT_SYMBOL_GPL(qcom_register_ssr_notifier);
/**
* qcom_unregister_ssr_notifier() - unregister SSR notification handler
+ * @notify:subsystem coookie returned from qcom_register_ssr_notifier
* @nb:notifier_block to unregister
+ *
+ * This function will unregister the notifier from the particular notifier
+ * chain.
+ *
+ * Return: 0 on success, %ENOENT otherwise.
*/
-void qcom_unregister_ssr_notifier(struct notifier_block *nb)
+int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb)
{
- blocking_notifier_chain_unregister(_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,
+ };
