Re: [External] Re: [PATCH v2 2/3] firmware: introduce FFI for SMBIOS entry.

[运辉崔]

Hi Conor,


> nit: please don't write your commit messages as bullet lists
Okay, thanks for your suggestion.

> > +FDT FIRMWARE INTERFACE (FFI)
> > +M:   Yunhui Cui cuiyunhui@xxxxxxxxxxxxx
> > +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.

>
> >  EXTERNAL CONNECTOR SUBSYSTEM (EXTCON)
> >  M:   MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
> >  M:   Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > index b59e3041fd62..ea0149fb4683 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -303,6 +303,17 @@ config TURRIS_MOX_RWTM
> >         other manufacturing data and also utilize the Entropy Bit Generator
> >         for hardware random number generation.
> >
> > +config FDT_FW_INTERFACE
> > +       bool "An interface for passing firmware info through FDT"
> > +       depends on OF && OF_FLATTREE
> > +       default n
> > +       help
> > +         When some bootloaders do not support EFI, and the arch does not
> > +         support SMBIOS_ENTRY_POINT_SCAN_START, then you can enable this option
> > +         to support the transfer of firmware information, such as smbios tables.
>
> Could you express this dependency on !SMBIOS_ENTRY_POINT_SCAN_START in
> Kconfig & then simply the text to:
> "Enable this option to support the transfer of firmware information,
> such as smbios tables, for bootloaders that do not support EFI."
> since it would not even appear if the arch supports scanning for the
> entry point?
> If I was was a punter trying to configure my kernel in menuconfig or
> whatever, I should be able to decide based on the help text if I need
> this, not going grepping for #defines in headers.
Okay， I'll update on v3.


>
> >  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 platform
> 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 goto 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_FALLBACK)) {
                p = dmi_early_remap(SMBIOS_ENTRY_POINT_SCAN_START, 0x10000);
                if (p == NULL)
                        goto error;
....
}

>
> >       if (efi_enabled(EFI_CONFIG_TABLES)) {
> > -             /*
> > -              * According to the DMTF SMBIOS reference spec v3.0.0, it is
> > -              * allowed to define both the 64-bit entry point (smbios3) and
> > -              * the 32-bit entry point (smbios), in which case they should
> > -              * either both point to the same SMBIOS structure table, or the
> > -              * table pointed to by the 64-bit entry point should contain a
> > -              * superset of the table contents pointed to by the 32-bit entry
> > -              * point (section 5.2)
> > -              * This implies that the 64-bit entry point should have
> > -              * precedence if it is defined and supported by the OS. If we
> > -              * have the 64-bit entry point, but fail to decode it, fall
> > -              * back to the legacy one (if available)
> > -              */
> > -             if (efi.smbios3 != EFI_INVALID_TABLE_ADDR) {
> > -                     p = dmi_early_remap(efi.smbios3, 32);
> > -                     if (p == NULL)
> > -                             goto error;
> > -                     memcpy_fromio(buf, p, 32);
> > -                     dmi_early_unmap(p, 32);
> > -
> > -                     if (!dmi_smbios3_present(buf)) {
> > -                             dmi_available = 1;
> > -                             return;
> > -                     }
> > -             }
> > -             if (efi.smbios == EFI_INVALID_TABLE_ADDR)
> > +             if (dmi_sacn_smbios(efi.smbios3, efi.smbios))
> >                       goto error;
> > -
> > -             /* This is called as a core_initcall() because it isn't
> > -              * needed during early boot.  This also means we can
> > -              * iounmap the space when we're done with it.
> > -              */
> > -             p = dmi_early_remap(efi.smbios, 32);
> > -             if (p == NULL)
> > -                     goto error;
> > -             memcpy_fromio(buf, p, 32);
> > -             dmi_early_unmap(p, 32);
> > -
> > -             if (!dmi_present(buf)) {
> > -                     dmi_available = 1;
> > -                     return;
> > -             }
> > diff --git a/drivers/firmware/ffi.c b/drivers/firmware/ffi.c
> > new file mode 100644
> > index 000000000000..169802b4a7a8
> > --- /dev/null
> > +++ b/drivers/firmware/ffi.c
> > @@ -0,0 +1,36 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/of.h>
> > +#include <linux/of_fdt.h>
> > +#include <linux/libfdt.h>
> > +#include <linux/ffi.h>
> > +
> > +#define FFI_INVALID_TABLE_ADDR       (~0UL)
> > +
> > +struct ffi __read_mostly ffi = {
> > +     .smbios = FFI_INVALID_TABLE_ADDR,
> > +     .smbios3 = FFI_INVALID_TABLE_ADDR,
> > +};
>
> > +EXPORT_SYMBOL(ffi);
>
> > +// SPDX-License-Identifier: GPL-2.0-only
>
> Why not EXPORT_SYMBOL_GPL? But also, who is the user of this export?
Just like efi.

>
> > +
> > +void __init ffi_smbios_root_pointer(void)
> > +{
> > +     int cfgtbl, len;
> > +     fdt64_t *prop;
> > +
> > +     cfgtbl = fdt_path_offset(initial_boot_params, "/cfgtables");
>
> These DT properties need to be documented in a binding.
>
> > +     if (cfgtbl < 0) {
> > +             pr_info("firmware table not found.\n");
>
> Isn't it perfectly valid for a DT not to contain this table? This print
> should be, at the very least, a pr_debug().
>
> > +             return;
> > +     }
> > +     prop = fdt_getprop_w(initial_boot_params, cfgtbl, "smbios_phy_ptr", &len);
>
> Again, undocumented DT property. Please document them in a binding.
Okay, I'll add them into binding.


>
> > +     if (!prop || len != sizeof(u64))
> > +             pr_info("smbios entry point not found.\n");
> > +     else
> > +             ffi.smbios = fdt64_to_cpu(*prop);
> > +
> > +     pr_info("smbios root pointer: %lx\n", ffi.smbios);
>
> ffi.smbios is not set if (!prop || len != sizeof(u64)), looks like your
> "if" should return and the contents of the else become unconditional?
> Otherwise, this print seems wrong.
OK, I will optimize this logic and print.

>
> > +}
> > +
> > diff --git a/include/linux/ffi.h b/include/linux/ffi.h
> > new file mode 100644
> > index 000000000000..95298a805222
> > --- /dev/null
> > +++ b/include/linux/ffi.h
> > @@ -0,0 +1,15 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef _LINUX_FFI_H
> > +#define _LINUX_FFI_H
> > +
> > +extern struct ffi {
> > +     unsigned long smbios;  /* SMBIOS table (32 bit entry point) */
> > +     unsigned long smbios3;  /* SMBIOS table (64 bit entry point) */
> > +     unsigned long flags;
> > +
> > +} ffi;
> > +
> > +void ffi_smbios_root_pointer(void);
>
> Please provide a stub for !FDT_FW_INTERFACE so that we don't need
> ifdeffery at callsites.
OK, update it on v3.



Thanks,
Yunhui