Re: [PATCH v6 06/15] cpufreq: Add Hardware P-State (HWP) driver

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Tue, Jul 25, 2023 at 2:27 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 24.07.2023 21:49, Jason Andryuk wrote:
> > On Mon, Jul 24, 2023 at 12:15 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >> On 24.07.2023 14:58, Jason Andryuk wrote:
> >>> --- /dev/null
> >>> +++ b/xen/arch/x86/acpi/cpufreq/hwp.c
> >>> +#define hwp_err(cpu, fmt, ...) \
> >>> +    printk(XENLOG_ERR "HWP: CPU%u error: " fmt, cpu, ##__VA_ARGS__)
> >>> +#define hwp_info(fmt, ...)    printk(XENLOG_INFO "HWP: " fmt, 
> >>> ##__VA_ARGS__)
> >>
> >> I'm not convinced ", ##__VA_ARGS__" is a good construct to use. I notice
> >> we have a few instances (mainly in code inherited from elsewhere), but I
> >> think it ought to either be plain C99 style (without the ##) or purely
> >> the gcc extension form (not using __VA_ARGS__).
> >
> > Using plain __VA_ARGS__ doesn't work for the cases without arguments:
> > arch/x86/acpi/cpufreq/hwp.c:78:53: error: expected expression before ‘)’ 
> > token
> >    78 |         printk(XENLOG_DEBUG "HWP: " fmt, __VA_ARGS__);  \
> >       |                                                     ^
> > arch/x86/acpi/cpufreq/hwp.c:201:9: note: in expansion of macro ‘hwp_verbose’
> >   201 |         hwp_verbose("disabled: No energy/performance
> > preference available");
> >       |         ^~~~~~~~~~~
>
> Of course.
>
> > I can use "__VA_OPT__(,) __VA_ARGS__" though.
>
> __VA_OPT__ is yet newer than C99, so this is an option only if all
> compilers we continue to support actually know of this.

Right, sorry.

> We have no
> uses of it in the codebase so far, which suggests you might best go
> with the longstanding gcc extension here.

FTAOD, "##__VA_ARGS__" is the longstanding extension?  It's the only
one I've been able to get to compile.  I'm reading
https://gcc.gnu.org/onlinedocs/cpp/Variadic-Macros.html and it
mentions a few different extensions.

This part seemed promising:
"""
This has been fixed in C++20, and GNU CPP also has a pair of
extensions which deal with this problem.

First, in GNU CPP, and in C++ beginning in C++20, you are allowed to
leave the variable argument out entirely:

eprintf ("success!\n")
     → fprintf(stderr, "success!\n", );
"""

However, it doesn't seem to actually work for me.  I still get an
error like the one above for plain __VA_ARGS__.  That is for:

    #define hwp_err(cpu, fmt, args...) \
        printk(XENLOG_ERR "HWP: CPU%u error: " fmt, cpu, args)

I can drop "fmt" from hwp_info() and hwp_verbose() to just use
__VA_ARGS__, but that doesn't work for hwp_err() since we want to
re-order fmt and cpu inside the macro.

Thanks,
Jason