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

[Hajda, Andrzej]

W dniu 03.12.2025 o 15:47, Andy Shevchenko pisze:
> 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@xxxxxxxxx/
> 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;

I hope 'va_format_call' is a strong suggestion that sth is to be called.

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

Many functions sometimes takes the same argument twice.

1st argument is to indicate beginning of the '...' in caller function 
args list, quite natural.

2nd argument is function name, and the rest are it's arguments, quite 
natural.

Since C23 standard, we could omit the 1st argument [1], but this is 
potential future.

[1]: https://en.cppreference.com/w/c/variadic/va_start


> 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...

???, expression statement "({ ...})" is present even in macro samples in 
kernel coding-style [2].

[2] https://www.kernel.org/doc/html/latest/process/coding-style.html

> And what is the motivation? Just make less LoCs?

Also, but first:

- encapsulate common repeatable pattern into one macro, with clear 
purpose - forward variable args to va_format aware function.

- simplify handling variable arguments: it is easier to write single 
line instead of ten, usually interleaved by other declarations and code.

Is it so hard to see it? Have you seen other patches - quite good examples.

>   I would really like to see at least vmlinux sizes,

What for? This not about binary size optimisation.

> 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.


I do not expect any problems here, but will check :)


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

No. Yes.


Regards

Andrzej


> > > 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().