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

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':

quoted

quoted

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

quoted

quoted

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':

quoted

quoted

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=]

quoted

quoted

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, "0x%llx@0x%llx",
   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, "0x%llx@0x%llx:%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