[External] Re: [PATCH v2 2/3] firmware: introduce FFI for SMBIOS entry.
运辉崔
cuiyunhui at bytedance.com
Mon Jul 3 01:23:53 PDT 2023
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 at 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.
>
> > EXTERNAL CONNECTOR SUBSYSTEM (EXTCON)
> > M: MyungJoo Ham <myungjoo.ham at samsung.com>
> > M: Chanwoo Choi <cw00.choi at samsung.com>
> > 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
More information about the linux-riscv
mailing list