Messages in this thread | | | From | Andy Shevchenko <> | Date | Sat, 9 Jun 2018 21:57:19 +0300 | Subject | Re: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table |
| |
On Sat, Jun 9, 2018 at 4:04 AM, Darren Hart <dvhart@infradead.org> wrote: > On Thu, Jun 07, 2018 at 08:11:41PM +0300, Andy Shevchenko wrote: >> On Thu, Jun 7, 2018 at 6:59 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
>> > +static inline struct smm_eps_table *check_eps_table(u8 *addr) >> > +{ >> > + struct smm_eps_table *eps = (struct smm_eps_table *)addr; >> > + >> >> > + if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0) >> >> I'm not sure about strings operation here. >> I would rather do like with other magic constants: introduce hex value >> and compare it as unsigned integer. >> >> Also, it might be a warning, since \0 wasn't ever checked from the >> string literal. Though, I'm not sure if it applicable to strncmp() >> function (it's for strncpy for sure). > > I think we're OK here, and we're being consistent with the > dell-wmi-descriptor test for "DELL WMI". I don't recall if it was that > one or something else, but doing it in HEX ended up being more > confusing. The \0 isn't an issue since strncmp will only compare the n > (4) bytes.
Yeah, consistency is usually a priority over style. Though in the context below I see also magic numbers instead of strings. I have no strong opinion here which one is better to follow.
>> Also I've a feeling that 0xf0000 constant is defined already somewhere >> under arch/x86/include/asm or evem include/linux. > > But... is it defined for this purpose? If not, I'd prefer it hardcoded > (or with a DEFINE).
OK, I did a research, and the following is the result.
arch/x86/kernel/mpparse.c:636: smp_scan_config(0xF0000, 0x10000)) arch/x86/kernel/probe_roms.c:27: .start = 0xf0000,
arch/x86/pci/irq.c:91: * Search 0xf0000 -- 0xfffff for the PCI IRQ Routing Table. arch/x86/pci/irq.c:105: for (addr = (u8 *) __va(0xf0000); addr < (u8 *) __va(0x100000); addr += 16) {
arch/x86/platform/geode/alix.c:34:#define BIOS_SIGNATURE_TINYBIOS 0xf0000 arch/x86/platform/olpc/olpc-xo1-pm.c:36:} ofw_bios_entry = { 0xF0000 + PAGE_OFFSET, __KERNEL_CS }; arch/x86/platform/ts5500/ts5500.c:103: bios = ioremap(0xf0000, 0x10000);
Perhaps it would be a good idea to introduce such constant in the future.
Note mpparse.c and irq.c, that's similar purpose of the address range.
>> > #define SMI_CMD_MAGIC (0x534D4931) >> > >> > +#define SMM_EPS_SIG "$SCB" >> >> Just integer like above and put the sting as a comment. >> (Side note: above magic also looks like string) > > Given the above, I think we can use the more recognizable string - since > that is clearly how they think of this label.
OK with me!
-- With Best Regards, Andy Shevchenko
| |