build warnings: multi_v7_defconfig + LPAE, printk with variable types

build warnings: multi_v7_defconfig + LPAE, printk with variable types

[Kevin Hilman]

I've started auto-building the multi_v7_defconfig with LPAE enabled, and
noticed a handful of warnings[1]

Several are due to using prink format on a type that changes with a
config option (e.g several below use %x to print a resource_size_t,
which is obviously different between 32- and 64-bit builds.

So this got me to wondering if there is "one true way" for using printk
on types that might change with config option (preferably without
ifdef.)

The one that comes to mind would be to just always cast to the larger of
the types and use the correspondig format.  For the ones below, it would
mean casting to (u64) and using '%llx'.

Is there a better way?

Kevin

[1] 
Warnings:
        arch/arm/mach-omap2/gpmc.c:1495:4: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'resource_size_t' [-Wformat]
        net/ipv6/addrconf.c:2793:22: warning: 'link_dev' may be used uninitialized in this function [-Wuninitialized]
        drivers/dma/ste_dma40.c:3228:2: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'resource_size_t' [-Wformat]
        drivers/dma/ste_dma40.c:3582:3: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'resource_size_t' [-Wformat]
        drivers/dma/ste_dma40.c:3582:3: warning: format '%x' expects argument of type 'unsigned int', but argument 5 has type 'resource_size_t' [-Wformat]
        drivers/dma/ste_dma40.c:3593:5: warning: format '%x' expects argument of type 'unsigned int', but argument 5 has type 'resource_size_t' [-Wformat]
        drivers/spi/spi-pl022.c:2178:9: warning: format '%x' expects argument of type 'unsigned int', but argument 2 has type 'resource_size_t' [-Wformat]
        drivers/tty/serial/imx.c:1542:6: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]

build warnings: multi_v7_defconfig + LPAE, printk with variable types

[Fabio Estevam]

On Wed, Aug 21, 2013 at 12:15 PM, Kevin Hilman <khilman@linaro.org> wrote:
> I've started auto-building the multi_v7_defconfig with LPAE enabled, and
> noticed a handful of warnings[1]
>
> Several are due to using prink format on a type that changes with a
> config option (e.g several below use %x to print a resource_size_t,
> which is obviously different between 32- and 64-bit builds.
>
> So this got me to wondering if there is "one true way" for using printk
> on types that might change with config option (preferably without
> ifdef.)
>
> The one that comes to mind would be to just always cast to the larger of
> the types and use the correspondig format.  For the ones below, it would
> mean casting to (u64) and using '%llx'.
>
> Is there a better way?

Yes, according to Documentation/printk-formats.txt:

"Physical addresses:

    %pa    0x01234567 or 0x0123456789abcdef

    For printing a phys_addr_t type (and its derivatives, such as
    resource_size_t) which can vary based on build options, regardless of
    the width of the CPU data path. Passed by reference."

I can send a patch fixing these warnings if you want.

Regards,

Fabio Estevam

build warnings: multi_v7_defconfig + LPAE, printk with variable types

[Kevin Hilman]

Fabio Estevam <festevam@gmail.com> writes:

> On Wed, Aug 21, 2013 at 12:15 PM, Kevin Hilman <khilman@linaro.org> wrote:
>> I've started auto-building the multi_v7_defconfig with LPAE enabled, and
>> noticed a handful of warnings[1]
>>
>> Several are due to using prink format on a type that changes with a
>> config option (e.g several below use %x to print a resource_size_t,
>> which is obviously different between 32- and 64-bit builds.
>>
>> So this got me to wondering if there is "one true way" for using printk
>> on types that might change with config option (preferably without
>> ifdef.)
>>
>> The one that comes to mind would be to just always cast to the larger of
>> the types and use the correspondig format.  For the ones below, it would
>> mean casting to (u64) and using '%llx'.
>>
>> Is there a better way?
>
> Yes, according to Documentation/printk-formats.txt:

> "Physical addresses:
>
>     %pa    0x01234567 or 0x0123456789abcdef
>
>     For printing a phys_addr_t type (and its derivatives, such as
>     resource_size_t) which can vary based on build options, regardless of
>     the width of the CPU data path. Passed by reference."

doh, RTFM.  That one is new since the last time I read it.

>
> I can send a patch fixing these warnings if you want.
>

Yes, please.  Thanks!

Please split it up into in to one patch for arch/arm/mach-omap2, one for
drivers/dma/* and one for drivers/spi/* so they can be merged through
the proper maintainers.

Thanks,

Kevin

build warnings: multi_v7_defconfig + LPAE, printk with variable types

[Fabio Estevam]

On Wed, Aug 21, 2013 at 12:49 PM, Kevin Hilman <khilman@linaro.org> wrote:

> Yes, please.  Thanks!
>
> Please split it up into in to one patch for arch/arm/mach-omap2, one for
> drivers/dma/* and one for drivers/spi/* so they can be merged through
> the proper maintainers.

Ok, will do it later today.

Regards,

Fabio Estevam

build warnings: multi_v7_defconfig + LPAE, printk with variable types

[Fabio Estevam]

On Wed, Aug 21, 2013 at 12:15 PM, Kevin Hilman <khilman@linaro.org> wrote:
> [1]
> Warnings:
>         arch/arm/mach-omap2/gpmc.c:1495:4: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'resource_size_t' [-Wformat]

For this warning, what about this?

--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -1491,8 +1491,8 @@ static int gpmc_probe_generic_child(struct
platform_device *pdev,
         */
        ret = gpmc_cs_remap(cs, res.start);
        if (ret < 0) {
-               dev_err(&pdev->dev, "cannot remap GPMC CS %d to 0x%x\n",
-                       cs, res.start);
+               dev_err(&pdev->dev, "cannot remap GPMC CS %d to 0x%pa\n",
+                       cs, &res.start);
                goto err;
        }

build warnings: multi_v7_defconfig + LPAE, printk with variable types

[Kevin Hilman]

Fabio Estevam <festevam@gmail.com> writes:

> On Wed, Aug 21, 2013 at 12:15 PM, Kevin Hilman <khilman@linaro.org> wrote:
>> [1]
>> Warnings:
>>         arch/arm/mach-omap2/gpmc.c:1495:4: warning: format '%x'
>> expects argument of type 'unsigned int', but argument 4 has type
>> resource_size_t' [-Wformat]
>
> For this warning, what about this?
>
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -1491,8 +1491,8 @@ static int gpmc_probe_generic_child(struct
> platform_device *pdev,
>          */
>         ret = gpmc_cs_remap(cs, res.start);
>         if (ret < 0) {
> -               dev_err(&pdev->dev, "cannot remap GPMC CS %d to 0x%x\n",
> -                       cs, res.start);
> +               dev_err(&pdev->dev, "cannot remap GPMC CS %d to 0x%pa\n",
> +                       cs, &res.start);

hmm, why did you add the '&' ?  That changes the behavior, not just the
format.

Kevin

build warnings: multi_v7_defconfig + LPAE, printk with variable types

[Russell King - ARM Linux]

On Wed, Aug 21, 2013 at 04:51:18PM -0700, Kevin Hilman wrote:
> Fabio Estevam <festevam@gmail.com> writes:
> 
> > On Wed, Aug 21, 2013 at 12:15 PM, Kevin Hilman <khilman@linaro.org> wrote:
> >> [1]
> >> Warnings:
> >>         arch/arm/mach-omap2/gpmc.c:1495:4: warning: format '%x'
> >> expects argument of type 'unsigned int', but argument 4 has type
> >> resource_size_t' [-Wformat]
> >
> > For this warning, what about this?
> >
> > --- a/arch/arm/mach-omap2/gpmc.c
> > +++ b/arch/arm/mach-omap2/gpmc.c
> > @@ -1491,8 +1491,8 @@ static int gpmc_probe_generic_child(struct
> > platform_device *pdev,
> >          */
> >         ret = gpmc_cs_remap(cs, res.start);
> >         if (ret < 0) {
> > -               dev_err(&pdev->dev, "cannot remap GPMC CS %d to 0x%x\n",
> > -                       cs, res.start);
> > +               dev_err(&pdev->dev, "cannot remap GPMC CS %d to 0x%pa\n",
> > +                       cs, &res.start);
> 
> hmm, why did you add the '&' ?  That changes the behavior, not just the
> format.

Remember that these strings are checked by the standard printf-format
checker.  So, in order to remain compatible, these extra extensions are
made to the %p format - hence %p with an a suffix.  Normally, the 'a'
would get printed with standard printf, but the kernel's version
will detect the following letter and use it to decide how to print the
pointer argument.

See Documentation/printk-formats.txt, and note that for %pa is says
"passed by reference".

If you don't like passing by reference here, the alternative solution is
this, from the same document:

If <type> is dependent on a config option for its size (e.g., sector_t,
blkcnt_t) or is architecture-dependent for its size (e.g., tcflag_t), use a
format specifier of its largest possible type and explicitly cast to it.
Example:

        printk("test: sector number/total blocks: %llu/%llu\n",
                (unsigned long long)sector, (unsigned long long)blockcount);

build warnings: multi_v7_defconfig + LPAE, printk with variable types

[Fabio Estevam]

On Wed, Aug 21, 2013 at 8:51 PM, Kevin Hilman <khilman@linaro.org> wrote:
> Fabio Estevam <festevam@gmail.com> writes:
>
>> On Wed, Aug 21, 2013 at 12:15 PM, Kevin Hilman <khilman@linaro.org> wrote:
>>> [1]
>>> Warnings:
>>>         arch/arm/mach-omap2/gpmc.c:1495:4: warning: format '%x'
>>> expects argument of type 'unsigned int', but argument 4 has type
>>> resource_size_t' [-Wformat]
>>
>> For this warning, what about this?
>>
>> --- a/arch/arm/mach-omap2/gpmc.c
>> +++ b/arch/arm/mach-omap2/gpmc.c
>> @@ -1491,8 +1491,8 @@ static int gpmc_probe_generic_child(struct
>> platform_device *pdev,
>>          */
>>         ret = gpmc_cs_remap(cs, res.start);
>>         if (ret < 0) {
>> -               dev_err(&pdev->dev, "cannot remap GPMC CS %d to 0x%x\n",
>> -                       cs, res.start);
>> +               dev_err(&pdev->dev, "cannot remap GPMC CS %d to 0x%pa\n",
>> +                       cs, &res.start);
>
> hmm, why did you add the '&' ?  That changes the behavior, not just the
> format.

Yes, this is required. Please refer to the 'Passed by reference.' in
the end of the paragrah of Documention/printk-formats.txt.

The other good thing of using '%pa' is that I can remove the '0x' as
well. Just noticed this after doing some local tests. Will submit the
patches shortly.