On Wed, 20 Sep 2023 10:27:58 +0200, Richard Fitzgerald wrote:
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);*arg = template; }
- template.args[0] = gpio_num;
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).
Yeah, fixing the macro would be ideal.
An easy and compromise solution would be to factor out the ARRAY_SIZE() call and allow giving nargs explicitly.
e.g.
--- a/include/linux/property.h +++ b/include/linux/property.h @@ -285,13 +285,18 @@ struct software_node_ref_args { u64 args[NR_FWNODE_REFERENCE_ARGS]; };
-#define SOFTWARE_NODE_REFERENCE(_ref_, ...) \ +#define __SOFTWARE_NODE_REFERENCE(_ref_, _nargs_, ...) \ (const struct software_node_ref_args) { \ .node = _ref_, \ - .nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1, \ + .nargs = _nargs_, \ .args = { __VA_ARGS__ }, \ }
+#define SOFTWARE_NODE_REFERENCE(_ref_, ...) \ + __SOFTWARE_NODE_REFERENCE(_ref_,\ + ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1,\ + ##__VA_ARGS__) + /** * struct property_entry - "Built-in" device property representation. * @name: Name of the property. --- a/sound/pci/hda/cirrus_scodec_test.c +++ b/sound/pci/hda/cirrus_scodec_test.c @@ -148,7 +148,7 @@ 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, 2, gpio_num, 0);
*arg = template; }
... though I'm not convinced by this change, either.
Takashi