* Borislav Petkov <[email protected]> wrote:
> From: "Chen, Gong" <[email protected]>
> diff --git a/arch/x86/include/uapi/asm/mce.h b/arch/x86/include/uapi/asm/mce.h
> index a0eab85ce7b8..76880ede9a35 100644
> --- a/arch/x86/include/uapi/asm/mce.h
> +++ b/arch/x86/include/uapi/asm/mce.h
> @@ -15,7 +15,8 @@ struct mce {
> __u64 time; /* wall time_t when error was detected */
> __u8 cpuvendor; /* cpu vendor as encoded in system.h */
> __u8 inject_flags; /* software inject flags */
> - __u16 pad;
> + __u8 severity;
> + __u8 usable_addr;
> __u32 cpuid; /* CPUID 1 EAX */
> __u8 cs; /* code segment */
> __u8 bank; /* machine check bank */
So this change appears to be completely unrelated to the stated purpose of this
patch?
> +/*
> + * printk() is not safe in MCE context. This is a lock-less memory allocator
> + * used to save error information organized in a lock-less list.
> + *
> + * This memory pool is only to be used to save MCE records in MCE context.
> + * MCE events are rare so a fixed size memory pool should be enough. Use
Missing comma.
> + * 2 pages to save MCE events for now (~80 MCE records at most).
> + */
> +#define MCE_POOLSZ (2 * PAGE_SIZE)
> +bool mce_genpool_add(struct mce *mce)
> +{
> + struct mce_evt_llist *node;
> +
> + if (!mce_evt_pool)
> + return false;
> +
> + node = (void *)gen_pool_alloc(mce_evt_pool, sizeof(*node));
> + if (!node) {
> + pr_warn_ratelimited("MCE records pool full!\n");
> + return false;
> + }
> +
> + memcpy(&node->mce, mce, sizeof(*mce));
> + llist_add(&node->llnode, &mce_event_llist);
> +
> + return true;
> +}
So I think the standard pattern for allocation failures with integer types is
to
return -ENOMEM, not bool. This really matters, because:
> +
> +static int mce_genpool_create(void)
> +{
> + struct gen_pool *tmpp;
> + int ret = -ENOMEM;
> +
> + tmpp = gen_pool_create(ilog2(sizeof(struct mce_evt_llist)), -1);
> + if (!tmpp)
> + goto out;
> +
> + ret = gen_pool_add(tmpp, (unsigned long)genpool_buf, MCE_POOLSZ, -1);
> + if (ret) {
> + gen_pool_destroy(tmpp);
> + goto out;
here gen_pool_add() has an inverted logic, and they looks confusing.
Furthermore, why do we spell it 'mce_genpool' if the generic facility is
spelling
it gen_pool?
Also, I'm questioning the whole premise of the patches:
> +/*
> + * printk() is not safe in MCE context. This is a lock-less memory allocator
> + * used to save error information organized in a lock-less list.
> + *
> + * This memory pool is only to be used to save MCE records in MCE context.
> + * MCE events are rare so a fixed size memory pool should be enough. Use
So how are we going to report uncorrectable errors that forcibly crash/panic
the
system if we cannot use printk? How will the admin learn what was amiss?
Thanks,
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
