Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 4 Oct 2022 16:56:40 -0600
From:   Jack Rosenthal <jrosenth@chromium.org>
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     linux-kernel@vger.kernel.org, chrome-platform@lists.linux.dev,
        Stephen Boyd <swboyd@chromium.org>,
        Tzung-Bi Shih <tzungbi@kernel.org>,
        Guenter Roeck <groeck@chromium.org>,
        Julius Werner <jwerner@chromium.org>
Subject: Re: [PATCH v12] firmware: google: Implement cbmem in sysfs driver
Message-ID: <Yzy6KNUHFqxWZb9U@chromium.org>
References: <20221004003811.4075765-1-jrosenth@chromium.org>
 <Yzvz9hn1G8rU4VaO@kroah.com>
 <Yzxl2oczTtwEvIqt@chromium.org>
 <Yzxsu2Ms43eTfOYR@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Yzxsu2Ms43eTfOYR@kroah.com>
Precedence: bulk

On 2022-10-04 at 19:26 +0200, Greg KH wrote:
> On Tue, Oct 04, 2022 at 10:56:58AM -0600, Jack Rosenthal wrote:
> > On 2022-10-04 at 10:51 +0200, Greg KH wrote:
> > > > +		A list of ids known to Coreboot can be found in the coreboot
> > > > +		source tree at
> > > > +		``src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h``.
> > > 
> > > That will not age well, why not point to the reference in the kernel
> > > tree instead?
> > 
> > There is no copy in the kernel tree.
> 
> Then how does the kernel know what to print out?  Why not add such a
> reference somewhere?

The kernel really doesn't need to know the list of possible ids: it
simply reads what coreboot gives it from the coreboot tables and proxies
that on up to sysfs nodes.

In an earlier version of this patch
(https://lore.kernel.org/chrome-platform/CAODwPW-JzXXsEANaS+6n695YqriAQ0j0LXm31R2u1OP3MhX9Uw@mail.gmail.com/T/#u),
I actually had this list so that the directory names were human-readable
instead of using the 32-bit CBMEM id, but Julius didn't like it citing
that we'd have to keep the kernel tree and the coreboot tree in sync.

I'm fine with either solution ... just want to make all parties happy
here =)

> 
> > > >  config GOOGLE_COREBOOT_TABLE
> > > >  	tristate "Coreboot Table Access"
> > > >  	depends on HAS_IOMEM && (ACPI || OF)
> > > > diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
> > > > index d17caded5d88..8151e323cc43 100644
> > > > --- a/drivers/firmware/google/Makefile
> > > > +++ b/drivers/firmware/google/Makefile
> > > > @@ -7,5 +7,8 @@ obj-$(CONFIG_GOOGLE_MEMCONSOLE)            += memconsole.o
> > > >  obj-$(CONFIG_GOOGLE_MEMCONSOLE_COREBOOT)   += memconsole-coreboot.o
> > > >  obj-$(CONFIG_GOOGLE_MEMCONSOLE_X86_LEGACY) += memconsole-x86-legacy.o
> > > >  
> > > > +# Must come after coreboot_table.o, as this driver depends on that bus type.
> > > 
> > > Doesn't the linker handle this for us?
> > 
> > Not in the case of compiling as a built-in module: I observed in this
> > scenario the order in the Makefile deterimined the module initialization
> > order, and, if this were to be listed alphabetically, the coreboot_table
> > module would not have been loaded before the cbmem module.
> 
> So is this a runtime dependancy or a symbol/link dependancy?
> 
> link one is easy, we always go off of the Makefile order, and if you
> move it and it breaks, well obviously move it back.  If it's a runtime
> order, then how will you handle one being a module and the other not?

link/symbol dependency ... it's just the cbmem driver depends on the
coreboot bus.  Existing EXPORT_SYMBOL facilities should have this
figured out for the module case, but the Makefile just needs to be in
the right order for the built-in module case.