linux-kernel - Re: [PATCH 2/2] ALSA: hda: cirrus

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 20 Sep 2023 09:27:58 +0100
From:   Richard Fitzgerald <rf@...nsource.cirrus.com>
To:     Takashi Iwai <tiwai@...e.de>,
        Nick Desaulniers <ndesaulniers@...gle.com>
CC:     <tiwai@...e.com>, <alsa-devel@...a-project.org>,
        <patches@...nsource.cirrus.com>, <linux-kernel@...r.kernel.org>,
        <llvm@...ts.linux.dev>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Daniel Scally <djrscally@...il.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        <linux-acpi@...r.kernel.org>
Subject: Re: [PATCH 2/2] ALSA: hda: cirrus_scodec: Add KUnit test

On 20/9/23 07:51, Takashi Iwai wrote:
> On Tue, 19 Sep 2023 22:44:28 +0200,
> Nick Desaulniers wrote:
>>
>> On Mon, Sep 18, 2023 at 10:51:29AM +0100, Richard Fitzgerald wrote:
> (snip)
>>> +static void cirrus_scodec_test_set_gpio_ref_arg(struct software_node_ref_args *arg,
>>> +						int gpio_num)
>>> +{
>>> +	struct software_node_ref_args template =
>>> +		SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, gpio_num, 0);
>>
>> I'm observing the following error when building with:
>>
>> $ make LLVM=1 -j128 allmodconfig sound/pci/hda/cirrus_scodec_test.o
>>
>> sound/pci/hda/cirrus_scodec_test.c:151:60: error: initializer element is not a compile-time constant
>>    151 |                 SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, gpio_num, 0);
>>        |                                                                          ^~~~~~~~
>> /builds/linux/include/linux/property.h:291:37: note: expanded from macro 'SOFTWARE_NODE_REFERENCE'
>>    291 |         .nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1, \
>>        |                                            ^~~~~~~~~~~
>> /builds/linux/include/linux/kernel.h:57:75: note: expanded from macro 'ARRAY_SIZE'
>>     57 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
>>        |                                                                           ^~~
>> /builds/linux/include/linux/compiler.h:228:59: note: expanded from macro '__must_be_array'
>>    228 | #define __must_be_array(a)      BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
>>        |                                                                ^
>> /builds/linux/include/linux/compiler_types.h:366:63: note: expanded from macro '__same_type'
>>    366 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
>>        |                                                               ^
>> /builds/linux/include/linux/build_bug.h:16:62: note: expanded from macro 'BUILD_BUG_ON_ZERO'
>>     16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
>>        |                                                              ^
> 
> Hm, this looks like some inconsistent handling of the temporary array
> passed to ARRAY_SIZE() in the SOFTWARE_NODE_REFERENCE macro.  LLVM
> can't treat it if it contains a variable in the given array, while GCC
> doesn't care.
> 
> A hackish workaround would be the patch like below, but it's really
> ugly.  Ideally speaking, it should be fixed in linux/properties.h, but
> I have no idea how to fix there for LLVM.
> 
> Adding more relevant people to Cc.
> 
> 
> thanks,
> 
> Takashi
> 
> --- a/sound/pci/hda/cirrus_scodec_test.c
> +++ b/sound/pci/hda/cirrus_scodec_test.c
> @@ -148,8 +148,9 @@ static void cirrus_scodec_test_set_gpio_ref_arg(struct software_node_ref_args *a
>   						int gpio_num)
>   {
>   	struct software_node_ref_args template =
> -		SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, gpio_num, 0);
> +		SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, 0, 0);
>   
> +	template.args[0] = gpio_num;
>   	*arg = template;
>   }
>   

The tests must generate sw nodes dynamically, not a fixed const struct.
I wanted to avoid directly accessing the internals of the SW node
structures. Use only the macros.

I can rewrite this code to open-code the construction of the
software_node_ref_args. Or I can wait a while in case anyone has a
suggested fix for the macros.

Its seems reasonable that you should be able to create software nodes
dynamically. A "real" use of these might need to construct the data from
some other data that is not known at runtime (for example, where the
ACPI provides some information but not the necessary info).