Re: [PATCH 1/4] powerpc/prom: avoid endian conversions for linux, memory-limit node

From: Hari Bathini <hbathini@linux.vnet.ibm.com>
To: Michael Ellerman <mpe@ellerman.id.au>,
	linuxppc-dev <linuxppc-dev@ozlabs.org>
Cc: Paul Mackerras <paulus@samba.org>,
	sjitindarsingh@gmail.com, ben@codiert.org
Subject: Re: [PATCH 1/4] powerpc/prom: avoid endian conversions for linux, memory-limit node
Date: Sat, 5 Aug 2017 00:08:42 +0530	[thread overview]
Message-ID: <30c461a6-8cef-cad2-d4a1-cda068c1c2dc@linux.vnet.ibm.com> (raw)
In-Reply-To: <87y3qzprlc.fsf@concordia.ellerman.id.au>

On Friday 04 August 2017 03:44 PM, Michael Ellerman wrote:
> Hari Bathini <hbathini@linux.vnet.ibm.com> writes:
>
>> On Friday 04 August 2017 09:21 AM, Michael Ellerman wrote:
>>> Hari Bathini <hbathini@linux.vnet.ibm.com> writes:
>>>
>>>> As linux,memory-limit node is set and also later used by the kernel,
>>>> avoid endian conversions for this property.
>>>>
>>>> Fixes: 493adffcb43f ("powerpc: Make prom_init.c endian safe")
>>>> Cc: stable@vger.kernel.org # 3.12+
>>>> Cc: Anton Blanchard <anton@ozlabs.org>
>>>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com>
>>>> ---
>>>>    arch/powerpc/kernel/prom_init.c |    3 +--
>>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>> As Ben said, this is not OK. The flat device tree is a data
>>> structure with a specified format[1], we don't violate the spec just to
>>> avoid an endian swap.
>>>
>>> Is there an actual bug you're trying to solve?
>> Yep. While retrieving this property in prom.c, no endian conversion is
>> being done.
>> It was broken for a while. Let me do the endian swap in prom.c while
>> retrieving..
> Does it actually not work though, mem=x on the command line?

mem=X works fine. The problem is with the early cmdline parsing of
'mem=' in prom_init, which is treating fadump_reserve_mem=X as
mem=X. So, when fadump_reserve_mem=X is passed, endian swapped
version of X is set to memory_limit as early parser takes it for mem=X
and linux,memory-limit read is not endian safe currently. This bug
was not hit so far as prom_memory_limit is set only when X is
< ram_top && > alloc_bottom which is not the case generally.

> I think that code in prom.c is basically dead code, it's still there
> because we were afraid removing it would break something. These days we
> parse the command line early enough that we don't need those properties.
>
>

This problem is not seen with mem=X as memory_limit is overwritten
with the right value as soon as parse_early_param() is called in prom.

Should I just get rid of linux,memory-limit node and mem=X handling
from early_cmdline_parse() in prom_init as this has been broken for
a while and nobody seem to have had a problem with that?

Thanks
Hari