linux-kernel - Re: [PATCH v2 9/9] ASoC: simple-card: Handle additional devices

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]

Date:   Fri, 26 May 2023 15:07:02 +0200
From:   Herve Codina <herve.codina@...tlin.com>
To:     Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, alsa-devel@...a-project.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-iio@...r.kernel.org,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v2 9/9] ASoC: simple-card: Handle additional devices

On Thu, 25 May 2023 00:01:14 +0000
Kuninori Morimoto <kuninori.morimoto.gx@...esas.com> wrote:

> Hi Herve
> 
> Thank you for your reply.
> 
> > So, IMHO, calling simple_populate_aux() from __simple_for_each_link() is
> > not correct as it has nothing to do with DAI links and must be call once
> > per Card.  
> 
> My biggest concern is that this code is calling same code multiple times.
> It is easy to forget such thing when updating in this kind of code.
> We don't forget / take mistake if these are merged.
> But we have such code everywhere ;) this is just my concern, not a big deal.
> 
> 	static int __simple_for_each_link (...)
> 	{
> 		...
> =>		add_devs = of_get_child_by_name(top, PREFIX "additional-devs");  
> 		...
> 	}
> 
> 	static int simple_populate_aux(...)
> 	{
> 		...
> =>		node = of_get_child_by_name(dev->of_node, PREFIX "additional-devs");  
> 		...
> 	}
> 

Well, of_get_child_by_name() is called twice to retrieve the additional-devs
node but for very different reason.

In __simple_for_each_link() to filter out the node as it has nothing to do with a DAI.
In simple_populate_aux() to take care of the devices declared in the node.

I am not sure that we should avoid that.
It will lead to a more complex code and flags just to avoid this call.

Not sure that it will be better.
__simple_for_each_link() is called multiple times and is supposed to look at links.
To avoid the of_get_child_by_name() filter-out call, __simple_for_each_link()
will look at link *and* populate devices calling simple_populate_aux().
And to do that correctly it will use a flag to be sure that simple_populate_aux()
was called only once.


In order to avoid some kind of duplication (at least the node name):

	static struct device_node *simple_of_get_add_devs(struct device_node *node)
	{
		return of_get_child_by_name(node, PREFIX "additional-devs");
	}

	static int __simple_for_each_link (...)
	{
		...
=>		add_devs = simple_of_get_add_devs(top);  
		...
	}

	static int simple_populate_aux(...)
	{
		...
=>		node = simple_of_get_add_devs(dev->of_node);  
		...
	}


Does it look better ?

Best regards,
Hervé

> Thank you for your help !!
> 
> Best regards
> ---
> Kuninori Morimoto



-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com