Re: [Qemu-devel] [PATCH v5 1/4] firmware: introduce sysfs driver for QEMU's fw_cfg device

[Gabriel L. Somlo]

On Tue, Nov 24, 2015 at 04:14:50AM +0800, kbuild test robot wrote:
> Hi Gabriel,
> 
> [auto build test WARNING on v4.4-rc2]
> [also build test WARNING on next-20151123]
> [cannot apply to robh/for-next]
> 
> url:    
> https://github.com/0day-ci/linux/commits/Gabriel-L-Somlo/SysFS-driver-for-QEMU-fw_cfg-device/20151124-000402
> config: arm-allyesconfig (attached as .config)
> reproduce:
>         wget 
> https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
>  -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=arm 
> 
> All warnings (new ones prefixed by >>):
> 
>    drivers/firmware/qemu_fw_cfg.c: In function 'fw_cfg_cmdline_set':
> >> drivers/firmware/qemu_fw_cfg.c:510:7: warning: format '%lli' expects 
> >> argument of type 'long long int *', but argument 3 has type 'phys_addr_t 
> >> *' [-Wformat=]
>           &ctrl_off, &data_off, &consumed);
>           ^

Oh, I think I know why this happened:

...
        phys_addr_t base;
        resource_size_t size, ctrl_off, data_off;
...
        /* get "@<base>[:<ctrl_off>:<data_off>]" chunks */
        processed = sscanf(str, "@%lli%n:%lli:%lli%n"
                           &base, &consumed,
                           &ctrl_off, &data_off, &consumed);
...

On some architectures, phys_addr_t (and resource_size_t) are u32,
rather than u64. In include/linux/types.h we have:

...
#ifdef CONFIG_PHYS_ADDR_T_64BIT
typedef u64 phys_addr_t;
#else
typedef u32 phys_addr_t;
#endif
...

So, I could use u64 instead of phys_addr_t and resource_size_t, and
keep "%lli" (or "%Li"), but then I'd have to check if the parsed value
would overflow a 32-bit address value on arches where phys_addr_t is
u32, which would make things a bit more messy and awkward.

I'm planning on #ifdef-ing the format string instead:

#ifdef CONFIG_PHYS_ADDR_T_64BIT
#define PH_ADDR_SCAN_FMT "@%Li%n:%Li:%Li%n"
#else
#define PH_ADDR_SCAN_FMT "@%li%n:%li:%li%n"
#endif
...
        processed = sscanf(str, PH_ADDR_SCAN_FMT,
                           &base, &consumed,
                           &ctrl_off, &data_off, &consumed);


This should make the warning go away, and be correct. Does that sound
like a valid plan, or is there some other stylistic reason I should
stay away from doing it that way ?

thanks,
--Gabriel

> >> drivers/firmware/qemu_fw_cfg.c:510:7: warning: format '%lli' expects 
> >> argument of type 'long long int *', but argument 5 has type 
> >> 'resource_size_t *' [-Wformat=]
>    drivers/firmware/qemu_fw_cfg.c:510:7: warning: format '%lli' expects 
> argument of type 'long long int *', but argument 6 has type 'resource_size_t 
> *' [-Wformat=]
>    drivers/firmware/qemu_fw_cfg.c: In function 'fw_cfg_cmdline_get':
> >> drivers/firmware/qemu_fw_cfg.c:563:5: warning: format '%llx' expects 
> >> argument of type 'long long unsigned int', but argument 4 has type 
> >> 'resource_size_t' [-Wformat=]
>         fw_cfg_cmdline_dev->resource[0].start);
>         ^
>    drivers/firmware/qemu_fw_cfg.c:563:5: warning: format '%llx' expects 
> argument of type 'long long unsigned int', but argument 5 has type 
> 'resource_size_t' [-Wformat=]
>    drivers/firmware/qemu_fw_cfg.c:569:5: warning: format '%llx' expects 
> argument of type 'long long unsigned int', but argument 4 has type 
> 'resource_size_t' [-Wformat=]
>         fw_cfg_cmdline_dev->resource[2].start);
>         ^
>    drivers/firmware/qemu_fw_cfg.c:569:5: warning: format '%llx' expects 
> argument of type 'long long unsigned int', but argument 5 has type 
> 'resource_size_t' [-Wformat=]
> >> drivers/firmware/qemu_fw_cfg.c:569:5: warning: format '%llu' expects 
> >> argument of type 'long long unsigned int', but argument 6 has type 
> >> 'resource_size_t' [-Wformat=]
>    drivers/firmware/qemu_fw_cfg.c:569:5: warning: format '%llu' expects 
> argument of type 'long long unsigned int', but argument 7 has type 
> 'resource_size_t' [-Wformat=]
> 
> vim +510 drivers/firmware/qemu_fw_cfg.c
> 
>    504                /* consume "<size>" portion of command line argument */
>    505                size = memparse(arg, &str);
>    506        
>    507                /* get "@<base>[:<ctrl_off>:<data_off>]" chunks */
>    508                processed = sscanf(str, "@%lli%n:%lli:%lli%n",
>    509                                   &base, &consumed,
>  > 510                                   &ctrl_off, &data_off, &consumed);
>    511        
>    512                /* sscanf() must process precisely 1 or 3 chunks:
>    513                 * <base> is mandatory, optionally followed by 
> <ctrl_off>
>    514                 * and <data_off>;
>    515                 * there must be no extra characters after the last 
> chunk,
>    516                 * so str[consumed] must be '\0'.
>    517                 */
>    518                if (str[consumed] ||
>    519                    (processed != 1 && processed != 3))
>    520                        return -EINVAL;
>    521        
>    522                res[0].start = base;
>    523                res[0].end = base + size - 1;
>    524                res[0].flags = !strcmp(kp->name, "mmio") ? 
> IORESOURCE_MEM :
>    525                                                           
> IORESOURCE_IO;
>    526        
>    527                /* insert register offsets, if provided */
>    528                if (processed > 1) {
>    529                        res[1].name = "ctrl";
>    530                        res[1].start = ctrl_off;
>    531                        res[1].flags = IORESOURCE_REG;
>    532                        res[2].name = "data";
>    533                        res[2].start = data_off;
>    534                        res[2].flags = IORESOURCE_REG;
>    535                }
>    536        
>    537                /* "processed" happens to nicely match the number of 
> resources
>    538                 * we need to pass in to this platform device.
>    539                 */
>    540                fw_cfg_cmdline_dev = 
> platform_device_register_simple("fw_cfg",
>    541                                                PLATFORM_DEVID_NONE, 
> res, processed);
>    542                if (IS_ERR(fw_cfg_cmdline_dev))
>    543                        return PTR_ERR(fw_cfg_cmdline_dev);
>    544        
>    545                return 0;
>    546        }
>    547        
>    548        static int fw_cfg_cmdline_get(char *buf, const struct 
> kernel_param *kp)
>    549        {
>    550                /* stay silent if device was not configured via the 
> command
>    551                 * line, or if the parameter name (ioport/mmio) doesn't 
> match
>    552                 * the device setting
>    553                 */
>    554                if (!fw_cfg_cmdline_dev ||
>    555                    (!strcmp(kp->name, "mmio") ^
>    556                     (fw_cfg_cmdline_dev->resource[0].flags == 
> IORESOURCE_MEM)))
>    557                        return 0;
>    558        
>    559                switch (fw_cfg_cmdline_dev->num_resources) {
>    560                case 1:
>    561                        return snprintf(buf, PAGE_SIZE, 
> "address@hidden",
>    562                                        
> resource_size(&fw_cfg_cmdline_dev->resource[0]),
>  > 563                                        
> fw_cfg_cmdline_dev->resource[0].start);
>    564                case 3:
>    565                        return snprintf(buf, PAGE_SIZE, 
> "address@hidden:%llu:%llu",
>    566                                        
> resource_size(&fw_cfg_cmdline_dev->resource[0]),
>    567                                        
> fw_cfg_cmdline_dev->resource[0].start,
>    568                                        
> fw_cfg_cmdline_dev->resource[1].start,
>  > 569                                        
> fw_cfg_cmdline_dev->resource[2].start);
>    570                }
>    571        
>    572                /* Should never get here */
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation