| 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
| ||
|
Message-ID: <CA+V-a8sqV2KOf2iCt9fHgxNhJG92UYdm0v7mZGNZj4U8QLRSXQ@mail.gmail.com>
Date: Fri, 11 Oct 2024 12:55:19 +0100
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>, linux-sound@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
Biju Das <biju.das.jz@...renesas.com>,
Fabrizio Castro <fabrizio.castro.jz@...esas.com>,
Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH] ASoC: rsnd: Fix probe failure on HiHope boards due to
endpoint parsing
Hi Geert,
Thank you for the review.
On Fri, Oct 11, 2024 at 7:42 AM Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Thu, Oct 10, 2024 at 4:15 PM Prabhakar <prabhakar.csengg@...il.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> >
> > On the HiHope boards, we have a single port with a single endpoint defined
> > as below:
> > ....
> > rsnd_port: port {
> > rsnd_endpoint: endpoint {
> > remote-endpoint = <&dw_hdmi0_snd_in>;
> >
> > dai-format = "i2s";
> > bitclock-master = <&rsnd_endpoint>;
> > frame-master = <&rsnd_endpoint>;
> >
> > playback = <&ssi2>;
> > };
> > };
> > ....
> >
> > With commit 547b02f74e4a ("ASoC: rsnd: enable multi Component support for
> > Audio Graph Card/Card2"), support for multiple ports was added. This caused
> > probe failures on HiHope boards, as the endpoint could not be retrieved due
> > to incorrect device node pointers being used.
> >
> > This patch fixes the issue by updating the `rsnd_dai_of_node()` and
> > `rsnd_dai_probe()` functions to use the correct device node pointers based
> > on the port names ('port' or 'ports'). It ensures that the endpoint is
> > properly parsed for both single and multi-port configurations, restoring
> > compatibility with HiHope boards.
> >
> > Fixes: 547b02f74e4a ("ASoC: rsnd: enable multi Component support for Audio Graph Card/Card2")
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
>
> Thanks for your patch!
>
> > --- a/sound/soc/sh/rcar/core.c
> > +++ b/sound/soc/sh/rcar/core.c
> > @@ -1281,7 +1281,9 @@ static int rsnd_dai_of_node(struct rsnd_priv *priv, int *is_graph)
> > if (!of_node_name_eq(ports, "ports") &&
> > !of_node_name_eq(ports, "port"))
> > continue;
> > - priv->component_dais[i] = of_graph_get_endpoint_count(ports);
> > + priv->component_dais[i] =
> > + of_graph_get_endpoint_count(of_node_name_eq(ports, "ports") ?
> > + ports : np);
>
> As of_node_name_eq() is not inline or __pure, of_node_name_eq(ports,
> "ports") will be called twice. So it may make sense to add a helper,
> combining the selection with the validation above:
>
> const struct device_node *pick_endpoint_node_for_ports(const
> struct device_node *np,
> const struct device_node *e_ports, const struct
> device_node *e_port)
> {
> if (of_node_name_eq(ports, "ports"))
> return e_ports;
> if (of_node_name_eq(ports, "port"))
> return e_port;
> return NULL;
> }
>
I'll drop the const maybe to reduce the diff and have something like below:
+static struct device_node*
+ pick_endpoint_node_for_ports(struct device_node *e_ports,
+ struct device_node *e_port)
+{
+ if (of_node_name_eq(e_ports, "ports"))
+ return e_ports;
+
+ if (of_node_name_eq(e_ports, "port"))
+ return e_port;
+
+ return NULL;
+}
and use it like below in the rsnd_dai_of_node() function:
@@ -1278,10 +1291,10 @@ static int rsnd_dai_of_node(struct rsnd_priv
*priv, int *is_graph)
* Audio-Graph-Card
*/
for_each_child_of_node(np, ports) {
- if (!of_node_name_eq(ports, "ports") &&
- !of_node_name_eq(ports, "port"))
+ node = pick_endpoint_node_for_ports(ports, np);
+ if (!node)
continue;
- priv->component_dais[i] = of_graph_get_endpoint_count(ports);
+ priv->component_dais[i] = of_graph_get_endpoint_count(node);
> > nr += priv->component_dais[i];
> > i++;
> > if (i >= RSND_MAX_COMPONENT) {
> > @@ -1493,7 +1495,8 @@ static int rsnd_dai_probe(struct rsnd_priv *priv)
> > if (!of_node_name_eq(ports, "ports") &&
> > !of_node_name_eq(ports, "port"))
> > continue;
> > - for_each_endpoint_of_node(ports, dai_np) {
> > + for_each_endpoint_of_node(of_node_name_eq(ports, "ports") ?
> > + ports : np, dai_np) {
>
> Likewise.
>
Ok, now I will have to create a local variable for this loop, which
will look like something below:
@@ -1486,14 +1499,15 @@ static int rsnd_dai_probe(struct rsnd_priv *priv)
*/
dai_i = 0;
if (is_graph) {
+ struct device_node *dai_np_port;
struct device_node *ports;
struct device_node *dai_np;
for_each_child_of_node(np, ports) {
- if (!of_node_name_eq(ports, "ports") &&
- !of_node_name_eq(ports, "port"))
+ dai_np_port = pick_endpoint_node_for_ports(ports, np);
+ if (!dai_np_port)
continue;
- for_each_endpoint_of_node(ports, dai_np) {
+ for_each_endpoint_of_node(dai_np_port, dai_np) {
__rsnd_dai_probe(priv, dai_np, dai_np,
0, dai_i);
if (!rsnd_is_gen1(priv) &&
!rsnd_is_gen2(priv)) {
rdai = rsnd_rdai_get(priv, dai_i);
Does that sound OK?
Cheers,
Prabhakar
Powered by blists - more mailing lists