Re: [PATCH v2 11/15] ASoC: Intel: avs: Machine board registration

[Vineet Gupta <vgupta@kernel.org>]

On 5/9/22 18:08, Amadeusz Sławiński wrote:
> On 5/9/2022 1:58 PM, kernel test robot wrote:
>> Hi Cezary,
>>
>> I love your patch! Perhaps something to improve:
>>
>> [auto build test WARNING on broonie-sound/for-next]
>> [also build test WARNING on next-20220506]
>> [cannot apply to tiwai-sound/for-next v5.18-rc6]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch]
>>
>> url: 
>> https://github.com/intel-lab-lkp/linux/commits/Cezary-Rojewski/ASoC-Intel-avs-Driver-core-and-PCM-operations/20220509-165656
>> base: 
>> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 
>> for-next
>> config: arc-allyesconfig 
>> (https://download.01.org/0day-ci/archive/20220509/202205091954.7hT2TBhd-lkp@intel.com/config)
>> compiler: arceb-elf-gcc (GCC) 11.3.0
>> reproduce (this is a W=1 build):
>>          wget 
>> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross 
>> -O ~/bin/make.cross
>>          chmod +x ~/bin/make.cross
>>          # 
>> https://github.com/intel-lab-lkp/linux/commit/2ef25400a7aee5a8e75c2ccdb5618be31c9f6809
>>          git remote add linux-review 
>> https://github.com/intel-lab-lkp/linux
>>          git fetch --no-tags linux-review 
>> Cezary-Rojewski/ASoC-Intel-avs-Driver-core-and-PCM-operations/20220509-165656
>>          git checkout 2ef25400a7aee5a8e75c2ccdb5618be31c9f6809
>>          # save the config file
>>          mkdir build_dir && cp config build_dir/.config
>>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 
>> make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash sound/soc/intel/avs/
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <lkp@intel.com>
>>
>> All warnings (new ones prefixed by >>):
>>
>>     In file included from include/linux/device.h:15,
>>                      from include/linux/acpi.h:15,
>>                      from sound/soc/intel/avs/board_selection.c:9:
>>     sound/soc/intel/avs/board_selection.c: In function 
>> 'avs_register_i2s_board':
>>>> sound/soc/intel/avs/board_selection.c:328:36: warning: format '%ld' 
>>>> expects argument of type 'long int', but argument 5 has type 'int' 
>>>> [-Wformat=]
>>       328 |                 dev_err(adev->dev, "Platform supports %d 
>> SSPs but board %s requires SSP%ld\n",
>>           | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>     include/linux/dev_printk.h:110:30: note: in definition of macro 
>> 'dev_printk_index_wrap'
>>       110 |                 _p_func(dev, fmt, 
>> ##__VA_ARGS__);                       \
>>           |                              ^~~
>>     include/linux/dev_printk.h:144:56: note: in expansion of macro 
>> 'dev_fmt'
>>       144 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, 
>> dev_fmt(fmt), ##__VA_ARGS__)
>> |                                                        ^~~~~~~
>>     sound/soc/intel/avs/board_selection.c:328:17: note: in expansion 
>> of macro 'dev_err'
>>       328 |                 dev_err(adev->dev, "Platform supports %d 
>> SSPs but board %s requires SSP%ld\n",
>>           |                 ^~~~~~~
>>     sound/soc/intel/avs/board_selection.c:328:90: note: format string 
>> is defined here
>>       328 |                 dev_err(adev->dev, "Platform supports %d 
>> SSPs but board %s requires SSP%ld\n",
>> | ~~^
>> | |
>> | long int
>> | %d
>>
>>
>> vim +328 sound/soc/intel/avs/board_selection.c
>>
>>     318
>>     319    static int avs_register_i2s_board(struct avs_dev *adev, 
>> struct snd_soc_acpi_mach *mach)
>>     320    {
>>     321        struct platform_device *board;
>>     322        int num_ssps;
>>     323        char *name;
>>     324        int ret;
>>     325
>>     326        num_ssps = adev->hw_cfg.i2s_caps.ctrl_count;
>>     327        if (fls(mach->mach_params.i2s_link_mask) > num_ssps) {
>>   > 328            dev_err(adev->dev, "Platform supports %d SSPs but 
>> board %s requires SSP%ld\n",
>>     329                num_ssps, mach->drv_name, 
>> __fls(mach->mach_params.i2s_link_mask));
>>     330            return -ENODEV;
>>     331        }
>>     332
>>     333        name = devm_kasprintf(adev->dev, GFP_KERNEL, 
>> "%s.%d-platform", mach->drv_name,
>>     334 mach->mach_params.i2s_link_mask);
>>     335        if (!name)
>>     336            return -ENOMEM;
>>     337
>>     338        ret = avs_i2s_platform_register(adev, name, 
>> mach->mach_params.i2s_link_mask, mach->pdata);
>>     339        if (ret < 0)
>>     340            return ret;
>>     341
>>     342        mach->mach_params.platform = name;
>>     343
>>     344        board = platform_device_register_data(NULL, 
>> mach->drv_name, mach->mach_params.i2s_link_mask,
>>     345                              (const void *)mach, sizeof(*mach));
>>     346        if (IS_ERR(board)) {
>>     347            dev_err(adev->dev, "ssp board register failed\n");
>>     348            return PTR_ERR(board);
>>     349        }
>>     350
>>     351        ret = devm_add_action(adev->dev, 
>> board_pdev_unregister, board);
>>     352        if (ret < 0) {
>>     353            platform_device_unregister(board);
>>     354            return ret;
>>     355        }
>>     356
>>     357        return 0;
>>     358    }
>>     359
>>
>
> Kernel test robot warns us about __fls and it is right, as __fls 
> depending on architecture returns either unsigned int or unsigned long.
> But I would say that this seems questionable, as I would expect 
> consistent return value between arches, especially for functions where 
> we operate on bits and probably don't want inconsistent results.
>
> Generic asm header [1] seems to suggest that it should accept unsigned 
> long as parameter and return unsigned long. It seems however that arc 
> accepts unsigned long as argument and returns int, while m68k uses int 
> for argument and return value...
>
> Adding relevant architecture maintainers to CC.
>
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/asm-generic/bitops/__fls.h

In generic code __fls() returns long, while fls() return int, which is 
weird as well. Anyhow for this error, ARC indeed needs fixing.
Do u want to send a patch ?

Thx,
-Vineet