Received-SPF: pass (google.com: domain of linux-kernel+bounces-29288-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 17 Jan 2024 11:41:14 -0600
From: Rob Herring <robh@kernel.org>
To: Stephen Boyd <sboyd@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>, linux-kernel@vger.kernel.org,
	patches@lists.linux.dev, linux-um@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org, kunit-dev@googlegroups.com,
	linux-kselftest@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH 4/6] of: Create of_root if no dtb provided by firmware
Message-ID: <20240117174114.GA2779523-robh@kernel.org>
References: <20240112200750.4062441-1-sboyd@kernel.org>
 <20240112200750.4062441-5-sboyd@kernel.org>
 <20240115203230.GA1439771-robh@kernel.org>
 <cdaadf62222a705cda198dd96dc7c73d.sboyd@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <cdaadf62222a705cda198dd96dc7c73d.sboyd@kernel.org>

On Tue, Jan 16, 2024 at 05:18:15PM -0800, Stephen Boyd wrote:
> Quoting Rob Herring (2024-01-15 12:32:30)
> > On Fri, Jan 12, 2024 at 12:07:47PM -0800, Stephen Boyd wrote:
> > > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> > > index da9826accb1b..9628e48baa15 100644
> > > --- a/drivers/of/Kconfig
> > > +++ b/drivers/of/Kconfig
> > > @@ -54,9 +54,14 @@ config OF_FLATTREE
> > >       select CRC32
> > >  
> > >  config OF_EARLY_FLATTREE
> > > -     bool
> > > +     bool "Functions for accessing Flat Devicetree (FDT) early in boot"
> > 
> > I think we could instead just get rid of this kconfig option. Or 
> > always enable with CONFIG_OF (except on Sparc). The only cost of 
> > enabling it is init section functions which get freed anyways.
> 
> Getting rid of it is a more massive change. It can be the default and
> kept hidden instead? If it can't be selected on Sparc then it should be
> hidden there anyway.

The easier option is certainly fine for this series. I just don't want 
it visible.

> > >       select DMA_DECLARE_COHERENT if HAS_DMA && HAS_IOMEM
> > >       select OF_FLATTREE
> > > +     help
> > > +       Normally selected by platforms that process an FDT that has been
> > > +       passed to the kernel by the bootloader.  If the bootloader does not
> > > +       pass an FDT to the kernel and you need an empty devicetree that
> > > +       contains only a root node to exist, then say Y here.
> > >  
> > >  config OF_PROMTREE
> > >       bool
> [...]
> > > @@ -195,6 +191,17 @@ static inline int of_node_check_flag(const struct device_node *n, unsigned long
> > >       return test_bit(flag, &n->_flags);
> > >  }
> > >  
> > > +/**
> > > + * of_have_populated_dt() - Has DT been populated by bootloader
> > > + *
> > > + * Return: True if a DTB has been populated by the bootloader and it isn't the
> > > + * empty builtin one. False otherwise.
> > > + */
> > > +static inline bool of_have_populated_dt(void)
> > > +{
> > > +     return of_root != NULL && !of_node_check_flag(of_root, OF_EMPTY_ROOT);
> > 
> > Just a side comment, but I think many/all callers of this function could 
> > just be removed.
> > 
> > I don't love new flags. Another possible way to handle this would be 
> > checking for "compatible" being present in the root node. I guess this 
> > is fine as-is for now at least.
> 
> Ok. I can add a check for a compatible property. That's probably better
> anyway. Should there be a compatible property there to signal that this
> DT isn't compatible with anything? I worry about DT overlays injecting a
> compatible string into the root node, but maybe that is already
> prevented.

I worry about DT overlays injecting anything...

I don't think it is explicitly forbidden, but I have asked that any 
general purpose interface to apply overlays be restricted to nodes 
explicitly allowed (e.g. downstream of a connector node). For now, the 
places (i.e. drivers) overlays are applied are limited.

We could probably restrict the root node to new nodes only and no new 
or changed properties.

Rob