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] [day] [month] [year] [list]

Date:   Thu, 12 Jul 2018 18:09:42 +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 12-07-18, 13:02, Srinivas Kandagatla wrote:
> Thanks Vinod for taking look at this!
> 
> On 12/07/18 11:59, Vinod wrote:
> > 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 *
> > 
> 
> I can rename argument "data" to "name", but I can not change the prototype
> of the compare callback expected by component framework.

oh yes the data type can't be changed.

> 
> > > +{
> > > +	struct snd_soc_component *component = NULL;
> > > +	struct snd_soc_component *t;
> > 
> > why do you need two variables for this
> 
> We need two because one is iterator and other is find.
> 
> if we use just one we will endup with valid iterator item which may not have
> matched dev.
> 
> Or I can re-organize/simplify the code like this:
> 
> static int snd_soc_card_comp_compare(struct device *dev, void *name)
> {
>         struct snd_soc_component *t;
> 
>         lockdep_assert_held(&client_mutex);
>         list_for_each_entry(t, &component_list, list) {
>                 if (dev == t->dev) {
>                         if (!strcmp(t->name, name))
>                                 return 1;
>                 }
>         }
> 
>         return 0;
> }

looks better :)

> 
> > > +
> > > +	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?
> 
> AFAIU, strcmp should be safe here as component->name is generated/sanitized
> by core and would be of max len of NAME_SIZE.
> core uses strcmp in may places. Any particular reason you want me to move to
> strncmp?

I agree that name is generated but I don't trust strings :)

-- 
~Vinod