[PATCH] CMA: Fix the phys_addr_t print types

[PATCH] CMA: Fix the phys_addr_t print types

[Santosh Shilimkar]

Otherwise prints would truncate the variables on LPAE machines.

Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 drivers/base/dma-contiguous.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c
index 165c2c2..b303a98 100644
--- a/drivers/base/dma-contiguous.c
+++ b/drivers/base/dma-contiguous.c
@@ -201,9 +201,8 @@ int __init dma_contiguous_reserve_area(phys_addr_t size, phys_addr_t base,
 	phys_addr_t alignment;
 	int ret = 0;
 
-	pr_debug("%s(size %lx, base %08lx, limit %08lx)\n", __func__,
-		 (unsigned long)size, (unsigned long)base,
-		 (unsigned long)limit);
+	pr_info("%s(size %pa, base %pa, limit %pa)\n", __func__,
+		 &size, &base, &limit);
 
 	/* Sanity checks */
 	if (cma_area_count == ARRAY_SIZE(cma_areas)) {
@@ -250,8 +249,8 @@ int __init dma_contiguous_reserve_area(phys_addr_t size, phys_addr_t base,
 	*res_cma = cma;
 	cma_area_count++;
 
-	pr_info("CMA: reserved %ld MiB at %08lx\n", (unsigned long)size / SZ_1M,
-		(unsigned long)base);
+	pr_info("CMA: reserved %ld MiB at %pa\n", (unsigned long)size / SZ_1M,
+		&base);
 
 	/* Architecture specific contiguous memory fixup. */
 	dma_contiguous_early_fixup(base, size);
-- 
1.7.9.5

Re: [PATCH] CMA: Fix the phys_addr_t print types

[Greg Kroah-Hartman]

On Sat, Nov 23, 2013 at 10:37:01PM -0500, Santosh Shilimkar wrote:
> Otherwise prints would truncate the variables on LPAE machines.
> 
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> ---
>  drivers/base/dma-contiguous.c |    9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c
> index 165c2c2..b303a98 100644
> --- a/drivers/base/dma-contiguous.c
> +++ b/drivers/base/dma-contiguous.c
> @@ -201,9 +201,8 @@ int __init dma_contiguous_reserve_area(phys_addr_t size, phys_addr_t base,
>  	phys_addr_t alignment;
>  	int ret = 0;
>  
> -	pr_debug("%s(size %lx, base %08lx, limit %08lx)\n", __func__,
> -		 (unsigned long)size, (unsigned long)base,
> -		 (unsigned long)limit);
> +	pr_info("%s(size %pa, base %pa, limit %pa)\n", __func__,
> +		 &size, &base, &limit);

Why did you change the logging level of this message?

Re: [PATCH] CMA: Fix the phys_addr_t print types

[Santosh Shilimkar]

On Saturday 23 November 2013 10:43 PM, Greg Kroah-Hartman wrote:
> On Sat, Nov 23, 2013 at 10:37:01PM -0500, Santosh Shilimkar wrote:
>> Otherwise prints would truncate the variables on LPAE machines.
>>
>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> ---
>>  drivers/base/dma-contiguous.c |    9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c
>> index 165c2c2..b303a98 100644
>> --- a/drivers/base/dma-contiguous.c
>> +++ b/drivers/base/dma-contiguous.c
>> @@ -201,9 +201,8 @@ int __init dma_contiguous_reserve_area(phys_addr_t size, phys_addr_t base,
>>  	phys_addr_t alignment;
>>  	int ret = 0;
>>  
>> -	pr_debug("%s(size %lx, base %08lx, limit %08lx)\n", __func__,
>> -		 (unsigned long)size, (unsigned long)base,
>> -		 (unsigned long)limit);
>> +	pr_info("%s(size %pa, base %pa, limit %pa)\n", __func__,
>> +		 &size, &base, &limit);
> 
> Why did you change the logging level of this message?
> 
Opps... That was accidental. Will post updated version.

regards,
Santosh

Re: [PATCH] CMA: Fix the phys_addr_t print types

[Greg Kroah-Hartman]

On Sat, Nov 23, 2013 at 10:37:01PM -0500, Santosh Shilimkar wrote:
> @@ -250,8 +249,8 @@ int __init dma_contiguous_reserve_area(phys_addr_t size, phys_addr_t base,
>  	*res_cma = cma;
>  	cma_area_count++;
>  
> -	pr_info("CMA: reserved %ld MiB at %08lx\n", (unsigned long)size / SZ_1M,
> -		(unsigned long)base);
> +	pr_info("CMA: reserved %ld MiB at %pa\n", (unsigned long)size / SZ_1M,
> +		&base);

Why is this pr_info() at all?  That's just noise, please move it to
pr_debug().

thanks,

greg k-h

Re: [PATCH] CMA: Fix the phys_addr_t print types

[Santosh Shilimkar]

On Saturday 23 November 2013 10:44 PM, Greg Kroah-Hartman wrote:
> On Sat, Nov 23, 2013 at 10:37:01PM -0500, Santosh Shilimkar wrote:
>> @@ -250,8 +249,8 @@ int __init dma_contiguous_reserve_area(phys_addr_t size, phys_addr_t base,
>>  	*res_cma = cma;
>>  	cma_area_count++;
>>  
>> -	pr_info("CMA: reserved %ld MiB at %08lx\n", (unsigned long)size / SZ_1M,
>> -		(unsigned long)base);
>> +	pr_info("CMA: reserved %ld MiB at %pa\n", (unsigned long)size / SZ_1M,
>> +		&base);
> 
> Why is this pr_info() at all?  That's just noise, please move it to
> pr_debug().
> 
Marek can comment better but I think its useful print to know CMA
reserved memory size.

regards,
Santosh

Re: [PATCH] CMA: Fix the phys_addr_t print types

[Greg Kroah-Hartman]

On Sat, Nov 23, 2013 at 10:45:37PM -0500, Santosh Shilimkar wrote:
> On Saturday 23 November 2013 10:44 PM, Greg Kroah-Hartman wrote:
> > On Sat, Nov 23, 2013 at 10:37:01PM -0500, Santosh Shilimkar wrote:
> >> @@ -250,8 +249,8 @@ int __init dma_contiguous_reserve_area(phys_addr_t size, phys_addr_t base,
> >>  	*res_cma = cma;
> >>  	cma_area_count++;
> >>  
> >> -	pr_info("CMA: reserved %ld MiB at %08lx\n", (unsigned long)size / SZ_1M,
> >> -		(unsigned long)base);
> >> +	pr_info("CMA: reserved %ld MiB at %pa\n", (unsigned long)size / SZ_1M,
> >> +		&base);
> > 
> > Why is this pr_info() at all?  That's just noise, please move it to
> > pr_debug().
> > 
> Marek can comment better but I think its useful print to know CMA
> reserved memory size.

Useful to who?

Re: [PATCH] CMA: Fix the phys_addr_t print types

[Santosh Shilimkar]

On Saturday 23 November 2013 11:10 PM, Greg Kroah-Hartman wrote:
> On Sat, Nov 23, 2013 at 10:45:37PM -0500, Santosh Shilimkar wrote:
>> On Saturday 23 November 2013 10:44 PM, Greg Kroah-Hartman wrote:
>>> On Sat, Nov 23, 2013 at 10:37:01PM -0500, Santosh Shilimkar wrote:
>>>> @@ -250,8 +249,8 @@ int __init dma_contiguous_reserve_area(phys_addr_t size, phys_addr_t base,
>>>>  	*res_cma = cma;
>>>>  	cma_area_count++;
>>>>  
>>>> -	pr_info("CMA: reserved %ld MiB at %08lx\n", (unsigned long)size / SZ_1M,
>>>> -		(unsigned long)base);
>>>> +	pr_info("CMA: reserved %ld MiB at %pa\n", (unsigned long)size / SZ_1M,
>>>> +		&base);
>>>
>>> Why is this pr_info() at all?  That's just noise, please move it to
>>> pr_debug().
>>>
>> Marek can comment better but I think its useful print to know CMA
>> reserved memory size.
> 
> Useful to who?
> 
Useful to anyone wants to know the CMA usage on a platform. 
CMA size is configurable and platforms tend to use different sizes
based on needs. The info don't appear in /proc/meminfo, so probably
dmsg grep is easy enough to know how much CMA memory being used on
a platform. That was my point.

I don't have strong argument here against not making it pr_debug
but was waiting for Marek's opinion on it.

Regards,
Santosh

Re: [PATCH] CMA: Fix the phys_addr_t print types

[Marek Szyprowski]

Hi,

On 2013-11-24 05:28, Santosh Shilimkar wrote:
> On Saturday 23 November 2013 11:10 PM, Greg Kroah-Hartman wrote:
> > On Sat, Nov 23, 2013 at 10:45:37PM -0500, Santosh Shilimkar wrote:
> >> On Saturday 23 November 2013 10:44 PM, Greg Kroah-Hartman wrote:
> >>> On Sat, Nov 23, 2013 at 10:37:01PM -0500, Santosh Shilimkar wrote:
> >>>> @@ -250,8 +249,8 @@ int __init dma_contiguous_reserve_area(phys_addr_t size, phys_addr_t base,
> >>>>  	*res_cma = cma;
> >>>>  	cma_area_count++;
> >>>>
> >>>> -	pr_info("CMA: reserved %ld MiB at %08lx\n", (unsigned long)size / SZ_1M,
> >>>> -		(unsigned long)base);
> >>>> +	pr_info("CMA: reserved %ld MiB at %pa\n", (unsigned long)size / SZ_1M,
> >>>> +		&base);
> >>>
> >>> Why is this pr_info() at all?  That's just noise, please move it to
> >>> pr_debug().
> >>>
> >> Marek can comment better but I think its useful print to know CMA
> >> reserved memory size.
> >
> > Useful to who?
> >
> Useful to anyone wants to know the CMA usage on a platform.
> CMA size is configurable and platforms tend to use different sizes
> based on needs. The info don't appear in /proc/meminfo, so probably
> dmsg grep is easy enough to know how much CMA memory being used on
> a platform. That was my point.
>
> I don't have strong argument here against not making it pr_debug
> but was waiting for Marek's opinion on it.

If possible I would like to keep pr_info. It already helped me a lot
while analyzing someone's logs to find why memory allocation failed.
A simple search for "CMA" messages enabled me to quickly check if CMA
has been enabled and configured properly or not. It also gives a curious
user some information about the memory configuration (especially if he
don't need CMA, he will investigate why kernel has reserved so much
memory).

Best regards
-- 
Marek Szyprowski
Samsung R&D Institute Poland