linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* Regression (ARM) arch/arm/mm/init.c doesn't build without CONFIG_ARM_PATCH_PHYS_VIRT.
@ 2013-12-31 11:20 Krzysztof Hałasa
  2013-12-31 11:23 ` Russell King - ARM Linux
  2014-01-06 19:28 ` Krzysztof Hałasa
  0 siblings, 2 replies; 13+ messages in thread
From: Krzysztof Hałasa @ 2013-12-31 11:20 UTC (permalink / raw)
  To: linux-arm-kernel

arch/arm/mm/init.c: In function 'setup_dma_zone':
arch/arm/mm/init.c:232:19: error: '__pv_phys_offset' undeclared (first use in this function)

Reverting the following commit fixes it:

commit 787b0d5c1ca7ff24feb6f92e4c7f4410ee7d81a8
Author: Santosh Shilimkar <santosh.shilimkar@ti.com>
Date:   Mon Dec 2 20:29:12 2013 +0100

    ARM: 7908/1: mm: Fix the arm_dma_limit calculation

    Current code is using PHYS_OFFSET to calculate the arm_dma_limit which
    will lead to wrong calculations in cases where PHYS_OFFSET is updated
    runtime.

    So fix the code by using __pv_phys_offset instead of PHYS_OFFSET.

It seems PHYS_OFFSET is equal to __pv_phys_offset if the latter is
available:

arch/arm/include/asm/memory.h:#define PHYS_OFFSET __pv_phys_offset

Otherwise (without CONFIG_ARM_PATCH_PHYS_VIRT) PHYS_OFFSET is a
hard-coded CONFIG value, or whatever the platform/CPU code needs:

arch/arm/include/asm/memory.h:#define PHYS_OFFSET     PLAT_PHYS_OFFSET

Perhaps the patch in question was needed at some point but I think the
situation had changed before it was commited.
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Regression (ARM) arch/arm/mm/init.c doesn't build without CONFIG_ARM_PATCH_PHYS_VIRT.
  2013-12-31 11:20 Regression (ARM) arch/arm/mm/init.c doesn't build without CONFIG_ARM_PATCH_PHYS_VIRT Krzysztof Hałasa
@ 2013-12-31 11:23 ` Russell King - ARM Linux
  2014-01-01 14:40   ` Krzysztof Hałasa
  2014-01-06 19:28 ` Krzysztof Hałasa
  1 sibling, 1 reply; 13+ messages in thread
From: Russell King - ARM Linux @ 2013-12-31 11:23 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Dec 31, 2013 at 12:20:56PM +0100, Krzysztof Ha?asa wrote:
> arch/arm/mm/init.c: In function 'setup_dma_zone':
> arch/arm/mm/init.c:232:19: error: '__pv_phys_offset' undeclared (first use in this function)
> 
> Reverting the following commit fixes it:

Already fixed long before Christmas.  Please use recent kernels.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Regression (ARM) arch/arm/mm/init.c doesn't build without CONFIG_ARM_PATCH_PHYS_VIRT.
  2013-12-31 11:23 ` Russell King - ARM Linux
@ 2014-01-01 14:40   ` Krzysztof Hałasa
  0 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Hałasa @ 2014-01-01 14:40 UTC (permalink / raw)
  To: linux-arm-kernel

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

> On Tue, Dec 31, 2013 at 12:20:56PM +0100, Krzysztof Ha?asa wrote:
>> arch/arm/mm/init.c: In function 'setup_dma_zone':
>> arch/arm/mm/init.c:232:19: error: '__pv_phys_offset' undeclared (first use in this function)
>> 
>> Reverting the following commit fixes it:
>
> Already fixed long before Christmas.  Please use recent kernels.

Do you mean something other than Linus's tree? The problem I noted
is present in his tree as of Dec 30 - on both 3.13-rc6 and (then tip)
71ce176ee6ed1735b9a1160a5704a915d13849b1.

Or... do you mean the commit that added this regression?
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Regression (ARM) arch/arm/mm/init.c doesn't build without CONFIG_ARM_PATCH_PHYS_VIRT.
  2013-12-31 11:20 Regression (ARM) arch/arm/mm/init.c doesn't build without CONFIG_ARM_PATCH_PHYS_VIRT Krzysztof Hałasa
  2013-12-31 11:23 ` Russell King - ARM Linux
@ 2014-01-06 19:28 ` Krzysztof Hałasa
  2014-01-06 19:33   ` Santosh Shilimkar
  2014-01-06 22:39   ` Russell King - ARM Linux
  1 sibling, 2 replies; 13+ messages in thread
From: Krzysztof Hałasa @ 2014-01-06 19:28 UTC (permalink / raw)
  To: linux-arm-kernel

Russell, Santosh,

the unneeded commit causing regression is still in place. Please try to
compile an ARM kernel without CONFIG_ARM_PATCH_PHYS_VIRT and with
CONFIG_ZONE_DMA and see for yourself, if you don't believe me.

Please be aware that this commit fixes nothing, its only function is
causing the regression - so we don't lose anything by reverting it.

If the attached wasn't clear, what the defective commit presently does
is changing a perfectly valid code into a code referencing a variable
which (without CONFIG_ARM_PATCH_PHYS_VIRT set) doesn't at all exist.

With CONFIG_ARM_PATCH_PHYS_VIRT set, this commit does precisely nothing.

grep __pv_phys_offset * -r gives:

Broken by the commit in question:

arch/arm/mm/init.c:             arm_dma_limit = __pv_phys_offset + arm_dma_zone_size - 1;

All of the following are only compiled/assembled with
CONFIG_ARM_PATCH_PHYS_VIRT set:

arch/arm/include/asm/memory.h:extern u64 __pv_phys_offset;
arch/arm/include/asm/memory.h:#define PHYS_OFFSET __pv_phys_offset
arch/arm/kernel/armksyms.c:EXPORT_SYMBOL(__pv_phys_offset);
arch/arm/kernel/head.S: add     r6, r6, r3      @ adjust
__pv_phys_offset address
arch/arm/kernel/head.S: str     r8, [r6, #LOW_OFFSET]   @ save computed
PHYS_OFFSET to __pv_phys_offset
arch/arm/kernel/head.S:2:       .long   __pv_phys_offset
arch/arm/kernel/head.S: .globl  __pv_phys_offset
arch/arm/kernel/head.S: .type   __pv_phys_offset, %object
arch/arm/kernel/head.S:__pv_phys_offset:
arch/arm/kernel/head.S: .size   __pv_phys_offset, . -__pv_phys_offset

In short, please revert 787b0d5c1ca7ff24feb6f92e4c7f4410ee7d81a8.
Or, do you want me to send a patch which does that?

HTH and TIA.

> arch/arm/mm/init.c: In function 'setup_dma_zone':
> arch/arm/mm/init.c:232:19: error: '__pv_phys_offset' undeclared (first use in this function)
>
> Reverting the following commit fixes it:
>
> commit 787b0d5c1ca7ff24feb6f92e4c7f4410ee7d81a8
> Author: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Date:   Mon Dec 2 20:29:12 2013 +0100
>
>     ARM: 7908/1: mm: Fix the arm_dma_limit calculation
>
>     Current code is using PHYS_OFFSET to calculate the arm_dma_limit which
>     will lead to wrong calculations in cases where PHYS_OFFSET is updated
>     runtime.
>
>     So fix the code by using __pv_phys_offset instead of PHYS_OFFSET.
>
> It seems PHYS_OFFSET is equal to __pv_phys_offset if the latter is
> available:
>
> arch/arm/include/asm/memory.h:#define PHYS_OFFSET __pv_phys_offset
>
> Otherwise (without CONFIG_ARM_PATCH_PHYS_VIRT) PHYS_OFFSET is a
> hard-coded CONFIG value, or whatever the platform/CPU code needs:
>
> arch/arm/include/asm/memory.h:#define PHYS_OFFSET     PLAT_PHYS_OFFSET
>
> Perhaps the patch in question was needed at some point but I think the
> situation had changed before it was commited.
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Regression (ARM) arch/arm/mm/init.c doesn't build without CONFIG_ARM_PATCH_PHYS_VIRT.
  2014-01-06 19:28 ` Krzysztof Hałasa
@ 2014-01-06 19:33   ` Santosh Shilimkar
  2014-01-06 22:08     ` Krzysztof Hałasa
  2014-01-06 22:39   ` Russell King - ARM Linux
  1 sibling, 1 reply; 13+ messages in thread
From: Santosh Shilimkar @ 2014-01-06 19:33 UTC (permalink / raw)
  To: linux-arm-kernel

On Monday 06 January 2014 02:28 PM, Krzysztof Ha?asa wrote:
> Russell, Santosh,
> 
> the unneeded commit causing regression is still in place. Please try to
> compile an ARM kernel without CONFIG_ARM_PATCH_PHYS_VIRT and with
> CONFIG_ZONE_DMA and see for yourself, if you don't believe me.
> 
> Please be aware that this commit fixes nothing, its only function is
> causing the regression - so we don't lose anything by reverting it.
> 
I am afraid you didn't understood what the fix is if you say above.
arm_dma_limit is broken without this fix for LPAE machines with
memory starting 4 GB physical boundary.

Regards,
Santosh

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Regression (ARM) arch/arm/mm/init.c doesn't build without CONFIG_ARM_PATCH_PHYS_VIRT.
  2014-01-06 19:33   ` Santosh Shilimkar
@ 2014-01-06 22:08     ` Krzysztof Hałasa
  2014-01-06 22:27       ` Santosh Shilimkar
  0 siblings, 1 reply; 13+ messages in thread
From: Krzysztof Hałasa @ 2014-01-06 22:08 UTC (permalink / raw)
  To: linux-arm-kernel

Santosh Shilimkar <santosh.shilimkar@ti.com> writes:

> I am afraid you didn't understood what the fix is if you say above.
> arm_dma_limit is broken without this fix for LPAE machines with
> memory starting 4 GB physical boundary.

I wonder what CONFIG_ARM_PATCH_PHYS_VIRT and CONFIG_ZONE_DMA do they
use?

Of course, CONFIG_ZONE_DMA must be set, because otherwise the patch
changes a part disabled with #ifdef. CONFIG_ARM_PATCH_PHYS_VIRT must be
set because otherwise the kernel will not compile at all.

Do you accept the fact the configurations with CONFIG_ZONE_DMA and
without CONFIG_ARM_PATCH_PHYS_VIRT are now broken - by this very commit?

Now, what "fix" does this commit in its entirety do:

-     arm_dma_limit = PHYS_OFFSET + arm_dma_zone_size - 1;
+     arm_dma_limit = __pv_phys_offset + arm_dma_zone_size - 1;

if under the above circumstances (CONFIG_*) in
arch/arm/include/asm/memory.h we have:

extern u64 __pv_phys_offset;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the only usable declaration of this variable
extern u64 __pv_offset;
extern void fixup_pv_table(const void *, unsigned long);
extern const void *__pv_table_begin, *__pv_table_end;

#define PHYS_OFFSET __pv_phys_offset
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Regression (ARM) arch/arm/mm/init.c doesn't build without CONFIG_ARM_PATCH_PHYS_VIRT.
  2014-01-06 22:08     ` Krzysztof Hałasa
@ 2014-01-06 22:27       ` Santosh Shilimkar
  0 siblings, 0 replies; 13+ messages in thread
From: Santosh Shilimkar @ 2014-01-06 22:27 UTC (permalink / raw)
  To: linux-arm-kernel

On Monday 06 January 2014 05:08 PM, Krzysztof Ha?asa wrote:
> Santosh Shilimkar <santosh.shilimkar@ti.com> writes:
> 
>> I am afraid you didn't understood what the fix is if you say above.
>> arm_dma_limit is broken without this fix for LPAE machines with
>> memory starting 4 GB physical boundary.
> 
> I wonder what CONFIG_ARM_PATCH_PHYS_VIRT and CONFIG_ZONE_DMA do they
> use?
> 
You need to grep where the CONFIG_ARM_PATCH_PHYS_VIRT is used ;)

> Of course, CONFIG_ZONE_DMA must be set, because otherwise the patch
> changes a part disabled with #ifdef. CONFIG_ARM_PATCH_PHYS_VIRT must be
> set because otherwise the kernel will not compile at all.
> 
> Do you accept the fact the configurations with CONFIG_ZONE_DMA and
> without CONFIG_ARM_PATCH_PHYS_VIRT are now broken - by this very commit?
> 
> Now, what "fix" does this commit in its entirety do:
> 
> -     arm_dma_limit = PHYS_OFFSET + arm_dma_zone_size - 1;
> +     arm_dma_limit = __pv_phys_offset + arm_dma_zone_size - 1;
> 
> if under the above circumstances (CONFIG_*) in
> arch/arm/include/asm/memory.h we have:
> 
> extern u64 __pv_phys_offset;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the only usable declaration of this variable
> extern u64 __pv_offset;
> extern void fixup_pv_table(const void *, unsigned long);
> extern const void *__pv_table_begin, *__pv_table_end;
> 
> #define PHYS_OFFSET __pv_phys_offset
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
Your grep actually missed the assembly files where the actual patching
happens. Please have a look at "arch/arm/kernel/head.S"

The "CONFIG_ARM_PATCH_PHYS_VIRT" is always enabled on almost all ARM builds
since its needed for multi-config kernel to work. May be its time to
make it default enabled but I let Russell comment if there is any
better way to handle this.

Regards,
Santosh

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Regression (ARM) arch/arm/mm/init.c doesn't build without CONFIG_ARM_PATCH_PHYS_VIRT.
  2014-01-06 19:28 ` Krzysztof Hałasa
  2014-01-06 19:33   ` Santosh Shilimkar
@ 2014-01-06 22:39   ` Russell King - ARM Linux
  2014-01-06 23:42     ` Santosh Shilimkar
  1 sibling, 1 reply; 13+ messages in thread
From: Russell King - ARM Linux @ 2014-01-06 22:39 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jan 06, 2014 at 08:28:20PM +0100, Krzysztof Ha?asa wrote:
> Russell, Santosh,
> 
> the unneeded commit causing regression is still in place. Please try to
> compile an ARM kernel without CONFIG_ARM_PATCH_PHYS_VIRT and with
> CONFIG_ZONE_DMA and see for yourself, if you don't believe me.
> 
> Please be aware that this commit fixes nothing, its only function is
> causing the regression - so we don't lose anything by reverting it.
> 
> If the attached wasn't clear, what the defective commit presently does
> is changing a perfectly valid code into a code referencing a variable
> which (without CONFIG_ARM_PATCH_PHYS_VIRT set) doesn't at all exist.
> 
> With CONFIG_ARM_PATCH_PHYS_VIRT set, this commit does precisely nothing.

Right, so, with Assabet, which has CONFIG_DMA_ZONE=y and
CONFIG_ARM_PATCH_PHYS_VIRT=y:

$ make O=../build/assabet arch/arm/mm/init.i

gives:
  arm_dma_limit = __pv_phys_offset + arm_dma_zone_size - 1;

with or without Santosh's patch.  So, with V2P patching in place, there's
absolutely no functional difference.

With CONFIG_ARM_PATCH_PHYS_VIRT=n, before Santosh's patch:

  arm_dma_limit = (0xc0000000UL) + arm_dma_zone_size - 1;

After:

  arm_dma_limit = __pv_phys_offset + arm_dma_zone_size - 1;

and this breaks the build because there is no __pv_phys_offset symbol.

Now, the case which matters for Santosh is the first case - the one
where CONFIG_ARM_PATCH_PHYS_VIRT=y.  We can clearly see that after
preprocessing, the results are 100% identical.

Therefore, I find myself agreeing with Krzysztof that the commit is
bad, has no functional change for the case it was proposed to solve,
and needs to be reverted.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Regression (ARM) arch/arm/mm/init.c doesn't build without CONFIG_ARM_PATCH_PHYS_VIRT.
  2014-01-06 22:39   ` Russell King - ARM Linux
@ 2014-01-06 23:42     ` Santosh Shilimkar
  2014-01-07  1:11       ` Russell King - ARM Linux
  0 siblings, 1 reply; 13+ messages in thread
From: Santosh Shilimkar @ 2014-01-06 23:42 UTC (permalink / raw)
  To: linux-arm-kernel

On Monday 06 January 2014 05:39 PM, Russell King - ARM Linux wrote:
> On Mon, Jan 06, 2014 at 08:28:20PM +0100, Krzysztof Ha?asa wrote:
>> Russell, Santosh,
>>
>> the unneeded commit causing regression is still in place. Please try to
>> compile an ARM kernel without CONFIG_ARM_PATCH_PHYS_VIRT and with
>> CONFIG_ZONE_DMA and see for yourself, if you don't believe me.
>>
>> Please be aware that this commit fixes nothing, its only function is
>> causing the regression - so we don't lose anything by reverting it.
>>
>> If the attached wasn't clear, what the defective commit presently does
>> is changing a perfectly valid code into a code referencing a variable
>> which (without CONFIG_ARM_PATCH_PHYS_VIRT set) doesn't at all exist.
>>
>> With CONFIG_ARM_PATCH_PHYS_VIRT set, this commit does precisely nothing.
> 
> Right, so, with Assabet, which has CONFIG_DMA_ZONE=y and
> CONFIG_ARM_PATCH_PHYS_VIRT=y:
> 
> $ make O=../build/assabet arch/arm/mm/init.i
> 
> gives:
>   arm_dma_limit = __pv_phys_offset + arm_dma_zone_size - 1;
> 
> with or without Santosh's patch.  So, with V2P patching in place, there's
> absolutely no functional difference.
> 
> With CONFIG_ARM_PATCH_PHYS_VIRT=n, before Santosh's patch:
> 
>   arm_dma_limit = (0xc0000000UL) + arm_dma_zone_size - 1;
> 
> After:
> 
>   arm_dma_limit = __pv_phys_offset + arm_dma_zone_size - 1;
> 
> and this breaks the build because there is no __pv_phys_offset symbol.
> 
> Now, the case which matters for Santosh is the first case - the one
> where CONFIG_ARM_PATCH_PHYS_VIRT=y.  We can clearly see that after
> preprocessing, the results are 100% identical.
> 
> Therefore, I find myself agreeing with Krzysztof that the commit is
> bad, has no functional change for the case it was proposed to solve,
> and needs to be reverted.
> 
May be I missed your point but I ended up creating the patch because
the CMA reservation was failing on Keystone since the arm_dma_limit
wasn't right since it wasn't considering the actual __pv_phys_offset.

Isn't that an issue ?

Regards,
Santosh

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Regression (ARM) arch/arm/mm/init.c doesn't build without CONFIG_ARM_PATCH_PHYS_VIRT.
  2014-01-06 23:42     ` Santosh Shilimkar
@ 2014-01-07  1:11       ` Russell King - ARM Linux
  2014-01-07 17:45         ` Santosh Shilimkar
  0 siblings, 1 reply; 13+ messages in thread
From: Russell King - ARM Linux @ 2014-01-07  1:11 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jan 06, 2014 at 06:42:13PM -0500, Santosh Shilimkar wrote:
> On Monday 06 January 2014 05:39 PM, Russell King - ARM Linux wrote:
> > On Mon, Jan 06, 2014 at 08:28:20PM +0100, Krzysztof Ha?asa wrote:
> >> Russell, Santosh,
> >>
> >> the unneeded commit causing regression is still in place. Please try to
> >> compile an ARM kernel without CONFIG_ARM_PATCH_PHYS_VIRT and with
> >> CONFIG_ZONE_DMA and see for yourself, if you don't believe me.
> >>
> >> Please be aware that this commit fixes nothing, its only function is
> >> causing the regression - so we don't lose anything by reverting it.
> >>
> >> If the attached wasn't clear, what the defective commit presently does
> >> is changing a perfectly valid code into a code referencing a variable
> >> which (without CONFIG_ARM_PATCH_PHYS_VIRT set) doesn't at all exist.
> >>
> >> With CONFIG_ARM_PATCH_PHYS_VIRT set, this commit does precisely nothing.
> > 

/--------------------------------------------------------------
| > > Right, so, with Assabet, which has CONFIG_DMA_ZONE=y and
| > > CONFIG_ARM_PATCH_PHYS_VIRT=y:
| > > 
| > > $ make O=../build/assabet arch/arm/mm/init.i
| > > 
| > > gives:
| > >   arm_dma_limit = __pv_phys_offset + arm_dma_zone_size - 1;
| > > 
| > > with or without Santosh's patch.
\--------------------------------------------------------------

> May be I missed your point but I ended up creating the patch because
> the CMA reservation was failing on Keystone since the arm_dma_limit
> wasn't right since it wasn't considering the actual __pv_phys_offset.
> 
> Isn't that an issue ?

See the above.  Your patch has _no_ effect what so ever when
CONFIG_ARM_PATCH_PHYS_VIRT=y - which you have on the Keystone, right?

If you don't believe me, ask make to produce arch/arm/mm/init.i, which
is the output from the preprocessor, comparing the resulting generated
file both with and without your patch applied.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Regression (ARM) arch/arm/mm/init.c doesn't build without CONFIG_ARM_PATCH_PHYS_VIRT.
  2014-01-07  1:11       ` Russell King - ARM Linux
@ 2014-01-07 17:45         ` Santosh Shilimkar
  2014-01-07 17:55           ` Russell King - ARM Linux
  0 siblings, 1 reply; 13+ messages in thread
From: Santosh Shilimkar @ 2014-01-07 17:45 UTC (permalink / raw)
  To: linux-arm-kernel

On Monday 06 January 2014 08:11 PM, Russell King - ARM Linux wrote:
> On Mon, Jan 06, 2014 at 06:42:13PM -0500, Santosh Shilimkar wrote:
>> On Monday 06 January 2014 05:39 PM, Russell King - ARM Linux wrote:
>>> On Mon, Jan 06, 2014 at 08:28:20PM +0100, Krzysztof Ha?asa wrote:
>>>> Russell, Santosh,
>>>>
>>>> the unneeded commit causing regression is still in place. Please try to
>>>> compile an ARM kernel without CONFIG_ARM_PATCH_PHYS_VIRT and with
>>>> CONFIG_ZONE_DMA and see for yourself, if you don't believe me.
>>>>
>>>> Please be aware that this commit fixes nothing, its only function is
>>>> causing the regression - so we don't lose anything by reverting it.
>>>>
>>>> If the attached wasn't clear, what the defective commit presently does
>>>> is changing a perfectly valid code into a code referencing a variable
>>>> which (without CONFIG_ARM_PATCH_PHYS_VIRT set) doesn't at all exist.
>>>>
>>>> With CONFIG_ARM_PATCH_PHYS_VIRT set, this commit does precisely nothing.
>>>
> 
> /--------------------------------------------------------------
> | > > Right, so, with Assabet, which has CONFIG_DMA_ZONE=y and
> | > > CONFIG_ARM_PATCH_PHYS_VIRT=y:
> | > > 
> | > > $ make O=../build/assabet arch/arm/mm/init.i
> | > > 
> | > > gives:
> | > >   arm_dma_limit = __pv_phys_offset + arm_dma_zone_size - 1;
> | > > 
> | > > with or without Santosh's patch.
> \--------------------------------------------------------------
> 
>> May be I missed your point but I ended up creating the patch because
>> the CMA reservation was failing on Keystone since the arm_dma_limit
>> wasn't right since it wasn't considering the actual __pv_phys_offset.
>>
>> Isn't that an issue ?
> 
> See the above.  Your patch has _no_ effect what so ever when
> CONFIG_ARM_PATCH_PHYS_VIRT=y - which you have on the Keystone, right?
> 
> If you don't believe me, ask make to produce arch/arm/mm/init.i, which
> is the output from the preprocessor, comparing the resulting generated
> file both with and without your patch applied.
> 
Looks like you are right. I did two fixes to have right arm_dma_limit in
below order.

1. 787b0d5 {ARM: 7908/1: mm: Fix the arm_dma_limit calculation}
2. 7c92732 {ARM: 7909/1: mm: Call setup_dma_zone() post early_paging_init()}

But with 2 alone the issue gets fixed since with ARM_PATCH_PHYS_VIRT and
below pre-processor, the PHYS_OFFSET also gets an updated value. I never
realised that 1 becomes redundant after second patch while creating them. 

#define PHYS_OFFSET __pv_phys_offset

So indeed, 787b0d5{ARM: 7908/1: mm: Fix the arm_dma_limit calculation}
won't be needed anymore and can be reverted. Sorry it took some time
for me to reach to your conclusion.

Regards,
Santosh

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Regression (ARM) arch/arm/mm/init.c doesn't build without CONFIG_ARM_PATCH_PHYS_VIRT.
  2014-01-07 17:45         ` Santosh Shilimkar
@ 2014-01-07 17:55           ` Russell King - ARM Linux
  2014-01-08  6:40             ` Krzysztof Hałasa
  0 siblings, 1 reply; 13+ messages in thread
From: Russell King - ARM Linux @ 2014-01-07 17:55 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Jan 07, 2014 at 12:45:42PM -0500, Santosh Shilimkar wrote:
> On Monday 06 January 2014 08:11 PM, Russell King - ARM Linux wrote:
> > /--------------------------------------------------------------
> > | > > Right, so, with Assabet, which has CONFIG_DMA_ZONE=y and
> > | > > CONFIG_ARM_PATCH_PHYS_VIRT=y:
> > | > > 
> > | > > $ make O=../build/assabet arch/arm/mm/init.i
> > | > > 
> > | > > gives:
> > | > >   arm_dma_limit = __pv_phys_offset + arm_dma_zone_size - 1;
> > | > > 
> > | > > with or without Santosh's patch.
> > \--------------------------------------------------------------
> > 
> >> May be I missed your point but I ended up creating the patch because
> >> the CMA reservation was failing on Keystone since the arm_dma_limit
> >> wasn't right since it wasn't considering the actual __pv_phys_offset.
> >>
> >> Isn't that an issue ?
> > 
> > See the above.  Your patch has _no_ effect what so ever when
> > CONFIG_ARM_PATCH_PHYS_VIRT=y - which you have on the Keystone, right?
> > 
> > If you don't believe me, ask make to produce arch/arm/mm/init.i, which
> > is the output from the preprocessor, comparing the resulting generated
> > file both with and without your patch applied.
> > 
> Looks like you are right. I did two fixes to have right arm_dma_limit in
> below order.
> 
> 1. 787b0d5 {ARM: 7908/1: mm: Fix the arm_dma_limit calculation}
> 2. 7c92732 {ARM: 7909/1: mm: Call setup_dma_zone() post early_paging_init()}
> 
> But with 2 alone the issue gets fixed since with ARM_PATCH_PHYS_VIRT and
> below pre-processor, the PHYS_OFFSET also gets an updated value. I never
> realised that 1 becomes redundant after second patch while creating them. 
> 
> #define PHYS_OFFSET __pv_phys_offset
> 
> So indeed, 787b0d5{ARM: 7908/1: mm: Fix the arm_dma_limit calculation}
> won't be needed anymore and can be reverted. Sorry it took some time
> for me to reach to your conclusion.

Okay, reverted.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Regression (ARM) arch/arm/mm/init.c doesn't build without CONFIG_ARM_PATCH_PHYS_VIRT.
  2014-01-07 17:55           ` Russell King - ARM Linux
@ 2014-01-08  6:40             ` Krzysztof Hałasa
  0 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Hałasa @ 2014-01-08  6:40 UTC (permalink / raw)
  To: linux-arm-kernel

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

> Okay, reverted.

Thanks.
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2014-01-08  6:40 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-31 11:20 Regression (ARM) arch/arm/mm/init.c doesn't build without CONFIG_ARM_PATCH_PHYS_VIRT Krzysztof Hałasa
2013-12-31 11:23 ` Russell King - ARM Linux
2014-01-01 14:40   ` Krzysztof Hałasa
2014-01-06 19:28 ` Krzysztof Hałasa
2014-01-06 19:33   ` Santosh Shilimkar
2014-01-06 22:08     ` Krzysztof Hałasa
2014-01-06 22:27       ` Santosh Shilimkar
2014-01-06 22:39   ` Russell King - ARM Linux
2014-01-06 23:42     ` Santosh Shilimkar
2014-01-07  1:11       ` Russell King - ARM Linux
2014-01-07 17:45         ` Santosh Shilimkar
2014-01-07 17:55           ` Russell King - ARM Linux
2014-01-08  6:40             ` Krzysztof Hałasa

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).