[PATCH v2] drivers: cma: fix addressing on PAE machines

[PATCH v2] drivers: cma: fix addressing on PAE machines

[Vitaly Andrianov]

This patch fixes a couple of bugs that otherwise impair CMA functionality on
PAE machines:

  - alignment must be a 64-bit type when running on systems with 64-bit
    physical addresses.  If this is not the case, the limit calculation thunks
    allocations down to an address range < 4G.

  - The allocated range check is removed. On 32bit ARM kernel with LPAE
    enabled the base may be allocated outside the fist 4GB of physical
    memory (keystone SoC for example).

Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
Signed-off-by: Cyril Chemparathy <cyril@ti.com>
---
 drivers/base/dma-contiguous.c  |   20 ++++++++------------
 include/linux/dma-contiguous.h |    4 ++--
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c
index 9a14694..097dd44 100644
--- a/drivers/base/dma-contiguous.c
+++ b/drivers/base/dma-contiguous.c
@@ -60,8 +60,8 @@ struct cma *dma_contiguous_default_area;
  * Users, who want to set the size of global CMA area for their system
  * should use cma= kernel parameter.
  */
-static const unsigned long size_bytes = CMA_SIZE_MBYTES * SZ_1M;
-static long size_cmdline = -1;
+static const phys_addr_t size_bytes = CMA_SIZE_MBYTES * SZ_1M;
+static phys_addr_t size_cmdline = -1;
 
 static int __init early_cma(char *p)
 {
@@ -73,7 +73,7 @@ early_param("cma", early_cma);
 
 #ifdef CONFIG_CMA_SIZE_PERCENTAGE
 
-static unsigned long __init __maybe_unused cma_early_percent_memory(void)
+static phys_addr_t __init __maybe_unused cma_early_percent_memory(void)
 {
 	struct memblock_region *reg;
 	unsigned long total_pages = 0;
@@ -91,7 +91,7 @@ static unsigned long __init __maybe_unused cma_early_percent_memory(void)
 
 #else
 
-static inline __maybe_unused unsigned long cma_early_percent_memory(void)
+static inline __maybe_unused phys_addr_t cma_early_percent_memory(void)
 {
 	return 0;
 }
@@ -109,7 +109,7 @@ static inline __maybe_unused unsigned long cma_early_percent_memory(void)
  */
 void __init dma_contiguous_reserve(phys_addr_t limit)
 {
-	unsigned long selected_size = 0;
+	phys_addr_t selected_size = 0;
 
 	pr_debug("%s(limit %08lx)\n", __func__, (unsigned long)limit);
 
@@ -129,7 +129,7 @@ void __init dma_contiguous_reserve(phys_addr_t limit)
 
 	if (selected_size) {
 		pr_debug("%s: reserving %ld MiB for global area\n", __func__,
-			 selected_size / SZ_1M);
+			 (unsigned long)selected_size / SZ_1M);
 
 		dma_declare_contiguous(NULL, selected_size, 0, limit);
 	}
@@ -230,11 +230,11 @@ core_initcall(cma_init_reserved_areas);
  * called by board specific code when early allocator (memblock or bootmem)
  * is still activate.
  */
-int __init dma_declare_contiguous(struct device *dev, unsigned long size,
+int __init dma_declare_contiguous(struct device *dev, phys_addr_t size,
 				  phys_addr_t base, phys_addr_t limit)
 {
 	struct cma_reserved *r = &cma_reserved[cma_reserved_count];
-	unsigned long alignment;
+	phys_addr_t alignment;
 
 	pr_debug("%s(size %lx, base %08lx, limit %08lx)\n", __func__,
 		 (unsigned long)size, (unsigned long)base,
@@ -271,10 +271,6 @@ int __init dma_declare_contiguous(struct device *dev, unsigned long size,
 		if (!addr) {
 			base = -ENOMEM;
 			goto err;
-		} else if (addr + size > ~(unsigned long)0) {
-			memblock_free(addr, size);
-			base = -EINVAL;
-			goto err;
 		} else {
 			base = addr;
 		}
diff --git a/include/linux/dma-contiguous.h b/include/linux/dma-contiguous.h
index 2f303e4..01b5c84 100644
--- a/include/linux/dma-contiguous.h
+++ b/include/linux/dma-contiguous.h
@@ -68,7 +68,7 @@ struct device;
 extern struct cma *dma_contiguous_default_area;
 
 void dma_contiguous_reserve(phys_addr_t addr_limit);
-int dma_declare_contiguous(struct device *dev, unsigned long size,
+int dma_declare_contiguous(struct device *dev, phys_addr_t size,
 			   phys_addr_t base, phys_addr_t limit);
 
 struct page *dma_alloc_from_contiguous(struct device *dev, int count,
@@ -83,7 +83,7 @@ bool dma_release_from_contiguous(struct device *dev, struct page *pages,
 static inline void dma_contiguous_reserve(phys_addr_t limit) { }
 
 static inline
-int dma_declare_contiguous(struct device *dev, unsigned long size,
+int dma_declare_contiguous(struct device *dev, phys_addr_t size,
 			   phys_addr_t base, phys_addr_t limit)
 {
 	return -ENOSYS;
-- 
1.7.9.5

Re: [PATCH v2] drivers: cma: fix addressing on PAE machines

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 4872 bytes --]

On Mon, Dec 03 2012, Vitaly Andrianov <vitalya@ti.com> wrote:
> This patch fixes a couple of bugs that otherwise impair CMA functionality on
> PAE machines:
>
>   - alignment must be a 64-bit type when running on systems with 64-bit
>     physical addresses.  If this is not the case, the limit calculation thunks
>     allocations down to an address range < 4G.
>
>   - The allocated range check is removed. On 32bit ARM kernel with LPAE
>     enabled the base may be allocated outside the fist 4GB of physical
>     memory (keystone SoC for example).
>
> Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
> Signed-off-by: Cyril Chemparathy <cyril@ti.com>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

> diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c
> index 9a14694..097dd44 100644
> --- a/drivers/base/dma-contiguous.c
> +++ b/drivers/base/dma-contiguous.c
> @@ -60,8 +60,8 @@ struct cma *dma_contiguous_default_area;
>   * Users, who want to set the size of global CMA area for their system
>   * should use cma= kernel parameter.
>   */
> -static const unsigned long size_bytes = CMA_SIZE_MBYTES * SZ_1M;
> -static long size_cmdline = -1;
> +static const phys_addr_t size_bytes = CMA_SIZE_MBYTES * SZ_1M;
> +static phys_addr_t size_cmdline = -1;
>  
>  static int __init early_cma(char *p)
>  {
> @@ -73,7 +73,7 @@ early_param("cma", early_cma);
>  
>  #ifdef CONFIG_CMA_SIZE_PERCENTAGE
>  
> -static unsigned long __init __maybe_unused cma_early_percent_memory(void)
> +static phys_addr_t __init __maybe_unused cma_early_percent_memory(void)
>  {
>  	struct memblock_region *reg;
>  	unsigned long total_pages = 0;
> @@ -91,7 +91,7 @@ static unsigned long __init __maybe_unused cma_early_percent_memory(void)
>  
>  #else
>  
> -static inline __maybe_unused unsigned long cma_early_percent_memory(void)
> +static inline __maybe_unused phys_addr_t cma_early_percent_memory(void)
>  {
>  	return 0;
>  }
> @@ -109,7 +109,7 @@ static inline __maybe_unused unsigned long cma_early_percent_memory(void)
>   */
>  void __init dma_contiguous_reserve(phys_addr_t limit)
>  {
> -	unsigned long selected_size = 0;
> +	phys_addr_t selected_size = 0;
>  
>  	pr_debug("%s(limit %08lx)\n", __func__, (unsigned long)limit);
>  
> @@ -129,7 +129,7 @@ void __init dma_contiguous_reserve(phys_addr_t limit)
>  
>  	if (selected_size) {
>  		pr_debug("%s: reserving %ld MiB for global area\n", __func__,
> -			 selected_size / SZ_1M);
> +			 (unsigned long)selected_size / SZ_1M);
>  
>  		dma_declare_contiguous(NULL, selected_size, 0, limit);
>  	}
> @@ -230,11 +230,11 @@ core_initcall(cma_init_reserved_areas);
>   * called by board specific code when early allocator (memblock or bootmem)
>   * is still activate.
>   */
> -int __init dma_declare_contiguous(struct device *dev, unsigned long size,
> +int __init dma_declare_contiguous(struct device *dev, phys_addr_t size,
>  				  phys_addr_t base, phys_addr_t limit)
>  {
>  	struct cma_reserved *r = &cma_reserved[cma_reserved_count];
> -	unsigned long alignment;
> +	phys_addr_t alignment;
>  
>  	pr_debug("%s(size %lx, base %08lx, limit %08lx)\n", __func__,
>  		 (unsigned long)size, (unsigned long)base,
> @@ -271,10 +271,6 @@ int __init dma_declare_contiguous(struct device *dev, unsigned long size,
>  		if (!addr) {
>  			base = -ENOMEM;
>  			goto err;
> -		} else if (addr + size > ~(unsigned long)0) {
> -			memblock_free(addr, size);
> -			base = -EINVAL;
> -			goto err;
>  		} else {
>  			base = addr;
>  		}
> diff --git a/include/linux/dma-contiguous.h b/include/linux/dma-contiguous.h
> index 2f303e4..01b5c84 100644
> --- a/include/linux/dma-contiguous.h
> +++ b/include/linux/dma-contiguous.h
> @@ -68,7 +68,7 @@ struct device;
>  extern struct cma *dma_contiguous_default_area;
>  
>  void dma_contiguous_reserve(phys_addr_t addr_limit);
> -int dma_declare_contiguous(struct device *dev, unsigned long size,
> +int dma_declare_contiguous(struct device *dev, phys_addr_t size,
>  			   phys_addr_t base, phys_addr_t limit);
>  
>  struct page *dma_alloc_from_contiguous(struct device *dev, int count,
> @@ -83,7 +83,7 @@ bool dma_release_from_contiguous(struct device *dev, struct page *pages,
>  static inline void dma_contiguous_reserve(phys_addr_t limit) { }
>  
>  static inline
> -int dma_declare_contiguous(struct device *dev, unsigned long size,
> +int dma_declare_contiguous(struct device *dev, phys_addr_t size,
>  			   phys_addr_t base, phys_addr_t limit)
>  {
>  	return -ENOSYS;

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [linux-keystone] [PATCH v2] drivers: cma: fix addressing on PAE machines

[Santosh Shilimkar]

On Monday 03 December 2012 09:16 PM, Vitaly Andrianov wrote:
> This patch fixes a couple of bugs that otherwise impair CMA functionality on
> PAE machines:
>
>    - alignment must be a 64-bit type when running on systems with 64-bit
>      physical addresses.  If this is not the case, the limit calculation thunks
>      allocations down to an address range < 4G.
>
>    - The allocated range check is removed. On 32bit ARM kernel with LPAE
>      enabled the base may be allocated outside the fist 4GB of physical
>      memory (keystone SoC for example).
>
Any reason you have clubbed two fixes in one patch. Its better to keep
the two fixes separate patches.

> Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
> Signed-off-by: Cyril Chemparathy <cyril@ti.com>
> ---
Other than that, patch looks good to my eyes.

Regards,
Santosh

Re: [linux-keystone] [PATCH v2] drivers: cma: fix addressing on PAE machines

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 1314 bytes --]

> On Monday 03 December 2012 09:16 PM, Vitaly Andrianov wrote:
>> This patch fixes a couple of bugs that otherwise impair CMA functionality on
>> PAE machines:
>>
>>    - alignment must be a 64-bit type when running on systems with 64-bit
>>      physical addresses.  If this is not the case, the limit calculation thunks
>>      allocations down to an address range < 4G.
>>
>>    - The allocated range check is removed. On 32bit ARM kernel with LPAE
>>      enabled the base may be allocated outside the fist 4GB of physical
>>      memory (keystone SoC for example).

On Tue, Dec 04 2012, Santosh Shilimkar wrote:
> Any reason you have clubbed two fixes in one patch. Its better to keep
> the two fixes separate patches.

They are all related to the very same issue, and what the whole patch
does is change the type used to store physical addresses from unsigned
long to phys_addr_t.  This is really a single change.

>> Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
>> Signed-off-by: Cyril Chemparathy <cyril@ti.com>

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [linux-keystone] [PATCH v2] drivers: cma: fix addressing on PAE machines

[Santosh Shilimkar]

On Tuesday 04 December 2012 06:37 PM, Michal Nazarewicz wrote:
>> On Monday 03 December 2012 09:16 PM, Vitaly Andrianov wrote:
>>> This patch fixes a couple of bugs that otherwise impair CMA functionality on
>>> PAE machines:
>>>
>>>     - alignment must be a 64-bit type when running on systems with 64-bit
>>>       physical addresses.  If this is not the case, the limit calculation thunks
>>>       allocations down to an address range < 4G.
>>>
>>>     - The allocated range check is removed. On 32bit ARM kernel with LPAE
>>>       enabled the base may be allocated outside the fist 4GB of physical
>>>       memory (keystone SoC for example).
>
> On Tue, Dec 04 2012, Santosh Shilimkar wrote:
>> Any reason you have clubbed two fixes in one patch. Its better to keep
>> the two fixes separate patches.
>
> They are all related to the very same issue, and what the whole patch
> does is change the type used to store physical addresses from unsigned
> long to phys_addr_t.  This is really a single change.
>
Thanks for clarification. 64 bit alignment fix and the allocation range 
checks
can be two separate fixes and that is exactly what change log describes.
You have a last say though :-) No problem if you want to commit the
patch as is.

Regards
Santosh

Re: [linux-keystone] [PATCH v2] drivers: cma: fix addressing on PAE machines

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 1814 bytes --]

> On Tuesday 04 December 2012 06:37 PM, Michal Nazarewicz wrote:
>> They are all related to the very same issue, and what the whole patch
>> does is change the type used to store physical addresses from unsigned
>> long to phys_addr_t.  This is really a single change.

On Tue, Dec 04 2012, Santosh Shilimkar wrote:
> Thanks for clarification. 64 bit alignment fix and the allocation
> range checks can be two separate fixes and that is exactly what change
> log describes.  You have a last say though :-) No problem if you want
> to commit the patch as is.

I don't have strong feelings on this one, but I feel like it's really
a single change which manifests itself in a few ways.  If this is
confusing, maybe commit message could be improved, to something like:

------------- >8 -------------------------------------------------------
drivers: cma: represent physicall addresses as phys_addr_t

This commit changes the CMA early initialisation code to use phys_addr_t
for representing physical addresses instead of unsigned long.

Without this change, among other things, dma_declare_contiguous() simply
discards any memory regions whose address is not represtible as unsigned
long.

This is a problem on 32-bit PAE machines where unsigned long is 32-bit
but physical address space is larger.
------------- 8< -------------------------------------------------------

Vitaly, if you could resend with that description, it would be awesome,
and sorry for so much trouble in what appears to be a trivial patch. :P

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]

RE: [linux-keystone] [PATCH v2] drivers: cma: fix addressing on PAE machines

[Andrianov, Vitaly]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2589 bytes --]



> -----Original Message-----
> From: Michal Nazarewicz [mailto:mpn@google.com] On Behalf Of Michal
> Nazarewicz
> Sent: Tuesday, December 04, 2012 2:05 PM
> To: Andrianov, Vitaly; Shilimkar, Santosh
> Cc: m.szyprowski@samsung.com; kyungmin.park@samsung.com; arnd@arndb.de;
> linux-kernel@vger.kernel.org; linux-keystone@list.ti.com - Linux
> developers for Keystone family of devices (May contain non-TIers);
> Chemparathy, Cyril
> Subject: Re: [linux-keystone] [PATCH v2] drivers: cma: fix addressing
> on PAE machines
> 
> > On Tuesday 04 December 2012 06:37 PM, Michal Nazarewicz wrote:
> >> They are all related to the very same issue, and what the whole
> patch
> >> does is change the type used to store physical addresses from
> >> unsigned long to phys_addr_t.  This is really a single change.
> 
> On Tue, Dec 04 2012, Santosh Shilimkar wrote:
> > Thanks for clarification. 64 bit alignment fix and the allocation
> > range checks can be two separate fixes and that is exactly what
> change
> > log describes.  You have a last say though :-) No problem if you want
> > to commit the patch as is.
> 
> I don't have strong feelings on this one, but I feel like it's really a
> single change which manifests itself in a few ways.  If this is
> confusing, maybe commit message could be improved, to something like:
> 
> ------------- >8 ------------------------------------------------------
> -
> drivers: cma: represent physicall addresses as phys_addr_t
> 
> This commit changes the CMA early initialisation code to use
> phys_addr_t for representing physical addresses instead of unsigned
> long.
> 
> Without this change, among other things, dma_declare_contiguous()
> simply discards any memory regions whose address is not represtible as
> unsigned long.
> 
> This is a problem on 32-bit PAE machines where unsigned long is 32-bit
> but physical address space is larger.
> ------------- 8< ------------------------------------------------------
> -
> 
> Vitaly, if you could resend with that description, it would be awesome,
> and sorry for so much trouble in what appears to be a trivial patch. :P
> 

Sure, I'll do that.

> --
> Best regards,                                         _     _
> .o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
> ..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
> ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

Regards,
Vitaly

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥