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:   Mon, 3 Jul 2023 14:02:15 +0100
From:   Conor Dooley <conor.dooley@microchip.com>
To:     =?utf-8?B?6L+Q6L6J5bSU?= <cuiyunhui@bytedance.com>
CC:     Conor Dooley <conor@kernel.org>, <ardb@kernel.org>,
        <palmer@dabbelt.com>, <paul.walmsley@sifive.com>,
        <aou@eecs.berkeley.edu>, <linux-riscv@lists.infradead.org>,
        <rminnich@gmail.com>, <mark.rutland@arm.com>,
        <lpieralisi@kernel.org>, <rafael@kernel.org>, <lenb@kernel.org>,
        <jdelvare@suse.com>, <yc.hung@mediatek.com>,
        <angelogioacchino.delregno@collabora.com>,
        <allen-kh.cheng@mediatek.com>,
        <pierre-louis.bossart@linux.intel.com>,
        <tinghan.shen@mediatek.com>, <linux-kernel@vger.kernel.org>,
        <linux-acpi@vger.kernel.org>, <geshijian@bytedance.com>,
        <weidong.wd@bytedance.com>
Subject: Re: [External] Re: [PATCH v2 2/3] firmware: introduce FFI for SMBIOS
 entry.
Message-ID: <20230703-patrol-paced-76cd5e24db6b@wendy>
References: <20230702095735.860-1-cuiyunhui@bytedance.com>
 <20230702095735.860-2-cuiyunhui@bytedance.com>
 <20230702-collide-rumor-f0d915a4f1b2@spud>
 <CAEEQ3w=CMSMnYmfprYS4ydsA=EBJtLhQQHWky754EC-iifcYtg@mail.gmail.com>
 <20230703-rebuff-smashing-6b2c6f80ba05@wendy>
 <CAEEQ3w=-1Zns_pZRSeM0+Wv46y6RttcT5jFy_ENnq--RptYp2g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="Uht1cacpbtCJeEos"
Content-Disposition: inline
In-Reply-To: <CAEEQ3w=-1Zns_pZRSeM0+Wv46y6RttcT5jFy_ENnq--RptYp2g@mail.gmail.com>
Precedence: bulk

--Uht1cacpbtCJeEos
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Jul 03, 2023 at 08:41:30PM +0800, =E8=BF=90=E8=BE=89=E5=B4=94 wrote:
> On Mon, Jul 3, 2023 at 4:36=E2=80=AFPM Conor Dooley <conor.dooley@microch=
ip.com> wrote:
> > On Mon, Jul 03, 2023 at 04:23:53PM +0800, =E8=BF=90=E8=BE=89=E5=B4=94 w=
rote:

> > > > > +FDT FIRMWARE INTERFACE (FFI)
> > > > > +M:   Yunhui Cui cuiyunhui@bytedance.com
> > > > > +S:   Maintained
> > > > > +F:   drivers/firmware/ffi.c
> > > > > +F:   include/linux/ffi.h
> > > >
> > > > Are you going to apply patches for this, or is someone else?
> > > Yes,  it will be used by patch 3/3.
> >
> > That's not what I asked :(
>=20
> Sorry,  ok,  what do you want to ask?

Who is going to apply patches for drivers/firmware/ffi*?

> > > > >  static void __init dmi_scan_machine(void)
> > > > > @@ -660,58 +686,22 @@ static void __init dmi_scan_machine(void)
> > > > >       char __iomem *p, *q;
> > > > >       char buf[32];
> > > > >
> > > > > +#ifdef CONFIG_FDT_FW_INTERFACE
> > > > > +     if (dmi_sacn_smbios(ffi.smbios3, ffi.smbios))
> > > >
> > > > "dmi_sacn_smbios"
> > > >
> > > > > +             goto error;
> > > > > +#endif
> > > >
> > > > Does this not mean that if FDT_FW_INTERFACE is enabled, but the pla=
tform
> > > > wants to use EFI, it won't be able to? The `goto error;` makes this=
 look
> > > > mutually exclusive to my efi-unaware eyes.
> > >
> > > If you have enabled FFI, then if something goes wrong, you should got=
o error.
> > > Just like the origin code:
> > >         if (efi_enabled(EFI_CONFIG_TABLES)) {
> > >                 if (dmi_sacn_smbios(efi.smbios3, efi.smbios))
> > >                         goto error;
> > >         } else if (IS_ENABLED(CONFIG_DMI_SCAN_MACHINE_NON_EFI_FALLBAC=
K)) {
> > >                 p =3D dmi_early_remap(SMBIOS_ENTRY_POINT_SCAN_START, =
0x10000);
> > >                 if (p =3D=3D NULL)
> > >                         goto error;
> >
> > Does this not make FFI and EFI mutually exclusive Kconfig options?
> > Suppose you are on a system that does not implement FFI, but does
> > implement EFI - what's going to happen then?
> > AFAICT, dmi_sacn_smbios(ffi.smbios3, ffi.smbios) will fail & you'll do a
> > `goto error` & skip the EFI code. What am I missing?
>=20
> Code is not intended to be mutually exclusive, get the correct value and =
return,
> The code is going to be changed to this:
>=20
> #ifdef CONFIG_FDT_FW_INTERFACE

Ideally, these would be IS_ENABLED() instead of #ifdef - but if you copy
what EFI does, then you don't need either, as there will always be an
ffi_enabled() defined.

>         if (ffi_enabled(FFI_CONFIG_TABLES)) {

I don't know what this function is, but this code seems like a step in
the right direction.

>                 if (!dmi_sacn_smbios(ffi.smbios3, ffi.smbios))
>                         return;
>         }
> #endif

Thanks,
Conor.

--Uht1cacpbtCJeEos
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZKLG1wAKCRB4tDGHoIJi
0p4CAP9UFrVTshkvkiDgNgv0fkaJeO/6lTdr5tKFaGdb9LpN5gD9HCH4Z3GCfUq8
vJfacU093Un2qeuoIdVM+04hLm4hnQ0=
=/UOt
-----END PGP SIGNATURE-----

--Uht1cacpbtCJeEos--