linux-kernel - Re: [PATCH 2/2] pinctrl: starfive: jh7100: ignore disabled device tree nodes

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: <20231204080543.C8LyBqTM@linutronix.de>
Date:   Mon, 4 Dec 2023 09:05:43 +0100
From:   Nam Cao <namcao@...utronix.de>
To:     Emil Renner Berthing <emil.renner.berthing@...onical.com>
Cc:     Emil Renner Berthing <kernel@...il.dk>,
        Jianlong Huang <jianlong.huang@...rfivetech.com>,
        Hal Feng <hal.feng@...rfivetech.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Huan Feng <huan.feng@...rfivetech.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Drew Fustini <drew@...gleboard.org>,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org
Subject: Re: [PATCH 2/2] pinctrl: starfive: jh7100: ignore disabled device
 tree nodes

Hi Emil,

On Fri, Dec 01, 2023 at 03:28:27PM +0100, Emil Renner Berthing wrote:
> Nam Cao wrote:
> > diff --git a/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c b/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c
> > index 530fe340a9a1..561fd0c6b9b0 100644
> > --- a/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c
> > +++ b/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c
> > @@ -492,7 +492,7 @@ static int starfive_dt_node_to_map(struct pinctrl_dev *pctldev,
> >
> > nmaps = 0;
> > ngroups = 0;
> > - for_each_child_of_node(np, child) {
> > + for_each_available_child_of_node(np, child) {
>
> Is this safe to do? I mean will the children considered "available" not change
> as drivers are loaded during boot so this is racy?

I think if node removal like this causes race condition, we would
already have race condition with node addition too: "what if the nodes
are added while the drivers are being loaded?"

At least with U-Boot, the device tree overlay is "merged" into the base
device tree, before the kernel even runs, so no race there. I don't know
if there are any cases where the device tree overlay is not guaranteed
to be applied before driver loading, but those cases do not sound sane
to me: they would cause race condition, regardless of whether nodes are
added or removed.

> Also arguably this is not a bugfix, but a new feature.

I'm not sure myself, I haven't seen official documentation/rules about
this. But many people do consider this to be a bug:

"Though you can add/override 'status' with 'status = "disabled";' which
should be treated very similar to a node not being present. I say
similar because it's a source of bugs for the OS to fail to pay
attention to status property." - Rob Herring [1].

"Linux has widespread use of the "status" property to indicate that a
node does not exist. (...). Expect efforts to fix the kernel code to
respect the "status" property." - elinux.org [2].

And I do agree with them. When someone write a device tree with some
nodes with "status = disabled" for whatever reasons, clearly they intend
to exclude these nodes.

Though I must admit that I am still quite new, so please correct me if
my reasoning/understanding is flawed.

Best regards,
Nam

[1] https://lore.kernel.org/lkml/CAL_JsqLV5d5cL3o3Dx=--zGD37c5O09rL9AXyRFmceTfBHt3Zg@mail.gmail.com/
[2] https://elinux.org/Device_Tree_Linux#status_property