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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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