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 [off-list ref]
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
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
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:
quoted
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
From: Russell King - ARM Linux <hidden> Date: 2014-01-06 23:56:26
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".
From: Russell King - ARM Linux <hidden> Date: 2014-01-07 01:11:29
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:
quoted
On Mon, Jan 06, 2014 at 08:28:20PM +0100, Krzysztof Ha?asa wrote:
quoted
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".
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:
quoted
On Monday 06 January 2014 05:39 PM, Russell King - ARM Linux wrote:
quoted
On Mon, Jan 06, 2014 at 08:28:20PM +0100, Krzysztof Ha?asa wrote:
quoted
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.
\--------------------------------------------------------------
quoted
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
From: Russell King - ARM Linux <hidden> Date: 2014-01-07 17:55:19
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:
quoted
/--------------------------------------------------------------
| > > 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.
\--------------------------------------------------------------
quoted
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".