* Re: [PATCH 2/2] ALSA: hda: cirrus_scodec: Add KUnit test [not found] ` <ZQoILN6QCjzosCOs@google.com> @ 2023-09-20 6:51 ` Takashi Iwai 2023-09-20 8:27 ` Richard Fitzgerald 2023-09-20 15:19 ` Andy Shevchenko 0 siblings, 2 replies; 4+ messages in thread From: Takashi Iwai @ 2023-09-20 6:51 UTC (permalink / raw) To: Nick Desaulniers Cc: Richard Fitzgerald, tiwai, alsa-devel, patches, linux-kernel, llvm, Andy Shevchenko, Daniel Scally, Heikki Krogerus, Sakari Ailus, linux-acpi 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; } ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] ALSA: hda: cirrus_scodec: Add KUnit test 2023-09-20 6:51 ` [PATCH 2/2] ALSA: hda: cirrus_scodec: Add KUnit test Takashi Iwai @ 2023-09-20 8:27 ` Richard Fitzgerald 2023-09-20 8:42 ` Takashi Iwai 2023-09-20 15:19 ` Andy Shevchenko 1 sibling, 1 reply; 4+ messages in thread From: Richard Fitzgerald @ 2023-09-20 8:27 UTC (permalink / raw) To: Takashi Iwai, Nick Desaulniers Cc: tiwai, alsa-devel, patches, linux-kernel, llvm, Andy Shevchenko, Daniel Scally, Heikki Krogerus, Sakari Ailus, linux-acpi 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). ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] ALSA: hda: cirrus_scodec: Add KUnit test 2023-09-20 8:27 ` Richard Fitzgerald @ 2023-09-20 8:42 ` Takashi Iwai 0 siblings, 0 replies; 4+ messages in thread From: Takashi Iwai @ 2023-09-20 8:42 UTC (permalink / raw) To: Richard Fitzgerald Cc: Nick Desaulniers, tiwai, alsa-devel, patches, linux-kernel, llvm, Andy Shevchenko, Daniel Scally, Heikki Krogerus, Sakari Ailus, linux-acpi 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); > > + 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). 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] ALSA: hda: cirrus_scodec: Add KUnit test 2023-09-20 6:51 ` [PATCH 2/2] ALSA: hda: cirrus_scodec: Add KUnit test Takashi Iwai 2023-09-20 8:27 ` Richard Fitzgerald @ 2023-09-20 15:19 ` Andy Shevchenko 1 sibling, 0 replies; 4+ messages in thread From: Andy Shevchenko @ 2023-09-20 15:19 UTC (permalink / raw) To: Takashi Iwai Cc: Nick Desaulniers, Richard Fitzgerald, tiwai, alsa-devel, patches, linux-kernel, llvm, Daniel Scally, Heikki Krogerus, Sakari Ailus, linux-acpi On Wed, Sep 20, 2023 at 08:51:57AM +0200, 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. Thank you, I think it's quite easy to fix. Lemme cook the patch... -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-09-20 15:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20230918095129.440-1-rf@opensource.cirrus.com>
[not found] ` <20230918095129.440-3-rf@opensource.cirrus.com>
[not found] ` <ZQoILN6QCjzosCOs@google.com>
2023-09-20 6:51 ` [PATCH 2/2] ALSA: hda: cirrus_scodec: Add KUnit test Takashi Iwai
2023-09-20 8:27 ` Richard Fitzgerald
2023-09-20 8:42 ` Takashi Iwai
2023-09-20 15:19 ` Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox