linux-kernel - Re: [PATCH] ASoC: soc-core: Stop using of_property_read

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: <CAMuHMdWpxKgSGGJb9Oy4Bzy7y4zPxVve=0_mCWE0G1d4njCJ+A@mail.gmail.com>
Date: Thu, 23 Jan 2025 09:38:28 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
Cc: Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, 
	Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>, Rob Herring <robh@...nel.org>, 
	linux-sound@...r.kernel.org, linux-renesas-soc@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ASoC: soc-core: Stop using of_property_read_bool() for
 non-boolean properties

Hi Morimoto-san,

On Thu, Jan 23, 2025 at 12:43 AM Kuninori Morimoto
<kuninori.morimoto.gx@...esas.com> wrote:
> > On R-Car:
> >
> >     OF: /sound: Read of boolean property 'simple-audio-card,bitclock-master' with a value.
> >     OF: /sound: Read of boolean property 'simple-audio-card,frame-master' with a value.
> >
> > or:
> >
> >     OF: /soc/sound@...00000/ports/port@...ndpoint: Read of boolean property 'bitclock-master' with a value.
> >     OF: /soc/sound@...00000/ports/port@...ndpoint: Read of boolean property 'frame-master' with a value.
> >
> > The use of of_property_read_bool() for non-boolean properties is
> > deprecated in favor of of_property_present() when testing for property
> > presence.
> >
> > Replace testing for presence before calling of_property_read_u32() by
> > testing for an -EINVAL return value from the latter, to simplify the
> > code.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@...der.be>
> > ---
> (snip)
> > -     if (of_property_read_bool(np, "dai-tdm-slot-num")) {
> > -             ret = of_property_read_u32(np, "dai-tdm-slot-num", &val);
> > -             if (ret)
> > -                     return ret;
> > -
> > -             if (slots)
> > -                     *slots = val;
> > -     }
> (snip)
> > +     ret = of_property_read_u32(np, "dai-tdm-slot-num", &val);
> > +     if (ret && ret != -EINVAL)
> > +             return ret;
> > +     if (!ret && slots)
> > +             *slots = val;
>
> Looks good to me
>
> Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>

Thank you!

> If my understanding was correct, old/new code should have same behavior.

Indeed, that was my objective...

> But because of the original code, new code looks complex for me.
> The case which this function return error are
>
>         (A) if property does not have a value
>         (B) if the property data isn't large enough
>
> I think "DT checker" will indicates error for both case ?

Correct, of_property_read_u32_array() would return -ENODATA resp.
-EOVERFLOW.

> If so, we can simply ignore these 2 cases. Then, the code will be more
> simple
>
>         ret = of_property_read_u32(np, "dai-tdm-slot-num", &val);
> -       if (ret && ret != -EINVAL)
> -               return ret;
>         if (!ret && slots)
>                 *slots = val;
>
> I think this should be extra new patch (if people can agree about it).

That would be a change in behavior. Probably it would be fine for
existing users, though, as no existing DTS should cause these errors,
else sound wouldn't work.  For a new DTS, it would silently ignore errors.
You are in a better position to make that decision, though.

BTW, is there any specific reason the code always checks for the
presence of "dai-tdm-slot-num", even if slots is NULL, and the result
sn't used? I.e. would

    if (slots) {
            ret = of_property_read_u32(np, "dai-tdm-slot-num", &val);
            if (!ret)
                    *slots = val;
            else if (ret != -EINVAL)
                    return ret;
    }

(perhaps dropping the else, as per above) be acceptable?
Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds