Re: [PATCH] ASoC: simple-card-utils: Check playback-only and capture-only input value

[Kuninori Morimoto]

Hi again
Add Alexander

> > The audio-graph-card2 gets the value of 'playback-only' and
> > 'capture_only' property in below sequence, if there is 'playback_only' or
> > 'capture_only' property in port_cpu and port_codec nodes, but no these
> > properties in ep_cpu and ep_codec nodes, the value of playback_only and
> > capture_only will be flushed to zero in the end.
> > 
> > graph_util_parse_link_direction(lnk,            &playback_only, &capture_only);
> > graph_util_parse_link_direction(ports_cpu,      &playback_only, &capture_only);
> > graph_util_parse_link_direction(ports_codec,    &playback_only, &capture_only);
> > graph_util_parse_link_direction(port_cpu,       &playback_only, &capture_only);
> > graph_util_parse_link_direction(port_codec,     &playback_only, &capture_only);
> > graph_util_parse_link_direction(ep_cpu,         &playback_only, &capture_only);
> > graph_util_parse_link_direction(ep_codec,       &playback_only, &capture_only);
> > 
> > So check the input value of playback_only and capture_only in
> > graph_util_parse_link_direction() function, if they are true, then won't
> > rewrite the values.
> > 
> > Fixes: 22a507d7680f ("ASoC: simple-card-utils: Check device node before overwrite direction")
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@xxxxxxx>
> > ---
> >  sound/soc/generic/simple-card-utils.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
> > index 3115e1f37c0c..e1564c1f7d8d 100644
> > --- a/sound/soc/generic/simple-card-utils.c
> > +++ b/sound/soc/generic/simple-card-utils.c
> > @@ -1202,9 +1202,9 @@ void graph_util_parse_link_direction(struct device_node *np,
> >  	bool is_playback_only = of_property_read_bool(np, "playback-only");
> >  	bool is_capture_only  = of_property_read_bool(np, "capture-only");
> >  
> > -	if (np && playback_only)
> > +	if (np && playback_only && !(*playback_only))
> >  		*playback_only = is_playback_only;
> > -	if (np && capture_only)
> > +	if (np && capture_only && !(*capture_only))
> >  		*capture_only = is_capture_only;
> >  }
> 
> Hmm... It become complex more and more...
> 
> I think it should check "is_playback_only" instead of "playback_only" ?
> Oops, this patch changed the behavior, and it is the reason why you
> got issue many times.
> 
> 	3cc393d2232ec770b5f79bf0673d67702a3536c3
>     	("ASoC: simple-card-utils: Fix pointer check in graph_util_parse
> 	 _link_direction")
> 
> *playback_only pointer check is indeed needed, but *np pointer check is
> not needed because of_property_read_bool() will ignore if it was NULL.
> So maybe we want to have is
> 
> 	if (playback_only && is_playback_only)
> 	    (A)              (B)
> 
> 	(A) pointer check
> 	(B) value   check


commit 3cc393d2232ec770b5f79bf0673d67702a3536c3 say

    Actually check if the passed pointers are valid, before writing to them.
    This also fixes a USBAN warning:
    UBSAN: invalid-load in ../sound/soc/fsl/imx-card.c:687:25
    load of value 255 is not a valid value for type '_Bool'

    This is because playback_only is uninitialized and is not written to, as
    the playback-only property is absent.

But I guess this is because he didn't initialise his playback-only,
instead of graph_util_parse_link_direction() ?

------ 8< ------ 8< ------ 8< ------ 8< ------ 8< ------ 8< ------
diff --git a/sound/soc/fsl/imx-card.c b/sound/soc/fsl/imx-card.c
index 05b4e971a3661..fc1563fd21f17 100644
--- a/sound/soc/fsl/imx-card.c
+++ b/sound/soc/fsl/imx-card.c
@@ -544,7 +544,7 @@ static int imx_card_parse_of(struct imx_card_data *data)
        struct snd_soc_dai_link *link;
        struct dai_link_data *link_data;
        struct of_phandle_args args;
-       bool playback_only, capture_only;
+       bool playback_only = 0, capture_only = 0;
        int ret, num_links;
        u32 asrc_fmt = 0;
        u32 width;