Re: [PATCH 01/21] e820, efi: add ACPI 6.0 persistent memory types

[Dan Williams]

On Sun, Apr 19, 2015 at 12:46 AM, Boaz Harrosh <boaz@xxxxxxxxxxxxx> wrote:
> On 04/18/2015 04:35 AM, Dan Williams wrote:
[..]
>> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
>> index 11cc7d54ec3f..410af501a941 100644
>> --- a/arch/x86/kernel/e820.c
>> +++ b/arch/x86/kernel/e820.c
>> @@ -137,6 +137,8 @@ static void __init e820_print_type(u32 type)
>>       case E820_RESERVED_KERN:
>>               printk(KERN_CONT "usable");
>>               break;
>> +     case E820_PMEM:
>> +     case E820_PRAM:
>
> NACK!
>
> This is the most important print in the system and it is a pure
> user Interface. It has no effect what so ever on functionality
> It is to Inform the user through dmesg what is the content of the
> table.

It still describes how the memory is used which is "reserved" for a
driver, I don't see how increasing the verbosity here improves debug
given the alternatives, see below...

>
>>       case E820_RESERVED:
>>               printk(KERN_CONT "reserved");
>>               break;
>> @@ -149,9 +151,6 @@ static void __init e820_print_type(u32 type)
>>       case E820_UNUSABLE:
>>               printk(KERN_CONT "unusable");
>>               break;
>
> +       case E820_PMEM:
>> -     case E820_PRAM:
>> -             printk(KERN_CONT "persistent (type %u)", type);
>> -             break;
>
> Just add the new (7) entry here please. Here Christoph has bike shed
> it for you.

/proc/iomem has these details to differentiate PRAM and PMEM as well
as show which driver(s)/device(s) have claimed the range(s).

>>       default:
>>               printk(KERN_CONT "type %u", type);
>>               break;

Here is where you can see undefined/unknown types.

>> @@ -919,10 +918,26 @@ static inline const char *e820_type_to_string(int e820_type)
>>       case E820_NVS:  return "ACPI Non-volatile Storage";
>>       case E820_UNUSABLE:     return "Unusable memory";
>>       case E820_PRAM: return "Persistent RAM";
>> +     case E820_PMEM: return "Persistent I/O Memory";
>>       default:        return "reserved";
>>       }
>>  }
>>
>> +static bool do_mark_busy(u32 type, struct resource *res)
>> +{
>> +     if (res->start < (1ULL<<20))
>> +             return true;
>> +
>> +     switch (type) {
>> +     case E820_RESERVED:
>> +     case E820_PRAM:
>> +     case E820_PMEM:
>> +             return false;
>> +     default:
>> +             return true;
>> +     }
>
> Sigh. Again an unknown type comes out busy. Busy means
> resource used. It does *not* mean "unknown type".
>
> It just forces researchers to ignore the return value of
> request_region. And not be protected by double lock. It
> does not really prevent anything

You're free to submit a standalone patch to change this policy... see
the new "OEM-reserved" memory types in ACPI 6.

That said, I think we're better off with the current policy.  If
unknown memory types were treated as permanently-busy back when we
initially started experimenting with NVDIMM support (2010) then I
doubt the e820-type-12 prototype would ever have escaped the lab.  We
could have avoided a good amount of confusion.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/