Extend SSR notifications framework
Subsystems (2)
v4 cover Extend SSR notifications framework
This set of patches gives kernel client drivers the ability to register for a particular remoteproc's SSR notifications. Also the notifications are extended to before/after-powerup/shutdown stages.
It also fixes the bug where clients need to register for notifications again if the platform driver is removed. This is done by creating a global list of per-remoteproc notification info data structures which remain static. An API is exported to register for a remoteproc's SSR notifications and uses remoteproc's ssr_name and notifier block as arguments.
Changelog:
v4 -> v3:
- Fix naming convention
v3 -> v2:
- Create a global list of per remoteproc notification data structure
- Pass ssr_name and crashed information as part of notification data
- Move notification type enum to qcom_rproc.h from remoteproc.h
v2 -> v1:
- Fix commit text
Rishabh Bhatnagar (1):
remoteproc: qcom: Add per subsystem SSR notification
Siddharth Gupta (1):
remoteproc: qcom: Add notification types to SSR
drivers/remoteproc/qcom_common.c | 126 ++++++++++++++++++++++++++++++---- drivers/remoteproc/qcom_common.h | 5 +- include/linux/remoteproc/qcom_rproc.h | 34 +++++++-- 3 files changed, 146 insertions(+), 19 deletions(-)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v4 1/2 remoteproc: qcom: Add per subsystem SSR notification
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.
--- drivers/remoteproc/qcom_common.c | 84 ++++++++++++++++++++++++++++++----- drivers/remoteproc/qcom_common.h | 5 ++- include/linux/remoteproc/qcom_rproc.h | 20 ++++++--- 3 files changed, 90 insertions(+), 19 deletions(-)
drivers/remoteproc/qcom_common.c +72 -12
@@ -12,6 +12,7 @@#include <linux/module.h> #include <linux/notifier.h> #include <linux/remoteproc.h> +#include <linux/remoteproc/qcom_rproc.h> #include <linux/rpmsg/qcom_glink.h> #include <linux/rpmsg/qcom_smd.h> #include <linux/soc/qcom/mdt_loader.h>
@@ -23,7 +24,14 @@#define to_smd_subdev (d) container_of(d, struct qcom_rproc_subdev, subdev)#define to_ssr_subdev (d) container_of(d, struct qcom_rproc_ssr, subdev) -static BLOCKING_NOTIFIER_HEAD (ssr_notifiers); +struct qcom_ssr_subsystem { +const char *name; +struct srcu_notifier_head notifier_list; +struct list_head list; +}; + +static LIST_HEAD (qcom_ssr_subsystem_list); +DEFINE_MUTEX (qcom_ssr_subsys_lock);static int glink_subdev_start (struct rproc_subdev *subdev) {
@@ -189,39 +197,79 @@ void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) }EXPORT_SYMBOL_GPL (qcom_remove_smd_subdev); +struct qcom_ssr_subsystem *qcom_ssr_get_subsys (const char *name) +{ +struct qcom_ssr_subsystem *info; + +/* Match in the global qcom_ssr_subsystem_list with name */ +list_for_each_entry (info, &qcom_ssr_subsystem_list, list) { +if (!strcmp (info->name, name)) +return info; + } + info =kzalloc (sizeof (*info),GFP_KERNEL ); +if (!info) +return ERR_PTR (-ENOMEM ); + info->name =kstrdup_const (name,GFP_KERNEL ); +srcu_init_notifier_head (&info->notifier_list); + +/* Add to global notif list */ +INIT_LIST_HEAD (&info->list); +list_add_tail (&info->list, &qcom_ssr_subsystem_list); + +return info; +} +/** * qcom_register_ssr_notifier() - register SSR notification handler +* @name: name that will be searched in global ssr subsystem list * @nb: notifier_block to notify for restart notifications * -* Returns 0 on success, negative errno on failure. +* Returns a subsystem cookie on success, ERR_PTR on failure. * -* This register the @notify function as handler for restart notifications. As -* remote processors are stopped this function will be called, with the SSR -* name passed as a parameter. +* This registers the @nb notifier block as part the notifier chain for a +* remoteproc associated with @name. The notifier block's callback +* will be invoked when the particular remote processor is stopped. */ -int qcom_register_ssr_notifier (struct notifier_block *nb) +void *qcom_register_ssr_notifier (const char *name,struct notifier_block *nb) { -return blocking_notifier_chain_register (&ssr_notifiers, nb); +struct qcom_ssr_subsystem *info; + +mutex_lock (&qcom_ssr_subsys_lock); + info =qcom_ssr_get_subsys (name); +if (IS_ERR (info)) { +mutex_unlock (&qcom_ssr_subsys_lock); +return info; + } + +srcu_notifier_chain_register (&info->notifier_list, nb); +mutex_unlock (&qcom_ssr_subsys_lock); +return &info->notifier_list; }EXPORT_SYMBOL_GPL (qcom_register_ssr_notifier);/** * qcom_unregister_ssr_notifier() - unregister SSR notification handler +* @notify: subsystem coookie returned from qcom_register_ssr_notifier * @nb: notifier_block to unregister */ -void qcom_unregister_ssr_notifier (struct notifier_block *nb) +int qcom_unregister_ssr_notifier (void *notify,struct notifier_block *nb) { -blocking_notifier_chain_unregister (&ssr_notifiers, nb); +return srcu_notifier_chain_unregister (notify, nb); }EXPORT_SYMBOL_GPL (qcom_unregister_ssr_notifier);static void ssr_notify_unprepare (struct rproc_subdev *subdev) {struct qcom_rproc_ssr *ssr =to_ssr_subdev (subdev); +struct qcom_ssr_notif_data data = { + .name = ssr->info->name, + .crashed = false, + }; -blocking_notifier_call_chain (&ssr_notifiers,0 , (void *)ssr->name); +srcu_notifier_call_chain (&ssr->info->notifier_list,0 , &data); } +/** * qcom_add_ssr_subdev() - register subdevice as restart notification source * @rproc: rproc handle
@@ -229,12 +277,23 @@ static void ssr_notify_unprepare(struct rproc_subdev *subdev)* @ssr_name: identifier to use for notifications originating from @rproc * * As the @ssr is registered with the @rproc SSR events will be sent to all -* registered listeners in the system as the remoteproc is shut down. +* registered listeners for the particular remoteproc when it is shutdown. */ void qcom_add_ssr_subdev (struct rproc *rproc,struct qcom_rproc_ssr *ssr,const char *ssr_name) { - ssr->name = ssr_name; +struct qcom_ssr_subsystem *info; + +mutex_lock (&qcom_ssr_subsys_lock); + info =qcom_ssr_get_subsys (ssr_name); +if (IS_ERR (info)) { +dev_err (&rproc->dev,"Failed to add ssr subdevice\n" ); +mutex_unlock (&qcom_ssr_subsys_lock); +return ; + } + +mutex_unlock (&qcom_ssr_subsys_lock); + ssr->info = info; ssr->subdev.unprepare = ssr_notify_unprepare;rproc_add_subdev (rproc, &ssr->subdev);
@@ -249,6 +308,7 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,void qcom_remove_ssr_subdev (struct rproc *rproc,struct qcom_rproc_ssr *ssr) {rproc_remove_subdev (rproc, &ssr->subdev); + ssr->info =NULL ; }EXPORT_SYMBOL_GPL (qcom_remove_ssr_subdev);
drivers/remoteproc/qcom_common.h +3 -2
@@ -26,10 +26,11 @@ struct qcom_rproc_subdev {struct qcom_smd_edge *edge; }; +struct qcom_ssr_subsystem ; +struct qcom_rproc_ssr {struct rproc_subdev subdev; - -const char *name; +struct qcom_ssr_subsystem *info; }; void qcom_add_glink_subdev(struct rproc *rproc, struct qcom_rproc_glink *glink,
include/linux/remoteproc/qcom_rproc.h +15 -5
@@ -5,17 +5,27 @@#if IS_ENABLED (CONFIG_QCOM_RPROC_COMMON ) -int qcom_register_ssr_notifier (struct notifier_block *nb); -void qcom_unregister_ssr_notifier (struct notifier_block *nb); +struct qcom_ssr_notif_data { +const char *name; +bool crashed; +}; + +void *qcom_register_ssr_notifier (const char *name,struct notifier_block *nb); +int qcom_unregister_ssr_notifier (void *notify,struct notifier_block *nb);#else -static inline int qcom_register_ssr_notifier (struct notifier_block *nb) +static inline void *qcom_register_ssr_notifier (const char *name, +struct notifier_block *nb) { -return 0 ; +return NULL ; } -static inline void qcom_unregister_ssr_notifier (struct notifier_block *nb) {} +static inline int qcom_unregister_ssr_notifier (void *notify, +struct notifier_block *nb) +{ +return 0 ; +}#endif
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
kbuild test robot
build botRe: [PATCH v4 1/2] remoteproc: qcom: Add per subsystem SSR notification
Thank you for the patch! Perhaps something to improve:
Hi Rishabh,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on next-20200526]
[also build test WARNING on v5.7-rc7]
[cannot apply to linus/master linux/master v5.7-rc7 v5.7-rc6 v5.7-rc5] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Rishabh-Bhatnagar/Extend-SSR-notifications-framework/20200528-115948 base: b0523c7b1c9d0edcd6c0fe6d2cb558a9ad5c60a8 config: arm64-allyesconfig (attached as .config) compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
All warnings (new ones prefixed by >>, old ones prefixed by <<):
>> drivers/remoteproc/qcom_common.c:200:28: warning: no previous prototype for 'qcom_ssr_get_subsys' [-Wmissing-prototypes]
200 | struct qcom_ssr_subsystem *qcom_ssr_get_subsys(const char *name)
| ^~~~~~~~~~~~~~~~~~~
vim +/qcom_ssr_get_subsys +200 drivers/remoteproc/qcom_common.c
199
> 200 struct qcom_ssr_subsystem *qcom_ssr_get_subsys(const char *name)
201 {
202 struct qcom_ssr_subsystem *info;
203
204 /* Match in the global qcom_ssr_subsystem_list with name */
205 list_for_each_entry(info, &qcom_ssr_subsystem_list, list) {
206 if (!strcmp(info->name, name))
207 return info;
208 }
209 info = kzalloc(sizeof(*info), GFP_KERNEL);
210 if (!info)
211 return ERR_PTR(-ENOMEM);
212 info->name = kstrdup_const(name, GFP_KERNEL);
213 srcu_init_notifier_head(&info->notifier_list);
214
215 /* Add to global notif list */
216 INIT_LIST_HEAD(&info->list);
217 list_add_tail(&info->list, &qcom_ssr_subsystem_list);
218
219 return info;
220 }
221
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
Re: [PATCH v4 1/2] remoteproc: qcom: Add per subsystem SSR notification
In reply to Rishabh BhatnagarQuoted 11 lines · Rishabh Bhatnagar
Currently there is a single notification chain which is called whenever any remoteproc shuts down. This leads to all the listeners being notified, and is not an optimal design as kernel drivers might only be interested in listening to notifications from a particular remoteproc. Create a global list of remoteproc notification info data structures. This will hold the name and
notifier_listinformation for a particular remoteproc. The API to register for notifications will use name argument to retrieve the notification info data structure and the notifier block will be added to that data structure's notification chain. Also move from blocking notifier to srcu notifer based implementation to support dynamic notifier head creation.
I'm looking at these patches now, without having reviewed the previous versions. Forgive me if I contradict or duplicate previous feedback.
I have a number of suggestions, below.
-Alex
Quoted 23 lines
Signed-off-by Siddharth Gupta <[email protected]>Signed-off-by Rishabh Bhatnagar <[email protected]>--- drivers/remoteproc/qcom_common.c | 84 ++++++++++++++++++++++++++++++----- drivers/remoteproc/qcom_common.h | 5 ++- include/linux/remoteproc/qcom_rproc.h | 20 ++++++--- 3 files changed, 90 insertions(+), 19 deletions(-)drivers/remoteproc/qcom_common.c +1
-0@@ -12,6 +12,7 @@#include <linux/module.h> #include <linux/notifier.h> #include <linux/remoteproc.h> +#include <linux/remoteproc/qcom_rproc.h> #include <linux/rpmsg/qcom_glink.h> #include <linux/rpmsg/qcom_smd.h> #include <linux/soc/qcom/mdt_loader.h> @@ -23,7 +24,14 @@#define to_smd_subdev (d) container_of(d, struct qcom_rproc_subdev, subdev)#define to_ssr_subdev (d) container_of(d, struct qcom_rproc_ssr, subdev)
drivers/remoteproc/qcom_common.c line 34
Quoted 10 lines
drivers/remoteproc/qcom_common.c +8
-1-static BLOCKING_NOTIFIER_HEAD (ssr_notifiers); +struct qcom_ssr_subsystem { +const char *name; +struct srcu_notifier_head notifier_list; +struct list_head list; +}; + +static LIST_HEAD (qcom_ssr_subsystem_list); +DEFINE_MUTEX (qcom_ssr_subsys_lock);
There is no need for this mutex to be global.
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 200
Quoted 7 lines
drivers/remoteproc/qcom_common.c +1
-0static int glink_subdev_start (struct rproc_subdev *subdev) {@@ -189,39 +197,79 @@ void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) }EXPORT_SYMBOL_GPL (qcom_remove_smd_subdev); +struct qcom_ssr_subsystem *qcom_ssr_get_subsys (const char *name)
This function should be made private (static).
I think the mutex should be taken in this function rather than the caller (more on this below). But if you leave it this way, please mention something in a comment that indicates the caller must hold the qcom_ssr_subsys_lock mutex.
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 207
Quoted 7 lines
drivers/remoteproc/qcom_common.c +7
-0+{ +struct qcom_ssr_subsystem *info; + +/* Match in the global qcom_ssr_subsystem_list with name */ +list_for_each_entry (info, &qcom_ssr_subsystem_list, list) { +if (!strcmp (info->name, name)) +return info;
This probably isn't strictly necessary, because you are returning a void pointer, but you could do this here:
return ERR_CAST(info);
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 208
Quoted 1 line
drivers/remoteproc/qcom_common.c +1
-0+ }
This is purely a style thing, but the curly braces around the loop body aren't necessary.
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 215
Quoted 7 lines
drivers/remoteproc/qcom_common.c +7
-0+ info =kzalloc (sizeof (*info),GFP_KERNEL ); +if (!info) +return ERR_PTR (-ENOMEM ); + info->name =kstrdup_const (name,GFP_KERNEL ); +srcu_init_notifier_head (&info->notifier_list); + +/* Add to global notif list */
s/notif/notification/
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 216
Quoted 1 line
drivers/remoteproc/qcom_common.c +1
-0+INIT_LIST_HEAD (&info->list);
No need to initialize the list element when adding it to a list. Both uts fields will be overwritten anyway.
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 224
Quoted 8 lines
drivers/remoteproc/qcom_common.c +6
-0+list_add_tail (&info->list, &qcom_ssr_subsystem_list); + +return info; +} +/** * qcom_register_ssr_notifier() - register SSR notification handler +* @name: name that will be searched in global ssr subsystem list
Maybe just "SSR subsystem name".
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 225
Quoted 1 line
drivers/remoteproc/qcom_common.c +0
-0* @nb: notifier_block to notify for restart notifications
Drop or modify "restart" in the comment line above.
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 227
Quoted 3 lines
drivers/remoteproc/qcom_common.c +1
-1* -* Returns 0 on success, negative errno on failure. +* Returns a subsystem cookie on success, ERR_PTR on failure.
Maybe make the above a @Return: comment.
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 231
Quoted 7 lines
drivers/remoteproc/qcom_common.c +3
-3* -* This register the @notify function as handler for restart notifications. As -* remote processors are stopped this function will be called, with the SSR -* name passed as a parameter. +* This registers the @nb notifier block as part the notifier chain for a +* remoteproc associated with @name. The notifier block's callback +* will be invoked when the particular remote processor is stopped.
It's not just for stopping, right? Maybe something more like:
Register to receive notification callbacks when remoteproc SSR events occur (pre- and post-startup and pre- and post-shutdown).
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 237
Quoted 8 lines
drivers/remoteproc/qcom_common.c +4
-2*/ -int qcom_register_ssr_notifier (struct notifier_block *nb) +void *qcom_register_ssr_notifier (const char *name,struct notifier_block *nb) { -return blocking_notifier_chain_register (&ssr_notifiers, nb); +struct qcom_ssr_subsystem *info; + +mutex_lock (&qcom_ssr_subsys_lock);
Can you explain why the mutex is taken here (and in qcom_add_ssr_subdev()), rather than having the mutex logic be isolated in qcom_ssr_get_subsys()?
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 242
Quoted 5 lines
drivers/remoteproc/qcom_common.c +5
-0+ info =qcom_ssr_get_subsys (name); +if (IS_ERR (info)) { +mutex_unlock (&qcom_ssr_subsys_lock); +return info; + }
I don't think there's any need to have the next function call be protected by the mutex, but maybe I'm mistaken.
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 253
Quoted 10 lines
drivers/remoteproc/qcom_common.c +4
-0+srcu_notifier_chain_register (&info->notifier_list, nb); +mutex_unlock (&qcom_ssr_subsys_lock); +return &info->notifier_list; }EXPORT_SYMBOL_GPL (qcom_register_ssr_notifier);/** * qcom_unregister_ssr_notifier() - unregister SSR notification handler +* @notify: subsystem coookie returned from qcom_register_ssr_notifier * @nb: notifier_block to unregister
Add a @Return comment (0 on success, %ENOENT otherwise).
Quoted 30 lines
drivers/remoteproc/qcom_common.c +8
-4*/ -void qcom_unregister_ssr_notifier (struct notifier_block *nb) +int qcom_unregister_ssr_notifier (void *notify,struct notifier_block *nb) { -blocking_notifier_chain_unregister (&ssr_notifiers, nb); +return srcu_notifier_chain_unregister (notify, nb); }EXPORT_SYMBOL_GPL (qcom_unregister_ssr_notifier);static void ssr_notify_unprepare (struct rproc_subdev *subdev) {struct qcom_rproc_ssr *ssr =to_ssr_subdev (subdev); +struct qcom_ssr_notif_data data = { + .name = ssr->info->name, + .crashed = false, + }; -blocking_notifier_call_chain (&ssr_notifiers,0 , (void *)ssr->name); +srcu_notifier_call_chain (&ssr->info->notifier_list,0 , &data); } +/** * qcom_add_ssr_subdev() - register subdevice as restart notification source * @rproc: rproc handle @@ -229,12 +277,23 @@ static void ssr_notify_unprepare(struct rproc_subdev *subdev)* @ssr_name: identifier to use for notifications originating from @rproc * * As the @ssr is registered with the @rproc SSR events will be sent to all -* registered listeners in the system as the remoteproc is shut down.
drivers/remoteproc/qcom_common.c static void ssr_notify_unprepare(struct rproc_subdev *subdev) · line 288
Quoted 10 lines
drivers/remoteproc/qcom_common.c +5
-1+* registered listeners for the particular remoteproc when it is shutdown. */ void qcom_add_ssr_subdev (struct rproc *rproc,struct qcom_rproc_ssr *ssr,const char *ssr_name) { - ssr->name = ssr_name; +struct qcom_ssr_subsystem *info; + +mutex_lock (&qcom_ssr_subsys_lock); + info =qcom_ssr_get_subsys (ssr_name);
If there already exists an SSR subsystem having the given name, its info structure is returned here. Is that OK? In practice, I don't expect this to be a problem, but it would be better to return an error if
Quoted 42 lines
drivers/remoteproc/qcom_common.c +9
-0+if (IS_ERR (info)) { +dev_err (&rproc->dev,"Failed to add ssr subdevice\n" ); +mutex_unlock (&qcom_ssr_subsys_lock); +return ; + } + +mutex_unlock (&qcom_ssr_subsys_lock); + ssr->info = info; ssr->subdev.unprepare = ssr_notify_unprepare;rproc_add_subdev (rproc, &ssr->subdev);@@ -249,6 +308,7 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,void qcom_remove_ssr_subdev (struct rproc *rproc,struct qcom_rproc_ssr *ssr) {rproc_remove_subdev (rproc, &ssr->subdev); + ssr->info =NULL ; }EXPORT_SYMBOL_GPL (qcom_remove_ssr_subdev);drivers/remoteproc/qcom_common.h +3
-2@@ -26,10 +26,11 @@ struct qcom_rproc_subdev {struct qcom_smd_edge *edge; }; +struct qcom_ssr_subsystem ; +struct qcom_rproc_ssr {struct rproc_subdev subdev; - -const char *name; +struct qcom_ssr_subsystem *info; }; void qcom_add_glink_subdev(struct rproc *rproc, struct qcom_rproc_glink *glink,include/linux/remoteproc/qcom_rproc.h +0
-0
include/linux/remoteproc/qcom_rproc.h line 10
Quoted 8 lines
include/linux/remoteproc/qcom_rproc.h +3
-2@@ -5,17 +5,27 @@ #if IS_ENABLED(CONFIG_QCOM_RPROC_COMMON) -int qcom_register_ssr_notifier (struct notifier_block *nb); -void qcom_unregister_ssr_notifier (struct notifier_block *nb); +struct qcom_ssr_notif_data { + const char *name; + bool crashed;
Is the crashed field strictly necessary? Could we instead have a QCOM_SSR_CRASHED event (in place of QCOM_SSR_BEFORE_SHUTDOWN)? I don't know, it's possible doing it the way you do ultimately simplifies the logic... So I'm asking, but not suggesting.
Quoted 25 lines
include/linux/remoteproc/qcom_rproc.h +12
-3+}; + +void *qcom_register_ssr_notifier (const char *name,struct notifier_block *nb); +int qcom_unregister_ssr_notifier (void *notify,struct notifier_block *nb);#else -static inline int qcom_register_ssr_notifier (struct notifier_block *nb) +static inline void *qcom_register_ssr_notifier (const char *name, +struct notifier_block *nb) { -return 0 ; +return NULL ; } -static inline void qcom_unregister_ssr_notifier (struct notifier_block *nb) {} +static inline int qcom_unregister_ssr_notifier (void *notify, +struct notifier_block *nb) +{ +return 0 ; +}#endif
Re: [PATCH v4 1/2] remoteproc: qcom: Add per subsystem SSR notification
Quoted 13 lines · Alex Elder
Quoted 11 lines · Rishabh Bhatnagar
Currently there is a single notification chain which is called whenever any remoteproc shuts down. This leads to all the listeners being notified, and is not an optimal design as kernel drivers might only be interested in listening to notifications from a particular remoteproc. Create a global list of remoteproc notification info data structures. This will hold the name and
notifier_listinformation 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.
Quoted 8 lines
I'm looking at these patches now, without having reviewed the previous versions. Forgive me if I contradict or duplicate previous feedback.
I have a number of suggestions, below.
-Alex
Thanks for your review Alex, some feedback on the patch and your response below.
Quoted 35 lines
Quoted 22 lines
Signed-off-by Siddharth Gupta <[email protected]>Signed-off-by Rishabh Bhatnagar <[email protected]>--- drivers/remoteproc/qcom_common.c | 84 ++++++++++++++++++++++++++++++----- drivers/remoteproc/qcom_common.h | 5 ++- include/linux/remoteproc/qcom_rproc.h | 20 ++++++--- 3 files changed, 90 insertions(+), 19 deletions(-)drivers/remoteproc/qcom_common.c +1
-0@@ -12,6 +12,7 @@#include <linux/module.h> #include <linux/notifier.h> #include <linux/remoteproc.h> +#include <linux/remoteproc/qcom_rproc.h> #include <linux/rpmsg/qcom_glink.h> #include <linux/rpmsg/qcom_smd.h> #include <linux/soc/qcom/mdt_loader.h> @@ -23,7 +24,14 @@#define to_smd_subdev (d) container_of(d, struct qcom_rproc_subdev, subdev)drivers/remoteproc/qcom_common.c line 33
Quoted 10 lines
drivers/remoteproc/qcom_common.c +8
-1#define to_ssr_subdev (d) container_of(d, struct qcom_rproc_ssr, subdev) -static BLOCKING_NOTIFIER_HEAD (ssr_notifiers); +struct qcom_ssr_subsystem { +const char *name; +struct srcu_notifier_head notifier_list; +struct list_head list; +}; + +static LIST_HEAD (qcom_ssr_subsystem_list); +DEFINE_MUTEX (qcom_ssr_subsys_lock);There is no need for this mutex to be global.
Quoted 9 lines
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 199
Quoted 6 lines
drivers/remoteproc/qcom_common.c +1
-0static int glink_subdev_start (struct rproc_subdev *subdev) {@@ -189,39 +197,79 @@ void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) }EXPORT_SYMBOL_GPL (qcom_remove_smd_subdev); +struct qcom_ssr_subsystem *qcom_ssr_get_subsys (const char *name)This function should be made private (static).
Yes.
Quoted 5 lines
I think the mutex should be taken in this function rather than the caller (more on this below). But if you leave it this way, please mention something in a comment that indicates the caller must hold the
qcom_ssr_subsys_lockmutex.
I agree, that would simplify reasoning about the lock.
Quoted 11 lines
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 207
Quoted 7 lines
drivers/remoteproc/qcom_common.c +7
-0+{ +struct qcom_ssr_subsystem *info; + +/* Match in the global qcom_ssr_subsystem_list with name */ +list_for_each_entry (info, &qcom_ssr_subsystem_list, list) { +if (!strcmp (info->name, name)) +return info;This probably isn't strictly necessary, because you are returning a void pointer, but you could do this here:
return ERR_CAST(info);
Info is a struct qcom_ssr_subsystem * and that's the function's return type as well, so Rishabh's approach looks correct to me.
Quoted 34 lines
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 207
Quoted 2 lines
drivers/remoteproc/qcom_common.c +1
-0+ }This is purely a style thing, but the curly braces around the loop body aren't necessary.
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 215
Quoted 7 lines
drivers/remoteproc/qcom_common.c +7
-0+ info =kzalloc (sizeof (*info),GFP_KERNEL ); +if (!info) +return ERR_PTR (-ENOMEM ); + info->name =kstrdup_const (name,GFP_KERNEL ); +srcu_init_notifier_head (&info->notifier_list); + +/* Add to global notif list */ s/notif/notification/
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 216
Quoted 1 line
drivers/remoteproc/qcom_common.c +1
-0+INIT_LIST_HEAD (&info->list);No need to initialize the list element when adding it to a list. Both uts fields will be overwritten anyway.
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 224
Quoted 8 lines
drivers/remoteproc/qcom_common.c +6
-0+list_add_tail (&info->list, &qcom_ssr_subsystem_list); + +return info; +} +/** * qcom_register_ssr_notifier() - register SSR notification handler +* @name: name that will be searched in global ssr subsystem list Maybe just "SSR subsystem name".
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 225
Quoted 1 line
drivers/remoteproc/qcom_common.c +0
-0* @nb: notifier_block to notify for restart notifications
Quoted 8 lines
Drop or modify "restart" in the comment line above.
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 227
Quoted 3 lines
drivers/remoteproc/qcom_common.c +1
-1* -* Returns 0 on success, negative errno on failure. +* Returns a subsystem cookie on success, ERR_PTR on failure. Maybe make the above a @Return: comment.
No @ in that, but "Return: foo" is the appropriate format.
Quoted 8 lines
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 231
Quoted 7 lines
drivers/remoteproc/qcom_common.c +3
-3* -* 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.
Quoted 6 lines
It's not just for stopping, right? Maybe something more like:
Register to receive notification callbacks when remoteproc SSR events occur (pre- and post-startup and pre- and post-shutdown).
And this description of the function should go above the Return:
See https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation
Quoted 68 lines
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 237
Quoted 8 lines
drivers/remoteproc/qcom_common.c +4
-2*/ -int qcom_register_ssr_notifier (struct notifier_block *nb) +void *qcom_register_ssr_notifier (const char *name,struct notifier_block *nb) { -return blocking_notifier_chain_register (&ssr_notifiers, nb); +struct qcom_ssr_subsystem *info; + +mutex_lock (&qcom_ssr_subsys_lock);Can you explain why the mutex is taken here (and in
qcom_add_ssr_subdev()), rather than having the mutex logic be isolated inqcom_ssr_get_subsys()?drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 242
Quoted 5 lines
drivers/remoteproc/qcom_common.c +5
-0+ info =qcom_ssr_get_subsys (name); +if (IS_ERR (info)) { +mutex_unlock (&qcom_ssr_subsys_lock); +return info; + }I don't think there's any need to have the next function call be protected by the mutex, but maybe I'm mistaken.
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 253
Quoted 9 lines
drivers/remoteproc/qcom_common.c +4
-0+srcu_notifier_chain_register (&info->notifier_list, nb); +mutex_unlock (&qcom_ssr_subsys_lock); +return &info->notifier_list; }EXPORT_SYMBOL_GPL (qcom_register_ssr_notifier);/** * qcom_unregister_ssr_notifier() - unregister SSR notification handler +* @notify: subsystem coookie returned from qcom_register_ssr_notifier * @nb: notifier_block to unregister Add a @Return comment (0 on success, %ENOENT otherwise).
Quoted 34 lines
drivers/remoteproc/qcom_common.c +10
-5*/ -void qcom_unregister_ssr_notifier (struct notifier_block *nb) +int qcom_unregister_ssr_notifier (void *notify,struct notifier_block *nb) { -blocking_notifier_chain_unregister (&ssr_notifiers, nb); +return srcu_notifier_chain_unregister (notify, nb); }EXPORT_SYMBOL_GPL (qcom_unregister_ssr_notifier);static void ssr_notify_unprepare (struct rproc_subdev *subdev) {struct qcom_rproc_ssr *ssr =to_ssr_subdev (subdev); +struct qcom_ssr_notif_data data = { + .name = ssr->info->name, + .crashed = false, + }; -blocking_notifier_call_chain (&ssr_notifiers,0 , (void *)ssr->name); +srcu_notifier_call_chain (&ssr->info->notifier_list,0 , &data); } +/** * qcom_add_ssr_subdev() - register subdevice as restart notification source * @rproc: rproc handle @@ -229,12 +277,23 @@ static void ssr_notify_unprepare(struct rproc_subdev *subdev)* @ssr_name: identifier to use for notifications originating from @rproc * * As the @ssr is registered with the @rproc SSR events will be sent to all -* registered listeners in the system as the remoteproc is shut down. +* registered listeners for the particular remoteproc when it is shutdown. */ void qcom_add_ssr_subdev (struct rproc *rproc,struct qcom_rproc_ssr *ssr,const char *ssr_name) { - ssr->name = ssr_name; +struct qcom_ssr_subsystem *info;
Quoted 9 lines
drivers/remoteproc/qcom_common.c static void ssr_notify_unprepare(struct rproc_subdev *subdev) · line 288
Quoted 3 lines
drivers/remoteproc/qcom_common.c +3
-0+ +mutex_lock (&qcom_ssr_subsys_lock); + info =qcom_ssr_get_subsys (ssr_name);If there already exists an SSR subsystem having the given name, its info structure is returned here. Is that OK? In practice, I don't expect this to be a problem, but it would be better to return an error if
You're right...that shouldn't happen. So a WARN_ON() and early return would be in order.
Quoted 41 lines
Quoted 41 lines
drivers/remoteproc/qcom_common.c +9
-0+if (IS_ERR (info)) { +dev_err (&rproc->dev,"Failed to add ssr subdevice\n" ); +mutex_unlock (&qcom_ssr_subsys_lock); +return ; + } + +mutex_unlock (&qcom_ssr_subsys_lock); + ssr->info = info; ssr->subdev.unprepare = ssr_notify_unprepare;rproc_add_subdev (rproc, &ssr->subdev);@@ -249,6 +308,7 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,void qcom_remove_ssr_subdev (struct rproc *rproc,struct qcom_rproc_ssr *ssr) {rproc_remove_subdev (rproc, &ssr->subdev); + ssr->info =NULL ; }EXPORT_SYMBOL_GPL (qcom_remove_ssr_subdev);drivers/remoteproc/qcom_common.h +3
-2@@ -26,10 +26,11 @@ struct qcom_rproc_subdev {struct qcom_smd_edge *edge; }; +struct qcom_ssr_subsystem ; +struct qcom_rproc_ssr {struct rproc_subdev subdev; - -const char *name; +struct qcom_ssr_subsystem *info; }; void qcom_add_glink_subdev(struct rproc *rproc, struct qcom_rproc_glink *glink,include/linux/remoteproc/qcom_rproc.h +0
-2@@ -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);
Quoted 9 lines
include/linux/remoteproc/qcom_rproc.h line 10
Quoted 3 lines
include/linux/remoteproc/qcom_rproc.h +3
-0+struct qcom_ssr_notif_data { +const char *name; +bool crashed;Is the crashed field strictly necessary? Could we instead have a
QCOM_SSR_CRASHEDevent (in place ofQCOM_SSR_BEFORE_SHUTDOWN)? I don't know, it's possible doing it the way you do ultimately simplifies the logic... So I'm asking, but not suggesting.
I looked at something similar for the subdev callbacks, but concluded that most cases I could find was cleaner if I just passed a bool crashed than having two separate functions to deal with in the client drivers.
So I'm okay with this.
Regards,
Bjorn
Quoted 21 lines
Quoted 20 lines
include/linux/remoteproc/qcom_rproc.h +12
-3+}; + +void *qcom_register_ssr_notifier (const char *name,struct notifier_block *nb); +int qcom_unregister_ssr_notifier (void *notify,struct notifier_block *nb);#else -static inline int qcom_register_ssr_notifier (struct notifier_block *nb) +static inline void *qcom_register_ssr_notifier (const char *name, +struct notifier_block *nb) { -return 0 ; +return NULL ; } -static inline void qcom_unregister_ssr_notifier (struct notifier_block *nb) {} +static inline int qcom_unregister_ssr_notifier (void *notify, +struct notifier_block *nb) +{ +return 0 ; +}#endif
Re: [PATCH v4 1/2] remoteproc: qcom: Add per subsystem SSR notification
Quoted 240 lines · Bjorn Andersson
Quoted 13 lines · Alex Elder
Quoted 11 lines · Rishabh Bhatnagar
Currently there is a single notification chain which is called whenever any remoteproc shuts down. This leads to all the listeners being notified, and is not an optimal design as kernel drivers might only be interested in listening to notifications from a particular remoteproc. Create a global list of remoteproc notification info data structures. This will hold the name and
notifier_listinformation 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.Quoted 8 lines
I'm looking at these patches now, without having reviewed the previous versions. Forgive me if I contradict or duplicate previous feedback.
I have a number of suggestions, below.
-Alex
Thanks for your review Alex, some feedback on the patch and your response below.
Quoted 35 lines
Quoted 22 lines
Signed-off-by Siddharth Gupta <[email protected]>Signed-off-by Rishabh Bhatnagar <[email protected]>--- drivers/remoteproc/qcom_common.c | 84 ++++++++++++++++++++++++++++++----- drivers/remoteproc/qcom_common.h | 5 ++- include/linux/remoteproc/qcom_rproc.h | 20 ++++++--- 3 files changed, 90 insertions(+), 19 deletions(-)drivers/remoteproc/qcom_common.c +1
-0@@ -12,6 +12,7 @@#include <linux/module.h> #include <linux/notifier.h> #include <linux/remoteproc.h> +#include <linux/remoteproc/qcom_rproc.h> #include <linux/rpmsg/qcom_glink.h> #include <linux/rpmsg/qcom_smd.h> #include <linux/soc/qcom/mdt_loader.h> @@ -23,7 +24,14 @@#define to_smd_subdev (d) container_of(d, struct qcom_rproc_subdev, subdev)drivers/remoteproc/qcom_common.c line 33
Quoted 10 lines
drivers/remoteproc/qcom_common.c +8
-1#define to_ssr_subdev (d) container_of(d, struct qcom_rproc_ssr, subdev) -static BLOCKING_NOTIFIER_HEAD (ssr_notifiers); +struct qcom_ssr_subsystem { +const char *name; +struct srcu_notifier_head notifier_list; +struct list_head list; +}; + +static LIST_HEAD (qcom_ssr_subsystem_list); +DEFINE_MUTEX (qcom_ssr_subsys_lock);There is no need for this mutex to be global.
Quoted 9 lines
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 199
Quoted 6 lines
drivers/remoteproc/qcom_common.c +1
-0static int glink_subdev_start (struct rproc_subdev *subdev) {@@ -189,39 +197,79 @@ void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) }EXPORT_SYMBOL_GPL (qcom_remove_smd_subdev); +struct qcom_ssr_subsystem *qcom_ssr_get_subsys (const char *name)This function should be made private (static).
Yes.
Quoted 5 lines
I think the mutex should be taken in this function rather than the caller (more on this below). But if you leave it this way, please mention something in a comment that indicates the caller must hold the
qcom_ssr_subsys_lockmutex.I agree, that would simplify reasoning about the lock.
Quoted 11 lines
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 207
Quoted 7 lines
drivers/remoteproc/qcom_common.c +7
-0+{ +struct qcom_ssr_subsystem *info; + +/* Match in the global qcom_ssr_subsystem_list with name */ +list_for_each_entry (info, &qcom_ssr_subsystem_list, list) { +if (!strcmp (info->name, name)) +return info;This probably isn't strictly necessary, because you are returning a void pointer, but you could do this here:
return ERR_CAST(info);Info is a struct
qcom_ssr_subsystem* and that's the function's return type as well, so Rishabh's approach looks correct to me.Quoted 34 lines
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 207
Quoted 2 lines
drivers/remoteproc/qcom_common.c +1
-0+ }This is purely a style thing, but the curly braces around the loop body aren't necessary.
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 215
Quoted 7 lines
drivers/remoteproc/qcom_common.c +7
-0+ info =kzalloc (sizeof (*info),GFP_KERNEL ); +if (!info) +return ERR_PTR (-ENOMEM ); + info->name =kstrdup_const (name,GFP_KERNEL ); +srcu_init_notifier_head (&info->notifier_list); + +/* Add to global notif list */ s/notif/notification/
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 216
Quoted 1 line
drivers/remoteproc/qcom_common.c +1
-0+INIT_LIST_HEAD (&info->list);No need to initialize the list element when adding it to a list. Both uts fields will be overwritten anyway.
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 224
Quoted 8 lines
drivers/remoteproc/qcom_common.c +6
-0+list_add_tail (&info->list, &qcom_ssr_subsystem_list); + +return info; +} +/** * qcom_register_ssr_notifier() - register SSR notification handler +* @name: name that will be searched in global ssr subsystem list Maybe just "SSR subsystem name".
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 225
Quoted 1 line
drivers/remoteproc/qcom_common.c +0
-0* @nb: notifier_block to notify for restart notifications Quoted 8 lines
Drop or modify "restart" in the comment line above.
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 227
Quoted 3 lines
drivers/remoteproc/qcom_common.c +1
-1* -* Returns 0 on success, negative errno on failure. +* Returns a subsystem cookie on success, ERR_PTR on failure. Maybe make the above a @Return: comment.
No @ in that, but "Return: foo" is the appropriate format.
Quoted 8 lines
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 231
Quoted 7 lines
drivers/remoteproc/qcom_common.c +3
-3* -* 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. Quoted 6 lines
It's not just for stopping, right? Maybe something more like:
Register to receive notification callbacks when remoteproc SSR events occur (pre- and post-startup and pre- and post-shutdown).And this description of the function should go above the Return:
See
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentationQuoted 68 lines
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 237
Quoted 8 lines
drivers/remoteproc/qcom_common.c +4
-2*/ -int qcom_register_ssr_notifier (struct notifier_block *nb) +void *qcom_register_ssr_notifier (const char *name,struct notifier_block *nb) { -return blocking_notifier_chain_register (&ssr_notifiers, nb); +struct qcom_ssr_subsystem *info; + +mutex_lock (&qcom_ssr_subsys_lock);Can you explain why the mutex is taken here (and in
qcom_add_ssr_subdev()), rather than having the mutex logic be isolated inqcom_ssr_get_subsys()?drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 242
Quoted 5 lines
drivers/remoteproc/qcom_common.c +5
-0+ info =qcom_ssr_get_subsys (name); +if (IS_ERR (info)) { +mutex_unlock (&qcom_ssr_subsys_lock); +return info; + }I don't think there's any need to have the next function call be protected by the mutex, but maybe I'm mistaken.
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 253
Quoted 9 lines
drivers/remoteproc/qcom_common.c +4
-0+srcu_notifier_chain_register (&info->notifier_list, nb); +mutex_unlock (&qcom_ssr_subsys_lock); +return &info->notifier_list; }EXPORT_SYMBOL_GPL (qcom_register_ssr_notifier);/** * qcom_unregister_ssr_notifier() - unregister SSR notification handler +* @notify: subsystem coookie returned from qcom_register_ssr_notifier * @nb: notifier_block to unregister Add a @Return comment (0 on success, %ENOENT otherwise).
Quoted 34 lines
drivers/remoteproc/qcom_common.c +10
-5*/ -void qcom_unregister_ssr_notifier (struct notifier_block *nb) +int qcom_unregister_ssr_notifier (void *notify,struct notifier_block *nb) { -blocking_notifier_chain_unregister (&ssr_notifiers, nb); +return srcu_notifier_chain_unregister (notify, nb); }EXPORT_SYMBOL_GPL (qcom_unregister_ssr_notifier);static void ssr_notify_unprepare (struct rproc_subdev *subdev) {struct qcom_rproc_ssr *ssr =to_ssr_subdev (subdev); +struct qcom_ssr_notif_data data = { + .name = ssr->info->name, + .crashed = false, + }; -blocking_notifier_call_chain (&ssr_notifiers,0 , (void *)ssr->name); +srcu_notifier_call_chain (&ssr->info->notifier_list,0 , &data); } +/** * qcom_add_ssr_subdev() - register subdevice as restart notification source * @rproc: rproc handle @@ -229,12 +277,23 @@ static void ssr_notify_unprepare(struct rproc_subdev *subdev)* @ssr_name: identifier to use for notifications originating from @rproc * * As the @ssr is registered with the @rproc SSR events will be sent to all -* registered listeners in the system as the remoteproc is shut down. +* registered listeners for the particular remoteproc when it is shutdown. */ void qcom_add_ssr_subdev (struct rproc *rproc,struct qcom_rproc_ssr *ssr,const char *ssr_name) { - ssr->name = ssr_name; +struct qcom_ssr_subsystem *info;Quoted 9 lines
drivers/remoteproc/qcom_common.c static void ssr_notify_unprepare(struct rproc_subdev *subdev) · line 288
Quoted 3 lines
drivers/remoteproc/qcom_common.c +3
-0+ +mutex_lock (&qcom_ssr_subsys_lock); + info =qcom_ssr_get_subsys (ssr_name);If there already exists an SSR subsystem having the given name, its info structure is returned here. Is that OK? In practice, I don't expect this to be a problem, but it would be better to return an error if
Quoted 3 lines
You're right...that shouldn't happen. So a
WARN_ON()and early return would be in order.
the info structure needs to be embedded in the qcom_rproc_ssr struct in case where clients register for notifications even before that particular ssr subdevice is registered. Logically add_ssr_subdev shouldn't happen twice for a rproc without doing remove.
Quoted 81 lines
Quoted 41 lines
Quoted 41 lines
drivers/remoteproc/qcom_common.c +9
-0+if (IS_ERR (info)) { +dev_err (&rproc->dev,"Failed to add ssr subdevice\n" ); +mutex_unlock (&qcom_ssr_subsys_lock); +return ; + } + +mutex_unlock (&qcom_ssr_subsys_lock); + ssr->info = info; ssr->subdev.unprepare = ssr_notify_unprepare;rproc_add_subdev (rproc, &ssr->subdev);@@ -249,6 +308,7 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,void qcom_remove_ssr_subdev (struct rproc *rproc,struct qcom_rproc_ssr *ssr) {rproc_remove_subdev (rproc, &ssr->subdev); + ssr->info =NULL ; }EXPORT_SYMBOL_GPL (qcom_remove_ssr_subdev);drivers/remoteproc/qcom_common.h +3
-2@@ -26,10 +26,11 @@ struct qcom_rproc_subdev {struct qcom_smd_edge *edge; }; +struct qcom_ssr_subsystem ; +struct qcom_rproc_ssr {struct rproc_subdev subdev; - -const char *name; +struct qcom_ssr_subsystem *info; }; void qcom_add_glink_subdev(struct rproc *rproc, struct qcom_rproc_glink *glink,include/linux/remoteproc/qcom_rproc.h +0
-2@@ -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);Quoted 9 lines
include/linux/remoteproc/qcom_rproc.h line 10
Quoted 3 lines
include/linux/remoteproc/qcom_rproc.h +3
-0+struct qcom_ssr_notif_data { +const char *name; +bool crashed;Is the crashed field strictly necessary? Could we instead have a
QCOM_SSR_CRASHEDevent (in place ofQCOM_SSR_BEFORE_SHUTDOWN)? I don't know, it's possible doing it the way you do ultimately simplifies the logic... So I'm asking, but not suggesting.I looked at something similar for the subdev callbacks, but concluded that most cases I could find was cleaner if I just passed a bool crashed than having two separate functions to deal with in the client drivers.
So I'm okay with this.
Regards,
BjornQuoted 21 lines
Quoted 20 lines
include/linux/remoteproc/qcom_rproc.h +12
-3+}; + +void *qcom_register_ssr_notifier (const char *name,struct notifier_block *nb); +int qcom_unregister_ssr_notifier (void *notify,struct notifier_block *nb);#else -static inline int qcom_register_ssr_notifier (struct notifier_block *nb) +static inline void *qcom_register_ssr_notifier (const char *name, +struct notifier_block *nb) { -return 0 ; +return NULL ; } -static inline void qcom_unregister_ssr_notifier (struct notifier_block *nb) {} +static inline int qcom_unregister_ssr_notifier (void *notify, +struct notifier_block *nb) +{ +return 0 ; +}#endif
Re: [PATCH v4 1/2] remoteproc: qcom: Add per subsystem SSR notification
Quoted 241 lines · On Sat 20 Jun 12:48 PDT 2020, [email protected] wrote:
Quoted 240 lines · Bjorn Andersson
Quoted 13 lines · Alex Elder
Quoted 11 lines · Rishabh Bhatnagar
Currently there is a single notification chain which is called whenever any remoteproc shuts down. This leads to all the listeners being notified, and is not an optimal design as kernel drivers might only be interested in listening to notifications from a particular remoteproc. Create a global list of remoteproc notification info data structures. This will hold the name and
notifier_listinformation 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.Quoted 8 lines
I'm looking at these patches now, without having reviewed the previous versions. Forgive me if I contradict or duplicate previous feedback.
I have a number of suggestions, below.
-Alex
Thanks for your review Alex, some feedback on the patch and your response below.
Quoted 35 lines
Quoted 22 lines
Signed-off-by Siddharth Gupta <[email protected]>Signed-off-by Rishabh Bhatnagar <[email protected]>--- drivers/remoteproc/qcom_common.c | 84 ++++++++++++++++++++++++++++++----- drivers/remoteproc/qcom_common.h | 5 ++- include/linux/remoteproc/qcom_rproc.h | 20 ++++++--- 3 files changed, 90 insertions(+), 19 deletions(-)drivers/remoteproc/qcom_common.c +1
-0@@ -12,6 +12,7 @@#include <linux/module.h> #include <linux/notifier.h> #include <linux/remoteproc.h> +#include <linux/remoteproc/qcom_rproc.h> #include <linux/rpmsg/qcom_glink.h> #include <linux/rpmsg/qcom_smd.h> #include <linux/soc/qcom/mdt_loader.h> @@ -23,7 +24,14 @@#define to_smd_subdev (d) container_of(d, struct qcom_rproc_subdev, subdev)drivers/remoteproc/qcom_common.c line 33
Quoted 10 lines
drivers/remoteproc/qcom_common.c +8
-1#define to_ssr_subdev (d) container_of(d, struct qcom_rproc_ssr, subdev) -static BLOCKING_NOTIFIER_HEAD (ssr_notifiers); +struct qcom_ssr_subsystem { +const char *name; +struct srcu_notifier_head notifier_list; +struct list_head list; +}; + +static LIST_HEAD (qcom_ssr_subsystem_list); +DEFINE_MUTEX (qcom_ssr_subsys_lock);There is no need for this mutex to be global.
Quoted 9 lines
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 199
Quoted 6 lines
drivers/remoteproc/qcom_common.c +1
-0static int glink_subdev_start (struct rproc_subdev *subdev) {@@ -189,39 +197,79 @@ void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) }EXPORT_SYMBOL_GPL (qcom_remove_smd_subdev); +struct qcom_ssr_subsystem *qcom_ssr_get_subsys (const char *name)This function should be made private (static).
Yes.
Quoted 5 lines
I think the mutex should be taken in this function rather than the caller (more on this below). But if you leave it this way, please mention something in a comment that indicates the caller must hold the
qcom_ssr_subsys_lockmutex.I agree, that would simplify reasoning about the lock.
Quoted 11 lines
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 207
Quoted 7 lines
drivers/remoteproc/qcom_common.c +7
-0+{ +struct qcom_ssr_subsystem *info; + +/* Match in the global qcom_ssr_subsystem_list with name */ +list_for_each_entry (info, &qcom_ssr_subsystem_list, list) { +if (!strcmp (info->name, name)) +return info;This probably isn't strictly necessary, because you are returning a void pointer, but you could do this here:
return ERR_CAST(info);Info is a struct
qcom_ssr_subsystem* and that's the function's return type as well, so Rishabh's approach looks correct to me.Quoted 34 lines
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 207
Quoted 2 lines
drivers/remoteproc/qcom_common.c +1
-0+ }This is purely a style thing, but the curly braces around the loop body aren't necessary.
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 215
Quoted 7 lines
drivers/remoteproc/qcom_common.c +7
-0+ info =kzalloc (sizeof (*info),GFP_KERNEL ); +if (!info) +return ERR_PTR (-ENOMEM ); + info->name =kstrdup_const (name,GFP_KERNEL ); +srcu_init_notifier_head (&info->notifier_list); + +/* Add to global notif list */ s/notif/notification/
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 216
Quoted 1 line
drivers/remoteproc/qcom_common.c +1
-0+INIT_LIST_HEAD (&info->list);No need to initialize the list element when adding it to a list. Both uts fields will be overwritten anyway.
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 224
Quoted 8 lines
drivers/remoteproc/qcom_common.c +6
-0+list_add_tail (&info->list, &qcom_ssr_subsystem_list); + +return info; +} +/** * qcom_register_ssr_notifier() - register SSR notification handler +* @name: name that will be searched in global ssr subsystem list Maybe just "SSR subsystem name".
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 225
Quoted 1 line
drivers/remoteproc/qcom_common.c +0
-0* @nb: notifier_block to notify for restart notifications Quoted 8 lines
Drop or modify "restart" in the comment line above.
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 227
Quoted 3 lines
drivers/remoteproc/qcom_common.c +1
-1* -* Returns 0 on success, negative errno on failure. +* Returns a subsystem cookie on success, ERR_PTR on failure. Maybe make the above a @Return: comment.
No @ in that, but "Return: foo" is the appropriate format.
Quoted 8 lines
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 231
Quoted 7 lines
drivers/remoteproc/qcom_common.c +3
-3* -* 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. Quoted 6 lines
It's not just for stopping, right? Maybe something more like:
Register to receive notification callbacks when remoteproc SSR events occur (pre- and post-startup and pre- and post-shutdown).And this description of the function should go above the Return:
See
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentationQuoted 77 lines
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 237
Quoted 8 lines
drivers/remoteproc/qcom_common.c +4
-2*/ -int qcom_register_ssr_notifier (struct notifier_block *nb) +void *qcom_register_ssr_notifier (const char *name,struct notifier_block *nb) { -return blocking_notifier_chain_register (&ssr_notifiers, nb); +struct qcom_ssr_subsystem *info; + +mutex_lock (&qcom_ssr_subsys_lock);Can you explain why the mutex is taken here (and in
qcom_add_ssr_subdev()), rather than having the mutex logic be isolated inqcom_ssr_get_subsys()?drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 242
Quoted 5 lines
drivers/remoteproc/qcom_common.c +5
-0+ info =qcom_ssr_get_subsys (name); +if (IS_ERR (info)) { +mutex_unlock (&qcom_ssr_subsys_lock); +return info; + }I don't think there's any need to have the next function call be protected by the mutex, but maybe I'm mistaken.
drivers/remoteproc/qcom_common.c void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) · line 253
Quoted 9 lines
drivers/remoteproc/qcom_common.c +4
-0+srcu_notifier_chain_register (&info->notifier_list, nb); +mutex_unlock (&qcom_ssr_subsys_lock); +return &info->notifier_list; }EXPORT_SYMBOL_GPL (qcom_register_ssr_notifier);/** * qcom_unregister_ssr_notifier() - unregister SSR notification handler +* @notify: subsystem coookie returned from qcom_register_ssr_notifier * @nb: notifier_block to unregister Add a @Return comment (0 on success, %ENOENT otherwise).
Quoted 27 lines
drivers/remoteproc/qcom_common.c +8
-4*/ -void qcom_unregister_ssr_notifier (struct notifier_block *nb) +int qcom_unregister_ssr_notifier (void *notify,struct notifier_block *nb) { -blocking_notifier_chain_unregister (&ssr_notifiers, nb); +return srcu_notifier_chain_unregister (notify, nb); }EXPORT_SYMBOL_GPL (qcom_unregister_ssr_notifier);static void ssr_notify_unprepare (struct rproc_subdev *subdev) {struct qcom_rproc_ssr *ssr =to_ssr_subdev (subdev); +struct qcom_ssr_notif_data data = { + .name = ssr->info->name, + .crashed = false, + }; -blocking_notifier_call_chain (&ssr_notifiers,0 , (void *)ssr->name); +srcu_notifier_call_chain (&ssr->info->notifier_list,0 , &data); } +/** * qcom_add_ssr_subdev() - register subdevice as restart notification source * @rproc: rproc handle @@ -229,12 +277,23 @@ static void ssr_notify_unprepare(struct rproc_subdev *subdev)* @ssr_name: identifier to use for notifications originating from @rproc * * As the @ssr is registered with the @rproc SSR events will be sent to all -* registered listeners in the system as the remoteproc is shut down. drivers/remoteproc/qcom_common.c static void ssr_notify_unprepare(struct rproc_subdev *subdev) · line 288
Quoted 10 lines
drivers/remoteproc/qcom_common.c +5
-1+* registered listeners for the particular remoteproc when it is shutdown. */ void qcom_add_ssr_subdev (struct rproc *rproc,struct qcom_rproc_ssr *ssr,const char *ssr_name) { - ssr->name = ssr_name; +struct qcom_ssr_subsystem *info; + +mutex_lock (&qcom_ssr_subsys_lock); + info =qcom_ssr_get_subsys (ssr_name);If there already exists an SSR subsystem having the given name, its info structure is returned here. Is that OK? In practice, I don't expect this to be a problem, but it would be better to return an error if
Quoted 7 lines
Quoted 3 lines
You're right...that shouldn't happen. So a
WARN_ON()and early return would be in order.the info structure needs to be embedded in the
qcom_rproc_ssrstruct in case where clients register for notifications even before that particular ssr subdevice is registered. Logicallyadd_ssr_subdevshouldn't happen twice for a rproc without doing remove.
You're right, I forgot that part of it. So if anything we would have a check to see that the given info isn't already associated with a remoteproc instance. But I don't see a link in that direction, so I'm fine with ignoring this.
Regards,
Bjorn
PATCH v4 2/2 remoteproc: qcom: Add notification types to SSR
In reply to Rishabh BhatnagarFrom: Siddharth Gupta <[email protected]>
The SSR subdevice only adds callback for the unprepare event. Add callbacks for unprepare, start and prepare events. The client driver for a particular remoteproc might be interested in knowing the status of the remoteproc while undergoing SSR, not just when the remoteproc has finished shutting down.
--- drivers/remoteproc/qcom_common.c | 46 +++++++++++++++++++++++++++++++++-- include/linux/remoteproc/qcom_rproc.h | 14 +++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-)
drivers/remoteproc/qcom_common.c +44 -2
@@ -228,7 +228,7 @@ struct qcom_ssr_subsystem *qcom_ssr_get_subsys(const char *name)* * This registers the @nb notifier block as part the notifier chain for a * remoteproc associated with @name. The notifier block's callback -* will be invoked when the particular remote processor is stopped. +* will be invoked when the particular remote processor is started/stopped. */ void *qcom_register_ssr_notifier (const char *name,struct notifier_block *nb) {
@@ -258,6 +258,44 @@ int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb) }EXPORT_SYMBOL_GPL (qcom_unregister_ssr_notifier); +static int ssr_notify_prepare (struct rproc_subdev *subdev) +{ +struct qcom_rproc_ssr *ssr =to_ssr_subdev (subdev); +struct qcom_ssr_notif_data data = { + .name = ssr->info->name, + .crashed = false, + }; + +srcu_notifier_call_chain (&ssr->info->notifier_list, +QCOM_SSR_BEFORE_POWERUP , &data); +return 0 ; +} + +static int ssr_notify_start (struct rproc_subdev *subdev) +{ +struct qcom_rproc_ssr *ssr =to_ssr_subdev (subdev); +struct qcom_ssr_notif_data data = { + .name = ssr->info->name, + .crashed = false, + }; + +srcu_notifier_call_chain (&ssr->info->notifier_list, +QCOM_SSR_AFTER_POWERUP , &data); +return 0 ; +} + +static void ssr_notify_stop (struct rproc_subdev *subdev,bool crashed) +{ +struct qcom_rproc_ssr *ssr =to_ssr_subdev (subdev); +struct qcom_ssr_notif_data data = { + .name = ssr->info->name, + .crashed = crashed, + }; + +srcu_notifier_call_chain (&ssr->info->notifier_list, +QCOM_SSR_BEFORE_SHUTDOWN , &data); +} +static void ssr_notify_unprepare (struct rproc_subdev *subdev) {struct qcom_rproc_ssr *ssr =to_ssr_subdev (subdev);
@@ -266,7 +304,8 @@ static void ssr_notify_unprepare(struct rproc_subdev *subdev) .crashed = false, }; -srcu_notifier_call_chain (&ssr->info->notifier_list,0 , &data); +srcu_notifier_call_chain (&ssr->info->notifier_list, +QCOM_SSR_AFTER_SHUTDOWN , &data); }
@@ -294,6 +333,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,mutex_unlock (&qcom_ssr_subsys_lock); ssr->info = info; + ssr->subdev.prepare = ssr_notify_prepare; + ssr->subdev.start = ssr_notify_start; + ssr->subdev.stop = ssr_notify_stop; ssr->subdev.unprepare = ssr_notify_unprepare;rproc_add_subdev (rproc, &ssr->subdev);
include/linux/remoteproc/qcom_rproc.h +14 -0
@@ -5,6 +5,20 @@ #if IS_ENABLED(CONFIG_QCOM_RPROC_COMMON) +/** + * enum qcom_ssr_notif_type - Different stages of remoteproc notifications + * @QCOM_SSR_BEFORE_SHUTDOWN: unprepare stage of remoteproc + * @QCOM_SSR_AFTER_SHUTDOWN: stop stage of remoteproc + * @QCOM_SSR_BEFORE_POWERUP: prepare stage of remoteproc + * @QCOM_SSR_AFTER_POWERUP: start stage of remoteproc + */ +enum qcom_ssr_notif_type { + QCOM_SSR_BEFORE_SHUTDOWN, + QCOM_SSR_AFTER_SHUTDOWN, + QCOM_SSR_BEFORE_POWERUP, + QCOM_SSR_AFTER_POWERUP, +}; + struct qcom_ssr_notif_data { const char *name; bool crashed;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v4 2/2] remoteproc: qcom: Add notification types to SSR
Quoted 4 lines · Rishabh Bhatnagar
From: Siddharth Gupta <[email protected]>
The SSR subdevice only adds callback for the unprepare event. Add callbacks for unprepare, start and prepare events. The client driver for a particular
for prepare, start, and stop events
Quoted 16 lines
remoteproc might be interested in knowing the status of the remoteproc while undergoing SSR, not just when the remoteproc has finished shutting down.
Signed-off-by Siddharth Gupta <[email protected]>Signed-off-by Rishabh Bhatnagar <[email protected]>--- drivers/remoteproc/qcom_common.c | 46 +++++++++++++++++++++++++++++++++-- include/linux/remoteproc/qcom_rproc.h | 14 +++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-)drivers/remoteproc/qcom_common.c +0
-0
drivers/remoteproc/qcom_common.c struct qcom_ssr_subsystem *qcom_ssr_get_subsys(const char *name) · line 231
Quoted 5 lines
drivers/remoteproc/qcom_common.c +1
-1@@ -228,7 +228,7 @@ struct qcom_ssr_subsystem *qcom_ssr_get_subsys(const char *name)* * This registers the @nb notifier block as part the notifier chain for a * remoteproc associated with @name. The notifier block's callback -* will be invoked when the particular remote processor is stopped. +* will be invoked when the particular remote processor is started/stopped.
Maybe something more like:
will be invoked when an SSR event occurs for the named remote processor.
Quoted 73 lines
drivers/remoteproc/qcom_common.c +43
-1*/ void *qcom_register_ssr_notifier (const char *name,struct notifier_block *nb) {@@ -258,6 +258,44 @@ int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb) }EXPORT_SYMBOL_GPL (qcom_unregister_ssr_notifier); +static int ssr_notify_prepare (struct rproc_subdev *subdev) +{ +struct qcom_rproc_ssr *ssr =to_ssr_subdev (subdev); +struct qcom_ssr_notif_data data = { + .name = ssr->info->name, + .crashed = false, + }; + +srcu_notifier_call_chain (&ssr->info->notifier_list, +QCOM_SSR_BEFORE_POWERUP , &data); +return 0 ; +} + +static int ssr_notify_start (struct rproc_subdev *subdev) +{ +struct qcom_rproc_ssr *ssr =to_ssr_subdev (subdev); +struct qcom_ssr_notif_data data = { + .name = ssr->info->name, + .crashed = false, + }; + +srcu_notifier_call_chain (&ssr->info->notifier_list, +QCOM_SSR_AFTER_POWERUP , &data); +return 0 ; +} + +static void ssr_notify_stop (struct rproc_subdev *subdev,bool crashed) +{ +struct qcom_rproc_ssr *ssr =to_ssr_subdev (subdev); +struct qcom_ssr_notif_data data = { + .name = ssr->info->name, + .crashed = crashed, + }; + +srcu_notifier_call_chain (&ssr->info->notifier_list, +QCOM_SSR_BEFORE_SHUTDOWN , &data); +} +static void ssr_notify_unprepare (struct rproc_subdev *subdev) {struct qcom_rproc_ssr *ssr =to_ssr_subdev (subdev);@@ -266,7 +304,8 @@ static void ssr_notify_unprepare(struct rproc_subdev *subdev) .crashed = false, }; -srcu_notifier_call_chain (&ssr->info->notifier_list,0 , &data); +srcu_notifier_call_chain (&ssr->info->notifier_list, +QCOM_SSR_AFTER_SHUTDOWN , &data); }@@ -294,6 +333,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,mutex_unlock (&qcom_ssr_subsys_lock); ssr->info = info; + ssr->subdev.prepare = ssr_notify_prepare; + ssr->subdev.start = ssr_notify_start; + ssr->subdev.stop = ssr_notify_stop; ssr->subdev.unprepare = ssr_notify_unprepare;rproc_add_subdev (rproc, &ssr->subdev);include/linux/remoteproc/qcom_rproc.h +0
-0
include/linux/remoteproc/qcom_rproc.h line 13
Quoted 9 lines
include/linux/remoteproc/qcom_rproc.h +6
-0@@ -5,6 +5,20 @@#if IS_ENABLED (CONFIG_QCOM_RPROC_COMMON ) +/** +* enum qcom_ssr_notif_type - Different stages of remoteproc notifications +* @QCOM_SSR_BEFORE_SHUTDOWN: unprepare stage of remoteproc +* @QCOM_SSR_AFTER_SHUTDOWN: stop stage of remoteproc +* @QCOM_SSR_BEFORE_POWERUP: prepare stage of remoteproc +* @QCOM_SSR_AFTER_POWERUP: start stage of remoteproc
I think your explanations of these symbols are less clear than the symbol names themselves... In any case, I wouldn't refer to these as "stages of notifications" but instead something more like startup/shutdown related events for a remote processor.
I personally might have ordered them differently too:
So maybe more like:
BEFORE_POWERUP Remoteproc about to start (prepare)
AFTER_POWERUP Remoteproc is running (start)
BEFORE_SHUTDOWN Remoteproc crashed, or shutting down (stop)
AFTER_SHUTDOWN Remoteproc is down (unprepare)
-Alex
Quoted 12 lines
include/linux/remoteproc/qcom_rproc.h +8
-0+*/ +enum qcom_ssr_notif_type { +QCOM_SSR_BEFORE_SHUTDOWN , +QCOM_SSR_AFTER_SHUTDOWN , +QCOM_SSR_BEFORE_POWERUP , +QCOM_SSR_AFTER_POWERUP , +}; +struct qcom_ssr_notif_data {const char *name;bool crashed;