linux-kernel - Re: [PATCH] ASoC: core: add support to card re-bind/unbind using component framework

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180712105910.GU3219@vkoul-mobl>
Date:   Thu, 12 Jul 2018 16:29:10 +0530
From:   Vinod <vkoul@...nel.org>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc:     broonie@...nel.org, lgirdwood@...il.com,
        alsa-devel@...a-project.org, perex@...ex.cz, tiwai@...e.com,
        linux-kernel@...r.kernel.org, rohitkr@...eaurora.org,
        bgoswami@...eaurora.org
Subject: Re: [PATCH] ASoC: core: add support to card re-bind/unbind using
 component framework

On 11-07-18, 09:43, Srinivas Kandagatla wrote:
> This patch aims at add achieving dynamic behaviour of audio card when
> the dependent components disappear and reappear.
> 
> With this patch the card is removed if any of the dependent component
> is removed and card is added back if the dependent component comes back.
> All this is done using component framework and matching based on
> component name.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>

Looks fine mostly, some nitpicks below:

> ---
> 
> During discussion regarding card re-binding when components unregister
> and register back at https://lkml.org/lkml/2018/7/9/785 it was suggested
> that component framework can be added into core to provide this feature.
> 
> With this new changes the card will re-bind once the dependent component
> re-registers after unregistering. This works based on the match done
> from component name using component framework.
> 
> I have tested this patch with qdsp start-stop usecase for more than 10000
> times in loop on Qcom platforms.
> 
> I will send qdsp side cleanup patches once I get some feedback on this patch.
> 
> 
>  include/sound/soc.h  |  5 ++++
>  sound/soc/soc-core.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 70 insertions(+), 5 deletions(-)
> 
> diff --git a/include/sound/soc.h b/include/sound/soc.h
> index 870ba6b64817..b94149d29c0d 100644
> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -17,6 +17,7 @@
>  #include <linux/workqueue.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
> +#include <linux/component.h>
>  #include <linux/regmap.h>
>  #include <linux/log2.h>
>  #include <sound/core.h>
> @@ -1088,6 +1089,10 @@ struct snd_soc_card {
>  
>  	struct work_struct deferred_resume_work;
>  
> +	/* component framework related */
> +	bool components_added;
> +	struct component_match *match;
> +
>  	/* lists of probed devices belonging to this card */
>  	struct list_head component_dev_list;
>  
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index 6d33634b934b..377ed8e67686 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -279,11 +279,31 @@ static inline void snd_soc_debugfs_exit(void)
>  
>  #endif
>  
> +static int snd_soc_card_comp_compare(struct device *dev, void *data)

Why not make last arg as name and avoid void *

> +{
> +	struct snd_soc_component *component = NULL;
> +	struct snd_soc_component *t;

why do you need two variables for this
> +
> +	lockdep_assert_held(&client_mutex);
> +	list_for_each_entry(t, &component_list, list) {
> +		if (dev == t->dev) {
> +			component = t;

you can skip this line and use t in below code.

> +			break;
> +		}
> +	}
> +
> +	if (component && !strcmp(component->name, data))

strncmp?

-- 
~Vinod