All of lore.kernel.org
 help / color / mirror / Atom feed
From: rishabhb@codeaurora.org
To: Alex Elder <elder@ieee.org>
Cc: linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org,
	bjorn.andersson@linaro.org, tsoni@codeaurora.org,
	psodagud@codeaurora.org, sidgup@codeaurora.org,
	linux-remoteproc-owner@vger.kernel.org
Subject: Re: [PATCH v5 1/2] remoteproc: qcom: Add per subsystem SSR notification
Date: Tue, 23 Jun 2020 18:41:37 -0700	[thread overview]
Message-ID: <cb31dfb50079a1377cf27807a7b2eb3e@codeaurora.org> (raw)
In-Reply-To: <8ed72321-6f6a-1083-9af9-a80aa945edeb@ieee.org>

On 2020-06-23 14:45, Alex Elder wrote:
> On 6/22/20 8:04 PM, Rishabh Bhatnagar wrote:
>> Currently there is a single notification chain which is called 
>> whenever any
>> remoteproc shuts down. This leads to all the listeners being notified, 
>> and
>> is not an optimal design as kernel drivers might only be interested in
>> listening to notifications from a particular remoteproc. Create a 
>> global
>> list of remoteproc notification info data structures. This will hold 
>> the
>> name and notifier_list information for a particular remoteproc. The 
>> API
>> to register for notifications will use name argument to retrieve the
>> notification info data structure and the notifier block will be added 
>> to
>> that data structure's notification chain. Also move from blocking 
>> notifier
>> to srcu notifer based implementation to support dynamic notifier head
>> creation.
>> 
>> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
>> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> 
> Sorry, a few more comments, but I think your next one will
> likely be fine.
> 
> General:
> - SSR subsystems are added but never removed.  Note that
>   "qcom_common.o" can be built as a module, and if that
>   module were ever removed, memory allocated for these
>   subsystems would be leaked.
Hi Alex,
Thank you for reviewing this patchset quickly. This point was
brought up by Bjorn and it was decided that I will push another patch on
top in which I'll do the cleanup during module exit.
> - Will a remoteproc subdev (and in particular, an SSR subdev)
>   ever be removed?  What happens to entities that have
>   registered for SSR notifications in that case?
In practice it should never be removed. If it is clients will
never get notification about subsystem shutdown/powerup.
> 
> (Maybe these are issues that won't/can't occur in practice?)
> 
>> ---
>>   drivers/remoteproc/qcom_common.c      | 86 
>> +++++++++++++++++++++++++++++------
>>   drivers/remoteproc/qcom_common.h      |  5 +-
>>   include/linux/remoteproc/qcom_rproc.h | 20 ++++++--
>>   3 files changed, 91 insertions(+), 20 deletions(-)
>> 
>> diff --git a/drivers/remoteproc/qcom_common.c 
>> b/drivers/remoteproc/qcom_common.c
>> index 9028cea..658f2ca 100644
>> --- a/drivers/remoteproc/qcom_common.c
>> +++ b/drivers/remoteproc/qcom_common.c
>> @@ -12,6 +12,7 @@
>>   #include <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);
>> +static DEFINE_MUTEX(qcom_ssr_subsys_lock);
>>     static int glink_subdev_start(struct rproc_subdev *subdev)
>>   {
>> @@ -189,37 +197,80 @@ void qcom_remove_smd_subdev(struct rproc *rproc, 
>> struct qcom_rproc_subdev *smd)
>>   }
>>   EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
>>   +static struct qcom_ssr_subsystem *qcom_ssr_get_subsys(const char 
>> *name)
>> +{
>> +	struct qcom_ssr_subsystem *info;
>> +
>> +	mutex_lock(&qcom_ssr_subsys_lock);
>> +	/* 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;
> 
> You need to unlock the mutex here.  You would probably
> be better off structuring this with a common exit path
> below, for example:
> 
> 		if (!strcmp(info->name, name))
> 			goto out_mutex_unlock;
> 
> 	. . .
> 
> out_mutex_unlock:
> 	mutex_unlock(&qcom_ssr_subsys_lock);
> 
> 	return info;
> }
> 
>> +
>> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
>> +	if (!info)
>> +		return ERR_PTR(-ENOMEM);
> 
> Here too.  Perhaps this:
> 
> 	if (!info) {
> 		info = ERR_PTR(-ENOMEM);
> 		goto out_mutex_unlock;
> 	}
> 
>> +	info->name = kstrdup_const(name, GFP_KERNEL);
>> +	srcu_init_notifier_head(&info->notifier_list);
>> +
>> +	/* Add to global notification list */
>> +	list_add_tail(&info->list, &qcom_ssr_subsystem_list);
>> +	mutex_unlock(&qcom_ssr_subsys_lock);
>> +
>> +	return info;
>> +}
>> +
>>   /**
>>    * qcom_register_ssr_notifier() - register SSR notification handler
>> - * @nb:		notifier_block to notify for restart notifications
>> + * @name:	Subsystem's SSR name
>> + * @nb:		notifier_block to be invoked upon subsystem's state change
>>    *
>> - * Returns 0 on success, negative errno on failure.
>> + * This registers the @nb notifier block as part the notifier chain 
>> for a
>> + * remoteproc associated with @name. The notifier block's callback
>> + * will be invoked when the remote processor's SSR events occur
>> + * (pre/post startup and pre/post shutdown).
>>    *
>> - * This register the @notify function as handler for restart 
>> notifications. As
>> - * remote processors are stopped this function will be called, with 
>> the SSR
>> - * name passed as a parameter.
>> + * Return: a subsystem cookie on success, ERR_PTR on failure.
>>    */
>> -int qcom_register_ssr_notifier(struct notifier_block *nb)
>> +void *qcom_register_ssr_notifier(const char *name, struct 
>> notifier_block *nb)
>>   {
>> -	return blocking_notifier_chain_register(&ssr_notifiers, nb);
>> +	struct qcom_ssr_subsystem *info;
>> +
>> +	info = qcom_ssr_get_subsys(name);
>> +	if (IS_ERR(info))
>> +		return info;
>> +
>> +	srcu_notifier_chain_register(&info->notifier_list, nb);
>> +
>> +	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
>> + *
>> + * This function will unregister the notifier from the particular 
>> notifier
>> + * chain.
>> + *
>> + * Return: 0 on success, %ENOENT otherwise.
>>    */
>> -void qcom_unregister_ssr_notifier(struct notifier_block *nb)
>> +int qcom_unregister_ssr_notifier(void *notify, struct notifier_block 
>> *nb)
>>   {
>> -	blocking_notifier_chain_unregister(&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 = {
> 
> It's defined in "qcom_rproc.h", but how about naming this type
> qcom_ssr_notify_data (or even just qcom_ssr_notify).
> 
Ok "qcom_ssr_notify_data" sounds fine.
>> +		.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);
>>   }
>>     /**
>> @@ -229,12 +280,20 @@ 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.
> 
> I suggest rewording this comment to make it more general,
> considering the events are related to both startup and
> shutdown.  Scan through the file for other instances
> similar to this (I mentioned one previously).
> 
>>    */
>>   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;
>> +
>> +	info = qcom_ssr_get_subsys(ssr_name);
>> +	if (IS_ERR(info)) {
>> +		dev_err(&rproc->dev, "Failed to add ssr subdevice\n");
>> +		return;
>> +	}
>> +
>> +	ssr->info = info;
>>   	ssr->subdev.unprepare = ssr_notify_unprepare;
> 
> Probably all fields should be initialized each time (though
> I know you're initializing them in the next patch, so I
> guess it's fine...).
> 
> 					-Alex
> 
>>   	rproc_add_subdev(rproc, &ssr->subdev);
>> @@ -249,6 +308,7 @@ EXPORT_SYMBOL_GPL(qcom_add_ssr_subdev);
>>   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);
>>   diff --git a/drivers/remoteproc/qcom_common.h 
>> b/drivers/remoteproc/qcom_common.h
>> index 34e5188..dfc641c 100644
>> --- a/drivers/remoteproc/qcom_common.h
>> +++ b/drivers/remoteproc/qcom_common.h
>> @@ -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,
>> diff --git a/include/linux/remoteproc/qcom_rproc.h 
>> b/include/linux/remoteproc/qcom_rproc.h
>> index fa8e386..58422b1 100644
>> --- a/include/linux/remoteproc/qcom_rproc.h
>> +++ b/include/linux/remoteproc/qcom_rproc.h
>> @@ -5,17 +5,27 @@ struct notifier_block;
>>     #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
>> 

  reply	other threads:[~2020-06-24  1:41 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-23  1:04 [PATCH v5 0/2] Extend SSR notifications framework Rishabh Bhatnagar
2020-06-23  1:04 ` [PATCH v5 1/2] remoteproc: qcom: Add per subsystem SSR notification Rishabh Bhatnagar
2020-06-23 21:45   ` Alex Elder
2020-06-24  1:41     ` rishabhb [this message]
2020-06-24  5:28       ` Bjorn Andersson
2020-06-23  1:04 ` [PATCH v5 2/2] remoteproc: qcom: Add notification types to SSR Rishabh Bhatnagar
2020-06-23 21:45   ` Alex Elder

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cb31dfb50079a1377cf27807a7b2eb3e@codeaurora.org \
    --to=rishabhb@codeaurora.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=elder@ieee.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc-owner@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=psodagud@codeaurora.org \
    --cc=sidgup@codeaurora.org \
    --cc=tsoni@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.