* [PATCH v3 0/2] Extend SSR notifications framework @ 2020-04-28 22:16 Rishabh Bhatnagar 2020-04-28 22:16 ` [PATCH v3 1/2] remoteproc: qcom: Add per subsystem SSR notification Rishabh Bhatnagar 2020-04-28 22:16 ` [PATCH v3 2/2] remoteproc: qcom: Add notification types to SSR Rishabh Bhatnagar 0 siblings, 2 replies; 7+ messages in thread From: Rishabh Bhatnagar @ 2020-04-28 22:16 UTC (permalink / raw) To: linux-remoteproc, bjorn.andersson Cc: ohad, linux-kernel, tsoni, psodagud, sidgup, Rishabh Bhatnagar This set of patches gives kernel client drivers the ability to register for a particular remoteproc's SSR notifications. Also the notifications are extended to before/after-powerup/shutdown stages. It also fixes the bug where clients need to register for notifications again if the platform driver is removed. This is done by creating a global list of per-remoteproc notification info data structures which remain static. An API is exported to register for a remoteproc's SSR notifications and uses remoteproc's ssr_name and notifier block as arguments. Rishabh Bhatnagar (1): remoteproc: qcom: Add per subsystem SSR notification Siddharth Gupta (1): remoteproc: qcom: Add notification types to SSR v3 -> v2: - Create a global list of per remoteproc notification data structure - Pass ssr_name and crashed information as part of notification data - Move notification type enum to qcom_rproc.h from remoteproc.h v2 -> v1: - Fix commit text drivers/remoteproc/qcom_common.c | 133 ++++++++++++++++++++++++++++++---- drivers/remoteproc/qcom_common.h | 11 ++- include/linux/remoteproc/qcom_rproc.h | 34 +++++++-- 3 files changed, 157 insertions(+), 21 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 1/2] remoteproc: qcom: Add per subsystem SSR notification 2020-04-28 22:16 [PATCH v3 0/2] Extend SSR notifications framework Rishabh Bhatnagar @ 2020-04-28 22:16 ` Rishabh Bhatnagar 2020-04-29 6:15 ` kbuild test robot 2020-05-19 20:38 ` Bjorn Andersson 2020-04-28 22:16 ` [PATCH v3 2/2] remoteproc: qcom: Add notification types to SSR Rishabh Bhatnagar 1 sibling, 2 replies; 7+ messages in thread From: Rishabh Bhatnagar @ 2020-04-28 22:16 UTC (permalink / raw) To: linux-remoteproc, bjorn.andersson Cc: ohad, linux-kernel, tsoni, psodagud, sidgup, Rishabh Bhatnagar Currently there is a single notification chain which is called whenever any remoteproc shuts down. This leads to all the listeners being notified, and is not an optimal design as kernel drivers might only be interested in listening to notifications from a particular remoteproc. Create a global list of remoteproc notification info data structures. This will hold the name and notifier_list information for a particular remoteproc. The API to register for notifications will use name argument to retrieve the notification info data structure and the notifier block will be added to that data structure's notification chain. Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org> --- drivers/remoteproc/qcom_common.c | 89 ++++++++++++++++++++++++++++++----- drivers/remoteproc/qcom_common.h | 10 +++- include/linux/remoteproc/qcom_rproc.h | 20 ++++++-- 3 files changed, 99 insertions(+), 20 deletions(-) diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c index 60650bc..7cd17be 100644 --- a/drivers/remoteproc/qcom_common.c +++ b/drivers/remoteproc/qcom_common.c @@ -15,16 +15,18 @@ #include <linux/rpmsg/qcom_glink.h> #include <linux/rpmsg/qcom_smd.h> #include <linux/soc/qcom/mdt_loader.h> +#include <linux/remoteproc/qcom_rproc.h> #include "remoteproc_internal.h" #include "qcom_common.h" +#define MAX_NAME_LEN 20 +DEFINE_MUTEX(rproc_notif_lock); + #define to_glink_subdev(d) container_of(d, struct qcom_rproc_glink, subdev) #define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, subdev) #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, subdev) -static BLOCKING_NOTIFIER_HEAD(ssr_notifiers); - static int glink_subdev_start(struct rproc_subdev *subdev) { struct qcom_rproc_glink *glink = to_glink_subdev(subdev); @@ -174,39 +176,81 @@ void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) } EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev); +struct rproc_notif_info *find_notif_info(const char *name) +{ + struct rproc_notif_info *info; + + /* Match in the global rproc_notif_list with name */ + list_for_each_entry(info, &rproc_notif_list, list) { + if (!strncmp(info->name, name, strlen(name))) + return info; + } + return NULL; +} + /** * qcom_register_ssr_notifier() - register SSR notification handler + * @name: pointer to name which will be searched in the global notif_list * @nb: notifier_block to notify for restart notifications * - * Returns 0 on success, negative errno on failure. + * Returns pointer to srcu notifier head on success, ERR_PTR on failure. * - * This register the @notify function as handler for restart notifications. As - * remote processors are stopped this function will be called, with the SSR - * name passed as a parameter. + * This registers the @nb notifier block as part the notifier chain for a + * remoteproc associated with @name. The notifier block's callback + * will be invoked when the particular remote processor is stopped. */ -int qcom_register_ssr_notifier(struct notifier_block *nb) +void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb) { - return blocking_notifier_chain_register(&ssr_notifiers, nb); + struct rproc_notif_info *info; + + mutex_lock(&rproc_notif_lock); + info = find_notif_info(name); + if (!info) { + info = kzalloc(sizeof(*info), GFP_KERNEL); + if (!info) { + mutex_unlock(&rproc_notif_lock); + return ERR_PTR(-ENOMEM); + } + info->name = kstrndup(name, MAX_NAME_LEN, GFP_KERNEL); + srcu_init_notifier_head(&info->notifier_list); + + /* Add to global notif list */ + INIT_LIST_HEAD(&info->list); + list_add_tail(&info->list, &rproc_notif_list); + } + + srcu_notifier_chain_register(&info->notifier_list, nb); + mutex_unlock(&rproc_notif_lock); + return &info->notifier_list; } EXPORT_SYMBOL_GPL(qcom_register_ssr_notifier); /** * qcom_unregister_ssr_notifier() - unregister SSR notification handler + * @notify: pointer to srcu notifier head * @nb: notifier_block to unregister */ -void qcom_unregister_ssr_notifier(struct notifier_block *nb) +int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb) { - blocking_notifier_chain_unregister(&ssr_notifiers, nb); + if (!notify) + return -EINVAL; + + return srcu_notifier_chain_unregister(notify, nb); } EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier); static void ssr_notify_unprepare(struct rproc_subdev *subdev) { struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); + struct rproc_notif_data data = { + .name = ssr->info->name, + .crashed = false, + }; - blocking_notifier_call_chain(&ssr_notifiers, 0, (void *)ssr->name); + srcu_notifier_call_chain(&ssr->info->notifier_list, 0, &data); } + /** * qcom_add_ssr_subdev() - register subdevice as restart notification source * @rproc: rproc handle @@ -214,12 +258,30 @@ static void ssr_notify_unprepare(struct rproc_subdev *subdev) * @ssr_name: identifier to use for notifications originating from @rproc * * As the @ssr is registered with the @rproc SSR events will be sent to all - * registered listeners in the system as the remoteproc is shut down. + * registered listeners for the particular remoteproc when it is shutdown. */ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr, const char *ssr_name) { - ssr->name = ssr_name; + struct rproc_notif_info *info; + + mutex_lock(&rproc_notif_lock); + info = find_notif_info(ssr_name); + if (!info) { + info = kzalloc(sizeof(*info), GFP_KERNEL); + if (!info) { + mutex_unlock(&rproc_notif_lock); + return; + } + info->name = ssr_name; + srcu_init_notifier_head(&info->notifier_list); + + /* Add to global notif_list */ + INIT_LIST_HEAD(&info->list); + list_add_tail(&info->list, &rproc_notif_list); + } + mutex_unlock(&rproc_notif_lock); + ssr->info = info; ssr->subdev.unprepare = ssr_notify_unprepare; rproc_add_subdev(rproc, &ssr->subdev); @@ -233,6 +295,7 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr, */ void qcom_remove_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr) { + ssr->info = NULL; rproc_remove_subdev(rproc, &ssr->subdev); } EXPORT_SYMBOL_GPL(qcom_remove_ssr_subdev); diff --git a/drivers/remoteproc/qcom_common.h b/drivers/remoteproc/qcom_common.h index 58de71e..0c1d288 100644 --- a/drivers/remoteproc/qcom_common.h +++ b/drivers/remoteproc/qcom_common.h @@ -24,10 +24,16 @@ struct qcom_rproc_subdev { struct qcom_smd_edge *edge; }; +struct rproc_notif_info { + const char *name; + struct srcu_notifier_head notifier_list; + struct list_head list; +}; +static LIST_HEAD(rproc_notif_list); + struct qcom_rproc_ssr { struct rproc_subdev subdev; - - const char *name; + struct rproc_notif_info *info; }; void qcom_add_glink_subdev(struct rproc *rproc, struct qcom_rproc_glink *glink); diff --git a/include/linux/remoteproc/qcom_rproc.h b/include/linux/remoteproc/qcom_rproc.h index fa8e386..3dc65c0 100644 --- a/include/linux/remoteproc/qcom_rproc.h +++ b/include/linux/remoteproc/qcom_rproc.h @@ -5,17 +5,27 @@ #if IS_ENABLED(CONFIG_QCOM_RPROC_COMMON) -int qcom_register_ssr_notifier(struct notifier_block *nb); -void qcom_unregister_ssr_notifier(struct notifier_block *nb); +struct rproc_notif_data { + const char *name; + bool crashed; +}; + +void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb); +int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb); #else -static inline int qcom_register_ssr_notifier(struct notifier_block *nb) +static inline void *qcom_register_ssr_notifier(const char *name, + struct notifier_block *nb) { - return 0; + return NULL; } -static inline void qcom_unregister_ssr_notifier(struct notifier_block *nb) {} +static inline int qcom_unregister_ssr_notifier(void *notify, + struct notifier_block *nb) +{ + return 0; +} #endif -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] remoteproc: qcom: Add per subsystem SSR notification 2020-04-28 22:16 ` [PATCH v3 1/2] remoteproc: qcom: Add per subsystem SSR notification Rishabh Bhatnagar @ 2020-04-29 6:15 ` kbuild test robot 2020-05-19 20:38 ` Bjorn Andersson 1 sibling, 0 replies; 7+ messages in thread From: kbuild test robot @ 2020-04-29 6:15 UTC (permalink / raw) To: Rishabh Bhatnagar, linux-remoteproc, bjorn.andersson Cc: kbuild-all, ohad, linux-kernel, tsoni, psodagud, sidgup, Rishabh Bhatnagar [-- Attachment #1: Type: text/plain, Size: 5476 bytes --] Hi Rishabh, Thank you for the patch! Yet something to improve: [auto build test ERROR on linus/master] [also build test ERROR on v5.7-rc3 next-20200428] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Rishabh-Bhatnagar/Extend-SSR-notifications-framework/20200429-071958 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 96c9a7802af7d500a582d89a8b864584fe878c1b config: arm64-defconfig (attached as .config) compiler: aarch64-linux-gcc (GCC) 9.3.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=arm64 If you fix the issue, kindly add following tag as appropriate Reported-by: kbuild test robot <lkp@intel.com> All error/warnings (new ones prefixed by >>): drivers/soc/qcom/glink_ssr.c: In function 'qcom_glink_ssr_probe': >> drivers/soc/qcom/glink_ssr.c:128:36: error: passing argument 1 of 'qcom_register_ssr_notifier' from incompatible pointer type [-Werror=incompatible-pointer-types] 128 | return qcom_register_ssr_notifier(&ssr->nb); | ^~~~~~~~ | | | struct notifier_block * In file included from drivers/soc/qcom/glink_ssr.c:11: include/linux/remoteproc/qcom_rproc.h:13:7: note: expected 'const char *' but argument is of type 'struct notifier_block *' 13 | void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb); | ^~~~~~~~~~~~~~~~~~~~~~~~~~ >> drivers/soc/qcom/glink_ssr.c:128:9: error: too few arguments to function 'qcom_register_ssr_notifier' 128 | return qcom_register_ssr_notifier(&ssr->nb); | ^~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from drivers/soc/qcom/glink_ssr.c:11: include/linux/remoteproc/qcom_rproc.h:13:7: note: declared here 13 | void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb); | ^~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/soc/qcom/glink_ssr.c: In function 'qcom_glink_ssr_remove': >> drivers/soc/qcom/glink_ssr.c:135:2: error: too few arguments to function 'qcom_unregister_ssr_notifier' 135 | qcom_unregister_ssr_notifier(&ssr->nb); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from drivers/soc/qcom/glink_ssr.c:11: include/linux/remoteproc/qcom_rproc.h:14:5: note: declared here 14 | int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/soc/qcom/glink_ssr.c: In function 'qcom_glink_ssr_probe': >> drivers/soc/qcom/glink_ssr.c:129:1: warning: control reaches end of non-void function [-Wreturn-type] 129 | } | ^ cc1: some warnings being treated as errors vim +/qcom_register_ssr_notifier +128 drivers/soc/qcom/glink_ssr.c c4d77d5fcd8bae Bjorn Andersson 2017-07-24 111 c4d77d5fcd8bae Bjorn Andersson 2017-07-24 112 static int qcom_glink_ssr_probe(struct rpmsg_device *rpdev) c4d77d5fcd8bae Bjorn Andersson 2017-07-24 113 { c4d77d5fcd8bae Bjorn Andersson 2017-07-24 114 struct glink_ssr *ssr; c4d77d5fcd8bae Bjorn Andersson 2017-07-24 115 c4d77d5fcd8bae Bjorn Andersson 2017-07-24 116 ssr = devm_kzalloc(&rpdev->dev, sizeof(*ssr), GFP_KERNEL); c4d77d5fcd8bae Bjorn Andersson 2017-07-24 117 if (!ssr) c4d77d5fcd8bae Bjorn Andersson 2017-07-24 118 return -ENOMEM; c4d77d5fcd8bae Bjorn Andersson 2017-07-24 119 c4d77d5fcd8bae Bjorn Andersson 2017-07-24 120 init_completion(&ssr->completion); c4d77d5fcd8bae Bjorn Andersson 2017-07-24 121 c4d77d5fcd8bae Bjorn Andersson 2017-07-24 122 ssr->dev = &rpdev->dev; c4d77d5fcd8bae Bjorn Andersson 2017-07-24 123 ssr->ept = rpdev->ept; c4d77d5fcd8bae Bjorn Andersson 2017-07-24 124 ssr->nb.notifier_call = qcom_glink_ssr_notify; c4d77d5fcd8bae Bjorn Andersson 2017-07-24 125 c4d77d5fcd8bae Bjorn Andersson 2017-07-24 126 dev_set_drvdata(&rpdev->dev, ssr); c4d77d5fcd8bae Bjorn Andersson 2017-07-24 127 c4d77d5fcd8bae Bjorn Andersson 2017-07-24 @128 return qcom_register_ssr_notifier(&ssr->nb); c4d77d5fcd8bae Bjorn Andersson 2017-07-24 @129 } c4d77d5fcd8bae Bjorn Andersson 2017-07-24 130 c4d77d5fcd8bae Bjorn Andersson 2017-07-24 131 static void qcom_glink_ssr_remove(struct rpmsg_device *rpdev) c4d77d5fcd8bae Bjorn Andersson 2017-07-24 132 { c4d77d5fcd8bae Bjorn Andersson 2017-07-24 133 struct glink_ssr *ssr = dev_get_drvdata(&rpdev->dev); c4d77d5fcd8bae Bjorn Andersson 2017-07-24 134 c4d77d5fcd8bae Bjorn Andersson 2017-07-24 @135 qcom_unregister_ssr_notifier(&ssr->nb); c4d77d5fcd8bae Bjorn Andersson 2017-07-24 136 } c4d77d5fcd8bae Bjorn Andersson 2017-07-24 137 :::::: The code at line 128 was first introduced by commit :::::: c4d77d5fcd8baefb358ae921b940e9bbd09a751e soc: qcom: GLINK SSR notifier :::::: TO: Bjorn Andersson <bjorn.andersson@linaro.org> :::::: CC: Bjorn Andersson <bjorn.andersson@linaro.org> --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 48802 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] remoteproc: qcom: Add per subsystem SSR notification 2020-04-28 22:16 ` [PATCH v3 1/2] remoteproc: qcom: Add per subsystem SSR notification Rishabh Bhatnagar 2020-04-29 6:15 ` kbuild test robot @ 2020-05-19 20:38 ` Bjorn Andersson 2020-05-26 18:03 ` rishabhb 1 sibling, 1 reply; 7+ messages in thread From: Bjorn Andersson @ 2020-05-19 20:38 UTC (permalink / raw) To: Rishabh Bhatnagar Cc: linux-remoteproc, ohad, linux-kernel, tsoni, psodagud, sidgup On Tue 28 Apr 15:16 PDT 2020, Rishabh Bhatnagar wrote: > Currently there is a single notification chain which is called whenever any > remoteproc shuts down. This leads to all the listeners being notified, and > is not an optimal design as kernel drivers might only be interested in > listening to notifications from a particular remoteproc. Create a global > list of remoteproc notification info data structures. This will hold the > name and notifier_list information for a particular remoteproc. The API > to register for notifications will use name argument to retrieve the > notification info data structure and the notifier block will be added to > that data structure's notification chain. > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> > Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org> Thanks Rishabh, design wise I think this looks good now, just some code style things below. > --- > drivers/remoteproc/qcom_common.c | 89 ++++++++++++++++++++++++++++++----- > drivers/remoteproc/qcom_common.h | 10 +++- > include/linux/remoteproc/qcom_rproc.h | 20 ++++++-- > 3 files changed, 99 insertions(+), 20 deletions(-) > > diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c > index 60650bc..7cd17be 100644 > --- a/drivers/remoteproc/qcom_common.c > +++ b/drivers/remoteproc/qcom_common.c > @@ -15,16 +15,18 @@ > #include <linux/rpmsg/qcom_glink.h> > #include <linux/rpmsg/qcom_smd.h> > #include <linux/soc/qcom/mdt_loader.h> > +#include <linux/remoteproc/qcom_rproc.h> Please maintain alphabetical sort order. > > #include "remoteproc_internal.h" > #include "qcom_common.h" > > +#define MAX_NAME_LEN 20 > +DEFINE_MUTEX(rproc_notif_lock); Please rename this qcom_ssr_subsystem_lock > + > #define to_glink_subdev(d) container_of(d, struct qcom_rproc_glink, subdev) > #define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, subdev) > #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, subdev) > > -static BLOCKING_NOTIFIER_HEAD(ssr_notifiers); Move the definition of rproc_notif_info, the new rproc_notif_info list head and move the two lines above here as well. > - > static int glink_subdev_start(struct rproc_subdev *subdev) > { > struct qcom_rproc_glink *glink = to_glink_subdev(subdev); > @@ -174,39 +176,81 @@ void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd) > } > EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev); > > +struct rproc_notif_info *find_notif_info(const char *name) Please make this qcom_ssr_get_subsystem(const char *name) > +{ > + struct rproc_notif_info *info; > + > + /* Match in the global rproc_notif_list with name */ > + list_for_each_entry(info, &rproc_notif_list, list) { > + if (!strncmp(info->name, name, strlen(name))) strncmp(a, b, strlen(b)) is the same thing as strcmp(a, b), unless a is shorted than b and not NUL terminated. > + return info; > + } > + return NULL; Both callers of this function will if NULL is returned allocate a new subsystem object and attach to the list. If you do that here you can remove the duplication between these. > +} > + > /** > * qcom_register_ssr_notifier() - register SSR notification handler > + * @name: pointer to name which will be searched in the global notif_list > * @nb: notifier_block to notify for restart notifications > * > - * Returns 0 on success, negative errno on failure. > + * Returns pointer to srcu notifier head on success, ERR_PTR on failure. This shouldn't mention that the opaque pointer is of a type standard to the kernel. Better just say that it returns a "subsystem cookie". > * > - * This register the @notify function as handler for restart notifications. As > - * remote processors are stopped this function will be called, with the SSR > - * name passed as a parameter. > + * This registers the @nb notifier block as part the notifier chain for a > + * remoteproc associated with @name. The notifier block's callback > + * will be invoked when the particular remote processor is stopped. > */ > -int qcom_register_ssr_notifier(struct notifier_block *nb) > +void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb) > { > - return blocking_notifier_chain_register(&ssr_notifiers, nb); > + struct rproc_notif_info *info; > + > + mutex_lock(&rproc_notif_lock); > + info = find_notif_info(name); > + if (!info) { > + info = kzalloc(sizeof(*info), GFP_KERNEL); > + if (!info) { > + mutex_unlock(&rproc_notif_lock); > + return ERR_PTR(-ENOMEM); > + } > + info->name = kstrndup(name, MAX_NAME_LEN, GFP_KERNEL); This is going to be a constant in a lot of cases, so please use kstrdup_const(). Also what's the purpose of limiting the length of this? > + srcu_init_notifier_head(&info->notifier_list); > + > + /* Add to global notif list */ > + INIT_LIST_HEAD(&info->list); > + list_add_tail(&info->list, &rproc_notif_list); > + } > + > + srcu_notifier_chain_register(&info->notifier_list, nb); > + mutex_unlock(&rproc_notif_lock); > + return &info->notifier_list; > } > EXPORT_SYMBOL_GPL(qcom_register_ssr_notifier); > > /** > * qcom_unregister_ssr_notifier() - unregister SSR notification handler > + * @notify: pointer to srcu notifier head @subsystem: subsystem cookie returned from qcom_register_ssr_notifier > * @nb: notifier_block to unregister > */ > -void qcom_unregister_ssr_notifier(struct notifier_block *nb) > +int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb) > { > - blocking_notifier_chain_unregister(&ssr_notifiers, nb); > + if (!notify) > + return -EINVAL; qcom_register_ssr_notifier() will return a valid cookie or a ERR_PTR() so if someone passes NULL here they did something wrong during development... So it's better to just remove this check and give the developer a nice callstack directly pointing out their mistake, than forcing them to chase where this -EINVAL comes from. > + > + return srcu_notifier_chain_unregister(notify, nb); > } > EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier); > > static void ssr_notify_unprepare(struct rproc_subdev *subdev) > { > struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > + struct rproc_notif_data data = { > + .name = ssr->info->name, > + .crashed = false, > + }; > > - blocking_notifier_call_chain(&ssr_notifiers, 0, (void *)ssr->name); > + srcu_notifier_call_chain(&ssr->info->notifier_list, 0, &data); Did we conclude on why you change blocking to srcu? Can we do it in a separate patch? > } > > + > /** > * qcom_add_ssr_subdev() - register subdevice as restart notification source > * @rproc: rproc handle > @@ -214,12 +258,30 @@ static void ssr_notify_unprepare(struct rproc_subdev *subdev) > * @ssr_name: identifier to use for notifications originating from @rproc > * > * As the @ssr is registered with the @rproc SSR events will be sent to all > - * registered listeners in the system as the remoteproc is shut down. > + * registered listeners for the particular remoteproc when it is shutdown. > */ > void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr, > const char *ssr_name) > { > - ssr->name = ssr_name; > + struct rproc_notif_info *info; > + > + mutex_lock(&rproc_notif_lock); > + info = find_notif_info(ssr_name); > + if (!info) { > + info = kzalloc(sizeof(*info), GFP_KERNEL); > + if (!info) { > + mutex_unlock(&rproc_notif_lock); > + return; > + } > + info->name = ssr_name; > + srcu_init_notifier_head(&info->notifier_list); > + > + /* Add to global notif_list */ > + INIT_LIST_HEAD(&info->list); > + list_add_tail(&info->list, &rproc_notif_list); > + } > + mutex_unlock(&rproc_notif_lock); > + ssr->info = info; > ssr->subdev.unprepare = ssr_notify_unprepare; > > rproc_add_subdev(rproc, &ssr->subdev); > @@ -233,6 +295,7 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr, > */ > void qcom_remove_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr) > { > + ssr->info = NULL; Move this after rproc_remove_subdev() and rely on the core for this not to race with the ssr_notify_unprepare(). > rproc_remove_subdev(rproc, &ssr->subdev); > } > EXPORT_SYMBOL_GPL(qcom_remove_ssr_subdev); It would be nice with a module_exit() that walks the rproc_notif_list and free all the elements, if qcom_common.ko is rmmod'ed. Given that this is uncommon I wouldn't mind to take that as a separate patch though. > diff --git a/drivers/remoteproc/qcom_common.h b/drivers/remoteproc/qcom_common.h > index 58de71e..0c1d288 100644 > --- a/drivers/remoteproc/qcom_common.h > +++ b/drivers/remoteproc/qcom_common.h > @@ -24,10 +24,16 @@ struct qcom_rproc_subdev { > struct qcom_smd_edge *edge; > }; > > +struct rproc_notif_info { Please rename this struct qcom_ssr_subsystem > + const char *name; > + struct srcu_notifier_head notifier_list; > + struct list_head list; > +}; > +static LIST_HEAD(rproc_notif_list); Please rename this list qcom_ssr_subsystem_list and as stated above move it into qcom_common.c. To allow using qcom_ssr_subsystem in the struct below simply forward declare it here as: struct qcom_ssr_subsystem; > + > struct qcom_rproc_ssr { > struct rproc_subdev subdev; > - > - const char *name; > + struct rproc_notif_info *info; > }; > Regards, Bjorn > void qcom_add_glink_subdev(struct rproc *rproc, struct qcom_rproc_glink *glink); > diff --git a/include/linux/remoteproc/qcom_rproc.h b/include/linux/remoteproc/qcom_rproc.h > index fa8e386..3dc65c0 100644 > --- a/include/linux/remoteproc/qcom_rproc.h > +++ b/include/linux/remoteproc/qcom_rproc.h > @@ -5,17 +5,27 @@ > > #if IS_ENABLED(CONFIG_QCOM_RPROC_COMMON) > > -int qcom_register_ssr_notifier(struct notifier_block *nb); > -void qcom_unregister_ssr_notifier(struct notifier_block *nb); > +struct rproc_notif_data { > + const char *name; > + bool crashed; > +}; > + > +void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb); > +int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb); > > #else > > -static inline int qcom_register_ssr_notifier(struct notifier_block *nb) > +static inline void *qcom_register_ssr_notifier(const char *name, > + struct notifier_block *nb) > { > - return 0; > + return NULL; > } > > -static inline void qcom_unregister_ssr_notifier(struct notifier_block *nb) {} > +static inline int qcom_unregister_ssr_notifier(void *notify, > + struct notifier_block *nb) > +{ > + return 0; > +} > > #endif > > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] remoteproc: qcom: Add per subsystem SSR notification 2020-05-19 20:38 ` Bjorn Andersson @ 2020-05-26 18:03 ` rishabhb 0 siblings, 0 replies; 7+ messages in thread From: rishabhb @ 2020-05-26 18:03 UTC (permalink / raw) To: Bjorn Andersson Cc: linux-remoteproc, ohad, linux-kernel, tsoni, psodagud, sidgup On 2020-05-19 13:38, Bjorn Andersson wrote: > On Tue 28 Apr 15:16 PDT 2020, Rishabh Bhatnagar wrote: > >> Currently there is a single notification chain which is called >> whenever any >> remoteproc shuts down. This leads to all the listeners being notified, >> and >> is not an optimal design as kernel drivers might only be interested in >> listening to notifications from a particular remoteproc. Create a >> global >> list of remoteproc notification info data structures. This will hold >> the >> name and notifier_list information for a particular remoteproc. The >> API >> to register for notifications will use name argument to retrieve the >> notification info data structure and the notifier block will be added >> to >> that data structure's notification chain. >> >> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> >> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org> > > Thanks Rishabh, design wise I think this looks good now, just some code > style things below. > >> --- >> drivers/remoteproc/qcom_common.c | 89 >> ++++++++++++++++++++++++++++++----- >> drivers/remoteproc/qcom_common.h | 10 +++- >> include/linux/remoteproc/qcom_rproc.h | 20 ++++++-- >> 3 files changed, 99 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/remoteproc/qcom_common.c >> b/drivers/remoteproc/qcom_common.c >> index 60650bc..7cd17be 100644 >> --- a/drivers/remoteproc/qcom_common.c >> +++ b/drivers/remoteproc/qcom_common.c >> @@ -15,16 +15,18 @@ >> #include <linux/rpmsg/qcom_glink.h> >> #include <linux/rpmsg/qcom_smd.h> >> #include <linux/soc/qcom/mdt_loader.h> >> +#include <linux/remoteproc/qcom_rproc.h> > > Please maintain alphabetical sort order. > >> >> #include "remoteproc_internal.h" >> #include "qcom_common.h" >> >> +#define MAX_NAME_LEN 20 >> +DEFINE_MUTEX(rproc_notif_lock); > > Please rename this qcom_ssr_subsystem_lock > >> + >> #define to_glink_subdev(d) container_of(d, struct qcom_rproc_glink, >> subdev) >> #define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, >> subdev) >> #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, >> subdev) >> >> -static BLOCKING_NOTIFIER_HEAD(ssr_notifiers); > > Move the definition of rproc_notif_info, the new rproc_notif_info list > head and move the two lines above here as well. > >> - >> static int glink_subdev_start(struct rproc_subdev *subdev) >> { >> struct qcom_rproc_glink *glink = to_glink_subdev(subdev); >> @@ -174,39 +176,81 @@ void qcom_remove_smd_subdev(struct rproc *rproc, >> struct qcom_rproc_subdev *smd) >> } >> EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev); >> >> +struct rproc_notif_info *find_notif_info(const char *name) > > Please make this qcom_ssr_get_subsystem(const char *name) > >> +{ >> + struct rproc_notif_info *info; >> + >> + /* Match in the global rproc_notif_list with name */ >> + list_for_each_entry(info, &rproc_notif_list, list) { >> + if (!strncmp(info->name, name, strlen(name))) > > strncmp(a, b, strlen(b)) is the same thing as strcmp(a, b), unless a is > shorted than b and not NUL terminated. > >> + return info; >> + } >> + return NULL; > > Both callers of this function will if NULL is returned allocate a new > subsystem object and attach to the list. If you do that here you can > remove the duplication between these. > >> +} >> + >> /** >> * qcom_register_ssr_notifier() - register SSR notification handler >> + * @name: pointer to name which will be searched in the global >> notif_list >> * @nb: notifier_block to notify for restart notifications >> * >> - * Returns 0 on success, negative errno on failure. >> + * Returns pointer to srcu notifier head on success, ERR_PTR on >> failure. > > This shouldn't mention that the opaque pointer is of a type standard to > the kernel. Better just say that it returns a "subsystem cookie". > >> * >> - * This register the @notify function as handler for restart >> notifications. As >> - * remote processors are stopped this function will be called, with >> the SSR >> - * name passed as a parameter. >> + * This registers the @nb notifier block as part the notifier chain >> for a >> + * remoteproc associated with @name. The notifier block's callback >> + * will be invoked when the particular remote processor is stopped. >> */ >> -int qcom_register_ssr_notifier(struct notifier_block *nb) >> +void *qcom_register_ssr_notifier(const char *name, struct >> notifier_block *nb) >> { >> - return blocking_notifier_chain_register(&ssr_notifiers, nb); >> + struct rproc_notif_info *info; >> + >> + mutex_lock(&rproc_notif_lock); >> + info = find_notif_info(name); >> + if (!info) { >> + info = kzalloc(sizeof(*info), GFP_KERNEL); >> + if (!info) { >> + mutex_unlock(&rproc_notif_lock); >> + return ERR_PTR(-ENOMEM); >> + } >> + info->name = kstrndup(name, MAX_NAME_LEN, GFP_KERNEL); > > This is going to be a constant in a lot of cases, so please use > kstrdup_const(). Also what's the purpose of limiting the length of > this? > >> + srcu_init_notifier_head(&info->notifier_list); >> + >> + /* Add to global notif list */ >> + INIT_LIST_HEAD(&info->list); >> + list_add_tail(&info->list, &rproc_notif_list); >> + } >> + >> + srcu_notifier_chain_register(&info->notifier_list, nb); >> + mutex_unlock(&rproc_notif_lock); >> + return &info->notifier_list; >> } >> EXPORT_SYMBOL_GPL(qcom_register_ssr_notifier); >> >> /** >> * qcom_unregister_ssr_notifier() - unregister SSR notification >> handler >> + * @notify: pointer to srcu notifier head > > @subsystem: subsystem cookie returned from > qcom_register_ssr_notifier > >> * @nb: notifier_block to unregister >> */ >> -void qcom_unregister_ssr_notifier(struct notifier_block *nb) >> +int qcom_unregister_ssr_notifier(void *notify, struct notifier_block >> *nb) >> { >> - blocking_notifier_chain_unregister(&ssr_notifiers, nb); >> + if (!notify) >> + return -EINVAL; > > qcom_register_ssr_notifier() will return a valid cookie or a ERR_PTR() > so if someone passes NULL here they did something wrong during > development... > > So it's better to just remove this check and give the developer a nice > callstack directly pointing out their mistake, than forcing them to > chase where this -EINVAL comes from. > >> + >> + return srcu_notifier_chain_unregister(notify, nb); >> } >> EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier); >> >> static void ssr_notify_unprepare(struct rproc_subdev *subdev) >> { >> struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); >> + struct rproc_notif_data data = { >> + .name = ssr->info->name, >> + .crashed = false, >> + }; >> >> - blocking_notifier_call_chain(&ssr_notifiers, 0, (void *)ssr->name); >> + srcu_notifier_call_chain(&ssr->info->notifier_list, 0, &data); > > Did we conclude on why you change blocking to srcu? Can we do it in a > separate patch? > Since the notifier list needs to be dynamically added for every subsystem using srcu notifier makes sense. I'm not sure if we can use blocking notifier dynamically here. >> } >> >> + >> /** >> * qcom_add_ssr_subdev() - register subdevice as restart notification >> source >> * @rproc: rproc handle >> @@ -214,12 +258,30 @@ static void ssr_notify_unprepare(struct >> rproc_subdev *subdev) >> * @ssr_name: identifier to use for notifications originating from >> @rproc >> * >> * As the @ssr is registered with the @rproc SSR events will be sent >> to all >> - * registered listeners in the system as the remoteproc is shut down. >> + * registered listeners for the particular remoteproc when it is >> shutdown. >> */ >> void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr >> *ssr, >> const char *ssr_name) >> { >> - ssr->name = ssr_name; >> + struct rproc_notif_info *info; >> + >> + mutex_lock(&rproc_notif_lock); >> + info = find_notif_info(ssr_name); >> + if (!info) { >> + info = kzalloc(sizeof(*info), GFP_KERNEL); >> + if (!info) { >> + mutex_unlock(&rproc_notif_lock); >> + return; >> + } >> + info->name = ssr_name; >> + srcu_init_notifier_head(&info->notifier_list); >> + >> + /* Add to global notif_list */ >> + INIT_LIST_HEAD(&info->list); >> + list_add_tail(&info->list, &rproc_notif_list); >> + } >> + mutex_unlock(&rproc_notif_lock); >> + ssr->info = info; >> ssr->subdev.unprepare = ssr_notify_unprepare; >> >> rproc_add_subdev(rproc, &ssr->subdev); >> @@ -233,6 +295,7 @@ void qcom_add_ssr_subdev(struct rproc *rproc, >> struct qcom_rproc_ssr *ssr, >> */ >> void qcom_remove_ssr_subdev(struct rproc *rproc, struct >> qcom_rproc_ssr *ssr) >> { >> + ssr->info = NULL; > > Move this after rproc_remove_subdev() and rely on the core for this not > to race with the ssr_notify_unprepare(). > >> rproc_remove_subdev(rproc, &ssr->subdev); >> } >> EXPORT_SYMBOL_GPL(qcom_remove_ssr_subdev); > > It would be nice with a module_exit() that walks the rproc_notif_list > and free all the elements, if qcom_common.ko is rmmod'ed. Given that > this is uncommon I wouldn't mind to take that as a separate patch > though. > Yes i can submit that as a separate patch. >> diff --git a/drivers/remoteproc/qcom_common.h >> b/drivers/remoteproc/qcom_common.h >> index 58de71e..0c1d288 100644 >> --- a/drivers/remoteproc/qcom_common.h >> +++ b/drivers/remoteproc/qcom_common.h >> @@ -24,10 +24,16 @@ struct qcom_rproc_subdev { >> struct qcom_smd_edge *edge; >> }; >> >> +struct rproc_notif_info { > > Please rename this struct qcom_ssr_subsystem > >> + const char *name; >> + struct srcu_notifier_head notifier_list; >> + struct list_head list; >> +}; >> +static LIST_HEAD(rproc_notif_list); > > Please rename this list qcom_ssr_subsystem_list and as stated above > move > it into qcom_common.c. > > > To allow using qcom_ssr_subsystem in the struct below simply forward > declare it here as: > > struct qcom_ssr_subsystem; > >> + >> struct qcom_rproc_ssr { >> struct rproc_subdev subdev; >> - >> - const char *name; >> + struct rproc_notif_info *info; >> }; >> > > Regards, > Bjorn > >> void qcom_add_glink_subdev(struct rproc *rproc, struct >> qcom_rproc_glink *glink); >> diff --git a/include/linux/remoteproc/qcom_rproc.h >> b/include/linux/remoteproc/qcom_rproc.h >> index fa8e386..3dc65c0 100644 >> --- a/include/linux/remoteproc/qcom_rproc.h >> +++ b/include/linux/remoteproc/qcom_rproc.h >> @@ -5,17 +5,27 @@ >> >> #if IS_ENABLED(CONFIG_QCOM_RPROC_COMMON) >> >> -int qcom_register_ssr_notifier(struct notifier_block *nb); >> -void qcom_unregister_ssr_notifier(struct notifier_block *nb); >> +struct rproc_notif_data { >> + const char *name; >> + bool crashed; >> +}; >> + >> +void *qcom_register_ssr_notifier(const char *name, struct >> notifier_block *nb); >> +int qcom_unregister_ssr_notifier(void *notify, struct notifier_block >> *nb); >> >> #else >> >> -static inline int qcom_register_ssr_notifier(struct notifier_block >> *nb) >> +static inline void *qcom_register_ssr_notifier(const char *name, >> + struct notifier_block *nb) >> { >> - return 0; >> + return NULL; >> } >> >> -static inline void qcom_unregister_ssr_notifier(struct notifier_block >> *nb) {} >> +static inline int qcom_unregister_ssr_notifier(void *notify, >> + struct notifier_block *nb) >> +{ >> + return 0; >> +} >> >> #endif >> >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora >> Forum, >> a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 2/2] remoteproc: qcom: Add notification types to SSR 2020-04-28 22:16 [PATCH v3 0/2] Extend SSR notifications framework Rishabh Bhatnagar 2020-04-28 22:16 ` [PATCH v3 1/2] remoteproc: qcom: Add per subsystem SSR notification Rishabh Bhatnagar @ 2020-04-28 22:16 ` Rishabh Bhatnagar 2020-05-19 20:40 ` Bjorn Andersson 1 sibling, 1 reply; 7+ messages in thread From: Rishabh Bhatnagar @ 2020-04-28 22:16 UTC (permalink / raw) To: linux-remoteproc, bjorn.andersson Cc: ohad, linux-kernel, tsoni, psodagud, sidgup, Rishabh Bhatnagar From: Siddharth Gupta <sidgup@codeaurora.org> The SSR subdevice only adds callback for the unprepare event. Add callbacks for unprepare, start and prepare events. The client driver for a particular remoteproc might be interested in knowing the status of the remoteproc while undergoing SSR, not just when the remoteproc has finished shutting down. Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org> --- drivers/remoteproc/qcom_common.c | 46 +++++++++++++++++++++++++++++++++-- include/linux/remoteproc/qcom_rproc.h | 14 +++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c index 7cd17be..0d91cf3 100644 --- a/drivers/remoteproc/qcom_common.c +++ b/drivers/remoteproc/qcom_common.c @@ -197,7 +197,7 @@ struct rproc_notif_info *find_notif_info(const char *name) * * This registers the @nb notifier block as part the notifier chain for a * remoteproc associated with @name. The notifier block's callback - * will be invoked when the particular remote processor is stopped. + * will be invoked when the particular remote processor is started/stopped. */ void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb) { @@ -239,6 +239,44 @@ int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb) } EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier); +static int ssr_notify_prepare(struct rproc_subdev *subdev) +{ + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); + struct rproc_notif_data data = { + .name = ssr->info->name, + .crashed = false, + }; + + srcu_notifier_call_chain(&ssr->info->notifier_list, + RPROC_BEFORE_POWERUP, &data); + return 0; +} + +static int ssr_notify_start(struct rproc_subdev *subdev) +{ + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); + struct rproc_notif_data data = { + .name = ssr->info->name, + .crashed = false, + }; + + srcu_notifier_call_chain(&ssr->info->notifier_list, + RPROC_AFTER_POWERUP, &data); + return 0; +} + +static void ssr_notify_stop(struct rproc_subdev *subdev, bool crashed) +{ + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); + struct rproc_notif_data data = { + .name = ssr->info->name, + .crashed = crashed, + }; + + srcu_notifier_call_chain(&ssr->info->notifier_list, + RPROC_BEFORE_SHUTDOWN, &data); +} + static void ssr_notify_unprepare(struct rproc_subdev *subdev) { struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); @@ -247,7 +285,8 @@ static void ssr_notify_unprepare(struct rproc_subdev *subdev) .crashed = false, }; - srcu_notifier_call_chain(&ssr->info->notifier_list, 0, &data); + srcu_notifier_call_chain(&ssr->info->notifier_list, + RPROC_AFTER_SHUTDOWN, &data); } @@ -282,6 +321,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr, } mutex_unlock(&rproc_notif_lock); ssr->info = info; + ssr->subdev.prepare = ssr_notify_prepare; + ssr->subdev.start = ssr_notify_start; + ssr->subdev.stop = ssr_notify_stop; ssr->subdev.unprepare = ssr_notify_unprepare; rproc_add_subdev(rproc, &ssr->subdev); diff --git a/include/linux/remoteproc/qcom_rproc.h b/include/linux/remoteproc/qcom_rproc.h index 3dc65c0..567c1f9 100644 --- a/include/linux/remoteproc/qcom_rproc.h +++ b/include/linux/remoteproc/qcom_rproc.h @@ -5,6 +5,20 @@ #if IS_ENABLED(CONFIG_QCOM_RPROC_COMMON) +/** + * enum rproc_notif_type - Different stages of remoteproc notifications + * @RPROC_BEFORE_SHUTDOWN: unprepare stage of remoteproc + * @RPROC_AFTER_SHUTDOWN: stop stage of remoteproc + * @RPROC_BEFORE_POWERUP: prepare stage of remoteproc + * @RPROC_AFTER_POWERUP: start stage of remoteproc + */ +enum rproc_notif_type { + RPROC_BEFORE_SHUTDOWN, + RPROC_AFTER_SHUTDOWN, + RPROC_BEFORE_POWERUP, + RPROC_AFTER_POWERUP, +}; + struct rproc_notif_data { const char *name; bool crashed; -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] remoteproc: qcom: Add notification types to SSR 2020-04-28 22:16 ` [PATCH v3 2/2] remoteproc: qcom: Add notification types to SSR Rishabh Bhatnagar @ 2020-05-19 20:40 ` Bjorn Andersson 0 siblings, 0 replies; 7+ messages in thread From: Bjorn Andersson @ 2020-05-19 20:40 UTC (permalink / raw) To: Rishabh Bhatnagar Cc: linux-remoteproc, ohad, linux-kernel, tsoni, psodagud, sidgup On Tue 28 Apr 15:16 PDT 2020, Rishabh Bhatnagar wrote: > From: Siddharth Gupta <sidgup@codeaurora.org> > > The SSR subdevice only adds callback for the unprepare event. Add callbacks > for unprepare, start and prepare events. The client driver for a particular > remoteproc might be interested in knowing the status of the remoteproc > while undergoing SSR, not just when the remoteproc has finished shutting > down. > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> > Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org> > --- > drivers/remoteproc/qcom_common.c | 46 +++++++++++++++++++++++++++++++++-- > include/linux/remoteproc/qcom_rproc.h | 14 +++++++++++ > 2 files changed, 58 insertions(+), 2 deletions(-) > > diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c > index 7cd17be..0d91cf3 100644 > --- a/drivers/remoteproc/qcom_common.c > +++ b/drivers/remoteproc/qcom_common.c > @@ -197,7 +197,7 @@ struct rproc_notif_info *find_notif_info(const char *name) > * > * This registers the @nb notifier block as part the notifier chain for a > * remoteproc associated with @name. The notifier block's callback > - * will be invoked when the particular remote processor is stopped. > + * will be invoked when the particular remote processor is started/stopped. > */ > void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb) > { > @@ -239,6 +239,44 @@ int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb) > } > EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier); > > +static int ssr_notify_prepare(struct rproc_subdev *subdev) > +{ > + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > + struct rproc_notif_data data = { > + .name = ssr->info->name, > + .crashed = false, > + }; > + > + srcu_notifier_call_chain(&ssr->info->notifier_list, > + RPROC_BEFORE_POWERUP, &data); > + return 0; > +} > + > +static int ssr_notify_start(struct rproc_subdev *subdev) > +{ > + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > + struct rproc_notif_data data = { > + .name = ssr->info->name, > + .crashed = false, > + }; > + > + srcu_notifier_call_chain(&ssr->info->notifier_list, > + RPROC_AFTER_POWERUP, &data); > + return 0; > +} > + > +static void ssr_notify_stop(struct rproc_subdev *subdev, bool crashed) > +{ > + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > + struct rproc_notif_data data = { > + .name = ssr->info->name, > + .crashed = crashed, > + }; > + > + srcu_notifier_call_chain(&ssr->info->notifier_list, > + RPROC_BEFORE_SHUTDOWN, &data); > +} > + > static void ssr_notify_unprepare(struct rproc_subdev *subdev) > { > struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > @@ -247,7 +285,8 @@ static void ssr_notify_unprepare(struct rproc_subdev *subdev) > .crashed = false, > }; > > - srcu_notifier_call_chain(&ssr->info->notifier_list, 0, &data); > + srcu_notifier_call_chain(&ssr->info->notifier_list, > + RPROC_AFTER_SHUTDOWN, &data); > } > > > @@ -282,6 +321,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr, > } > mutex_unlock(&rproc_notif_lock); > ssr->info = info; > + ssr->subdev.prepare = ssr_notify_prepare; > + ssr->subdev.start = ssr_notify_start; > + ssr->subdev.stop = ssr_notify_stop; > ssr->subdev.unprepare = ssr_notify_unprepare; > > rproc_add_subdev(rproc, &ssr->subdev); > diff --git a/include/linux/remoteproc/qcom_rproc.h b/include/linux/remoteproc/qcom_rproc.h > index 3dc65c0..567c1f9 100644 > --- a/include/linux/remoteproc/qcom_rproc.h > +++ b/include/linux/remoteproc/qcom_rproc.h > @@ -5,6 +5,20 @@ > > #if IS_ENABLED(CONFIG_QCOM_RPROC_COMMON) > > +/** > + * enum rproc_notif_type - Different stages of remoteproc notifications > + * @RPROC_BEFORE_SHUTDOWN: unprepare stage of remoteproc > + * @RPROC_AFTER_SHUTDOWN: stop stage of remoteproc > + * @RPROC_BEFORE_POWERUP: prepare stage of remoteproc > + * @RPROC_AFTER_POWERUP: start stage of remoteproc > + */ > +enum rproc_notif_type { > + RPROC_BEFORE_SHUTDOWN, > + RPROC_AFTER_SHUTDOWN, > + RPROC_BEFORE_POWERUP, > + RPROC_AFTER_POWERUP, Given that these are not generic remoteproc things I would like a qcom prefix on them. How about QCOM_SSR_BEFORE_SHUTDOWN ... Apart from that, this looks good. Thanks, Bjorn > +}; > + > struct rproc_notif_data { > const char *name; > bool crashed; > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-05-26 18:04 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-28 22:16 [PATCH v3 0/2] Extend SSR notifications framework Rishabh Bhatnagar 2020-04-28 22:16 ` [PATCH v3 1/2] remoteproc: qcom: Add per subsystem SSR notification Rishabh Bhatnagar 2020-04-29 6:15 ` kbuild test robot 2020-05-19 20:38 ` Bjorn Andersson 2020-05-26 18:03 ` rishabhb 2020-04-28 22:16 ` [PATCH v3 2/2] remoteproc: qcom: Add notification types to SSR Rishabh Bhatnagar 2020-05-19 20:40 ` Bjorn Andersson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).