Re: [PATCH v2 3/5] drivers/core: simplify variadic args handling

Andy Shevchenko
2025-12-03T14:47:55Z

On Tue, Dec 02, 2025 at 07:03:37PM +0100, Hajda, Andrzej wrote:
> W dniu 02.12.2025 o 16:51, Petr Mladek pisze:
> > I am adding Andy and Rasmus into Cc who are active vsprintf-related
> > code reviewers...
> >
> > You might see the entire patchset at
> > https://lore.kernel.org/all/20251201-va_format_call-v2-0-2906f3093b60@intel.com/

TBH, I don't like the result. There are two problems with readability:

1) macro well hides the actual low-level call, hard to parse from its
parameters;

2) sometimes it has va_format_call(fmt, ..., fmt, ...) which is confusing.

Implementation is also doubtful (to me) as GCC extension. Can't it rather
return an error code and use something like do { } while (0) inside? OTOH,
may be this is not feasible in a clean way...

And what is the motivation? Just make less LoCs? I would really like to see
at least vmlinux sizes, the reports that GCC _and_ clang are both happy with
the compilation as of `make W=1` of this on both 32- and 64-bit cases.

Does it solve any issue? Does it bring any consistency or standardisation here?

> > On Mon 2025-12-01 10:31:24, Andrzej Hajda wrote:

...

> > > - /*
> > > - * On x86_64 and possibly on other architectures, va_list is actually a
> > > - * size-1 array containing a structure. As a result, function parameter
> > > - * vargsp decays from T[1] to T*, and &vargsp has type T** rather than
> > > - * T(*)[1], which is expected by its assignment to vaf.va below.
> > > - *
> > > - * One standard way to solve this mess is by creating a copy in a local
> > > - * variable of type va_list and then using a pointer to that local copy
> > > - * instead, which is the approach employed here.
> > > - */
> > > - va_copy(vargs, vargsp);
> > > -
> > > - vaf.fmt = fmt;
> > > - vaf.va = &vargs;

> > I am always a bit lost when using this API.
> > Why is it safe to remove the va_copy() here, please?
>
> Not very familiar with this workaround, just my thoughts about it.
>
> It is just va_list is compiler's private implementation, which can be
> anything.
>
> And if it happens to be T[1], it's type decays to T* if it is type of
> argument of the function.
>
> So vargsp is in fact of type T*, and &vargs is of type T** and it does not
> point to va_list anymore.
>
> So in short passing va_list to a function, which takes a pointer to the arg
> is problematic.
>
> va_format_call DOES NOT pass va_list to a function, so it seems to be safe.

I'm sorry, I can't be helpful here, as I am not well familiar
with va_*() stuff. The idea is interesting, nevertheless, but
see above.

> > The va_format_call() uses va_start()/va_end() which is replacing
> > these calls in dev_err_probe() and dev_warn_probe().
> >
> > It is possible that the original code was actually wrong because
> > it uses the same copy (&vaf) everywhere, see below.
> >
> > > switch (err) {
> > > case -EPROBE_DEFER:
> > > - device_set_deferred_probe_reason(dev, &vaf);
> > This function processes the arguments via:
> >
> > + device_set_deferred_probe_reason()
> > + kasprintf()
> > + va_start()/va_end()
>
> This va_start/va_end is for var_args of kasprintf, not for &vaf, I hope
> parsing %pV uses va_copy.

Yes, it does call va_copy().

--
With Best Regards,
Andy Shevchenko