Re: [PATCH 1/3] e820: Don't let unknown DIMM type come out BUSY

[Dan Williams <dan.j.williams@intel.com>]

On Thu, Mar 5, 2015 at 2:20 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
>
> There is something not very nice (Gentlemen nice) In current
> e820.c code.
>
> At Multiple places for example @memblock_x86_fill() it will
> add the different memory resources *except the E820_RESERVED type*
>
> Then at e820_reserve_resources() it will mark all !E820_RESERVED
> as busy.
>
> This is all fine when we have only the known types one of:
>         E820_RESERVED_KERN:
>         E820_RAM:
>         E820_ACPI:
>         E820_NVS:
>         E820_UNUSABLE:
>         E820_RESERVED:
>
> But if the system encounters a brand new memory type it will
> not add it to any memory list, But will proceed to mark it
> BUSY. So now any other Driver in the system that does know
> how to deal with this new type, is not able to call
> request_mem_region_exclusive() on this new type because it is
> hard coded BUSY even though nothing really uses it.
>
> So make any unknown type behave like E820_RESERVED memory,
> it will show up as available to first caller of
> request_mem_region_exclusive().
>
> I Also change the string representation of an unknown type
> from "reserved" (So to not confuse with memmap "reserved"
> region). And call it "reserved-unknown"
> I wish I could return "reserved-type-X" But this is not possible
> because one must return a constant, code-segment, string.
>
> (NOTE: These unknown-types where called "reserved" in
>  /proc/iomem and in dmesg but behaved differently. What this
>  patch does is name them differently but let them behave
>  the same)
>
> By Popular demand An Extra WARNING message is printed if
> an "UNKNOWN" is found. It will look like this:
>   e820: WARNING [mem 0x100000000-0x1ffffffff] is unknown type 12
>
> An example of such "UNKNOWN" type is the not Standard type-12
> DDR3-NvDIMM which is used by multiple vendors for a while
> now. (Estimated 100ds of thousands sold world wide)
>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: x86@kernel.org
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Matthew Wilcox <willy@linux.intel.com>
> CC: Christoph Hellwig <hch@infradead.org>
> CC: Andy Lutomirski <luto@amacapital.net>
> Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
> ---
>  arch/x86/kernel/e820.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 46201de..c3a11cd 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -104,6 +104,21 @@ int __init e820_all_mapped(u64 start, u64 end, unsigned type)
>         return 0;
>  }
>
> +static bool _is_unknown_type(int e820_type)

Any reason for the leading "_"?

> +{
> +       switch (e820_type) {
> +       case E820_RESERVED_KERN:
> +       case E820_RAM:
> +       case E820_ACPI:
> +       case E820_NVS:
> +       case E820_UNUSABLE:
> +       case E820_RESERVED:
> +               return false;
> +       default:
> +               return true;
> +       }
> +}
> +
>  /*
>   * Add a memory region to the kernel e820 map.
>   */
> @@ -119,6 +134,11 @@ static void __init __e820_add_region(struct e820map *e820x, u64 start, u64 size,
>                 return;
>         }
>
> +       if (unlikely(_is_unknown_type(type)))

Unnecessary unlikely()?

> +               pr_warn("e820: WARNING [mem %#010llx-%#010llx] is unknown type %d\n",
> +                      (unsigned long long) start,
> +                      (unsigned long long) (start + size - 1), type);

I still think this warning can go and we can just fold patch 2 into
this one, but other than that this looks ok to me.