Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 02 Feb 2021 12:06:40 -0800 (PST)
Subject:     Re: [PATCH] arch_numa: fix common code printing of phys_addr_t
In-Reply-To: <0d6fb1da-6b14-7f54-3f4c-4697c88a14f7@infradead.org>
CC:     linux-kernel@vger.kernel.org, lkp@intel.com,
        Atish Patra <Atish.Patra@wdc.com>
From:   Palmer Dabbelt <palmer@dabbelt.com>
To:     rdunlap@infradead.org
Message-ID: <mhng-59dd686b-42bf-4880-8749-8b0426d55629@palmerdabbelt-glaptop>
Mime-Version: 1.0 (MHng)
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Mon, 01 Feb 2021 19:51:07 PST (-0800), rdunlap@infradead.org wrote:
> On 2/1/21 7:36 PM, Palmer Dabbelt wrote:
>> On Wed, 27 Jan 2021 19:55:33 PST (-0800), rdunlap@infradead.org wrote:
>>> Fix build warnings in the arch_numa common code:
>>>
>>> ../include/linux/kern_levels.h:5:18: warning: format '%Lx' expects argument of type 'long long unsigned int', but argument 3 has type 'phys_addr_t' {aka 'unsigned int'} [-Wformat=]
>>> ../drivers/base/arch_numa.c:360:56: note: format string is defined here
>>>   360 |    pr_warn("Warning: invalid memblk node %d [mem %#010Lx-%#010Lx]\n",
>>> ../drivers/base/arch_numa.c:435:39: note: format string is defined here
>>>   435 |  pr_info("Faking a node at [mem %#018Lx-%#018Lx]\n", start, end - 1);
>>>
>>> Fixes: ae3c107cd8be ("numa: Move numa implementation to common code")
>>> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Cc: Atish Patra <atish.patra@wdc.com>
>>> Cc: Palmer Dabbelt <palmerdabbelt@google.com>
>>> ---
>>>  drivers/base/arch_numa.c |   13 +++++++------
>>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>>
>>> --- linux-next-20210125.orig/drivers/base/arch_numa.c
>>> +++ linux-next-20210125/drivers/base/arch_numa.c
>>> @@ -355,11 +355,12 @@ static int __init numa_register_nodes(vo
>>>      /* Check that valid nid is set to memblks */
>>>      for_each_mem_region(mblk) {
>>>          int mblk_nid = memblock_get_region_node(mblk);
>>> +        phys_addr_t start = mblk->base;
>>> +        phys_addr_t end = mblk->base + mblk->size - 1;
>>>
>>>          if (mblk_nid == NUMA_NO_NODE || mblk_nid >= MAX_NUMNODES) {
>>> -            pr_warn("Warning: invalid memblk node %d [mem %#010Lx-%#010Lx]\n",
>>> -                mblk_nid, mblk->base,
>>> -                mblk->base + mblk->size - 1);
>>> +            pr_warn("Warning: invalid memblk node %d [mem %pap-%pap]\n",
>>> +                mblk_nid, &start, &end);
>>>              return -EINVAL;
>>>          }
>>>      }
>>> @@ -427,14 +428,14 @@ out_free_distance:
>>>  static int __init dummy_numa_init(void)
>>>  {
>>>      phys_addr_t start = memblock_start_of_DRAM();
>>> -    phys_addr_t end = memblock_end_of_DRAM();
>>> +    phys_addr_t end = memblock_end_of_DRAM() - 1;
>>>      int ret;
>>>
>>>      if (numa_off)
>>>          pr_info("NUMA disabled\n"); /* Forced off on command line. */
>>> -    pr_info("Faking a node at [mem %#018Lx-%#018Lx]\n", start, end - 1);
>>> +    pr_info("Faking a node at [mem %pap-%pap]\n", &start, &end);
>>>
>>> -    ret = numa_add_memblk(0, start, end);
>>> +    ret = numa_add_memblk(0, start, end + 1);
>>>      if (ret) {
>>>          pr_err("NUMA init failed\n");
>>>          return ret;
>>
>> Thanks, this is on for-next.  Did you, by any chance, find %Lx documented
>> anywhere?  It's not ISO C and the GCC source code says it's a GNU extension,
>> but I couldn't find it in the documentation (or even where to add it, which I
>> guess is how I forgot to send my version fo the patch).
>
> 'man sprintf' says this:
>
>        As  a nonstandard extension, the GNU implementations treats ll and L as
>        synonyms, so that one can, for example, write llg (as a synonym for the
>        standards-compliant  Lg) and Ld (as a synonym for the standards compli-
>        ant lld).  Such usage is nonportable.
>
>
> and linux/lib/vsprintf.c has some handling for it:
>
> 	if (qualifier == 'L')
> 		spec->type = FORMAT_TYPE_LONG_LONG;
>
> and
>
> 		case 'L':
> 			if (is_sign)
> 				*va_arg(args, long long *) = val.s;
> 			else
> 				*va_arg(args, unsigned long long *) = val.u;
> 			break;
>
>
> Does that help?

The manpage does it, I guess I just wasn't reading closely enough. Thanks!