From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758819AbbCEUln (ORCPT ); Thu, 5 Mar 2015 15:41:43 -0500 Received: from mail-we0-f170.google.com ([74.125.82.170]:44873 "EHLO mail-we0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754131AbbCEUlk (ORCPT ); Thu, 5 Mar 2015 15:41:40 -0500 MIME-Version: 1.0 In-Reply-To: <54F82DD7.90600@plexistor.com> References: <54F82CE0.4040502@plexistor.com> <54F82DD7.90600@plexistor.com> Date: Thu, 5 Mar 2015 12:41:38 -0800 Message-ID: Subject: Re: [PATCH 1/3] e820: Don't let unknown DIMM type come out BUSY From: Dan Williams To: Boaz Harrosh Cc: Ingo Molnar , X86 ML , linux-kernel , "Roger C. Pao" , Thomas Gleixner , linux-nvdimm , "H. Peter Anvin" , Matthew Wilcox , Andy Lutomirski , Christoph Hellwig , Ross Zwisler Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Mar 5, 2015 at 2:20 AM, Boaz Harrosh 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 > CC: Ingo Molnar > CC: "H. Peter Anvin" > CC: x86@kernel.org > CC: Dan Williams > CC: Matthew Wilcox > CC: Christoph Hellwig > CC: Andy Lutomirski > Signed-off-by: Boaz Harrosh > --- > 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.