Changing argument type from va_list to struct va_format * allows
to simplify variadic argument handling with va_format_call helper.
Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
---
drivers/base/core.c | 50 +++++++-------------------------------------------
1 file changed, 7 insertions(+), 43 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 513e5ef8a6da..4d76b67a87e3 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4965,30 +4965,12 @@ define_dev_printk_level(_dev_info, KERN_INFO);
#endif
static void __dev_probe_failed(const struct device *dev, int err, bool fatal,
- const char *fmt, va_list vargsp)
+ const char *fmt, struct va_format *vaf)
{
- struct va_format vaf;
- va_list vargs;
-
- /*
- * 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;
-
switch (err) {
case -EPROBE_DEFER:
- device_set_deferred_probe_reason(dev, &vaf);
- dev_dbg(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
+ device_set_deferred_probe_reason(dev, vaf);
+ dev_dbg(dev, "error %pe: %pV", ERR_PTR(err), vaf);
break;
case -ENOMEM:
@@ -4998,13 +4980,11 @@ static void __dev_probe_failed(const struct device *dev, int err, bool fatal,
default:
/* Log fatal final failures as errors, otherwise produce warnings */
if (fatal)
- dev_err(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
+ dev_err(dev, "error %pe: %pV", ERR_PTR(err), vaf);
else
- dev_warn(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
+ dev_warn(dev, "error %pe: %pV", ERR_PTR(err), vaf);
break;
}
-
- va_end(vargs);
}
/**
@@ -5042,15 +5022,7 @@ static void __dev_probe_failed(const struct device *dev, int err, bool fatal,
*/
int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
{
- va_list vargs;
-
- va_start(vargs, fmt);
-
- /* Use dev_err() for logging when err doesn't equal -EPROBE_DEFER */
- __dev_probe_failed(dev, err, true, fmt, vargs);
-
- va_end(vargs);
-
+ va_format_call(fmt, __dev_probe_failed, dev, err, true, fmt, va_format_arg);
return err;
}
EXPORT_SYMBOL_GPL(dev_err_probe);
@@ -5090,15 +5062,7 @@ EXPORT_SYMBOL_GPL(dev_err_probe);
*/
int dev_warn_probe(const struct device *dev, int err, const char *fmt, ...)
{
- va_list vargs;
-
- va_start(vargs, fmt);
-
- /* Use dev_warn() for logging when err doesn't equal -EPROBE_DEFER */
- __dev_probe_failed(dev, err, false, fmt, vargs);
-
- va_end(vargs);
-
+ va_format_call(fmt, __dev_probe_failed, dev, err, false, fmt, va_format_arg);
return err;
}
EXPORT_SYMBOL_GPL(dev_warn_probe);
--
2.43.0I 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/
On Mon 2025-12-01 10:31:24, Andrzej Hajda wrote:
> Changing argument type from va_list to struct va_format * allows
> to simplify variadic argument handling with va_format_call helper.
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 513e5ef8a6da..4d76b67a87e3 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -4965,30 +4965,12 @@ define_dev_printk_level(_dev_info, KERN_INFO);
> #endif
>
> static void __dev_probe_failed(const struct device *dev, int err, bool fatal,
> - const char *fmt, va_list vargsp)
> + const char *fmt, struct va_format *vaf)
> {
> - struct va_format vaf;
> - va_list vargs;
> -
> - /*
> - * 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?
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()
> - dev_dbg(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
This function uses the already processed copy of the arguments.
IMHO, it might print a garbage because of this. IMHO, it should use
the original va_list() or might need its own copy.
Note that this call does not modify the va_list because it uses "%pV"
and vsprintf() creates its own copy in this case, see va_format()
in lib/vsprintf.c.
> + device_set_deferred_probe_reason(dev, vaf);
> + dev_dbg(dev, "error %pe: %pV", ERR_PTR(err), vaf);
> break;
>
> case -ENOMEM:
> @@ -4998,13 +4980,11 @@ static void __dev_probe_failed(const struct device *dev, int err, bool fatal,
> default:
> /* Log fatal final failures as errors, otherwise produce warnings */
> if (fatal)
> - dev_err(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
> + dev_err(dev, "error %pe: %pV", ERR_PTR(err), vaf);
> else
> - dev_warn(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
> + dev_warn(dev, "error %pe: %pV", ERR_PTR(err), vaf);
This should be fine because of using "%pV".
> break;
> }
> -
> - va_end(vargs);
> }
>
> /**
> @@ -5042,15 +5022,7 @@ static void __dev_probe_failed(const struct device *dev, int err, bool fatal,
> */
> int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
> {
> - va_list vargs;
> -
> - va_start(vargs, fmt);
> -
> - /* Use dev_err() for logging when err doesn't equal -EPROBE_DEFER */
> - __dev_probe_failed(dev, err, true, fmt, vargs);
> -
> - va_end(vargs);
> -
> + va_format_call(fmt, __dev_probe_failed, dev, err, true, fmt, va_format_arg);
> return err;
> }
> EXPORT_SYMBOL_GPL(dev_err_probe);
> @@ -5090,15 +5062,7 @@ EXPORT_SYMBOL_GPL(dev_err_probe);
> */
> int dev_warn_probe(const struct device *dev, int err, const char *fmt, ...)
> {
> - va_list vargs;
> -
> - va_start(vargs, fmt);
> -
> - /* Use dev_warn() for logging when err doesn't equal -EPROBE_DEFER */
> - __dev_probe_failed(dev, err, false, fmt, vargs);
> -
> - va_end(vargs);
> -
> + va_format_call(fmt, __dev_probe_failed, dev, err, false, fmt, va_format_arg);
> return err;
> }
> EXPORT_SYMBOL_GPL(dev_warn_probe);
Best Regards,
Petr
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/
>
> On Mon 2025-12-01 10:31:24, Andrzej Hajda wrote:
>> Changing argument type from va_list to struct va_format * allows
>> to simplify variadic argument handling with va_format_call helper.
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index 513e5ef8a6da..4d76b67a87e3 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -4965,30 +4965,12 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>> #endif
>>
>> static void __dev_probe_failed(const struct device *dev, int err, bool fatal,
>> - const char *fmt, va_list vargsp)
>> + const char *fmt, struct va_format *vaf)
>> {
>> - struct va_format vaf;
>> - va_list vargs;
>> -
>> - /*
>> - * 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.
> 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.
Regards
Andrzej
>
>> - dev_dbg(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
> This function uses the already processed copy of the arguments.
> IMHO, it might print a garbage because of this. IMHO, it should use
> the original va_list() or might need its own copy.
>
> Note that this call does not modify the va_list because it uses "%pV"
> and vsprintf() creates its own copy in this case, see va_format()
> in lib/vsprintf.c.
>
>> + device_set_deferred_probe_reason(dev, vaf);
>> + dev_dbg(dev, "error %pe: %pV", ERR_PTR(err), vaf);
>> break;
>>
>> case -ENOMEM:
>> @@ -4998,13 +4980,11 @@ static void __dev_probe_failed(const struct device *dev, int err, bool fatal,
>> default:
>> /* Log fatal final failures as errors, otherwise produce warnings */
>> if (fatal)
>> - dev_err(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
>> + dev_err(dev, "error %pe: %pV", ERR_PTR(err), vaf);
>> else
>> - dev_warn(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
>> + dev_warn(dev, "error %pe: %pV", ERR_PTR(err), vaf);
> This should be fine because of using "%pV".
>
>> break;
>> }
>> -
>> - va_end(vargs);
>> }
>>
>> /**
>> @@ -5042,15 +5022,7 @@ static void __dev_probe_failed(const struct device *dev, int err, bool fatal,
>> */
>> int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
>> {
>> - va_list vargs;
>> -
>> - va_start(vargs, fmt);
>> -
>> - /* Use dev_err() for logging when err doesn't equal -EPROBE_DEFER */
>> - __dev_probe_failed(dev, err, true, fmt, vargs);
>> -
>> - va_end(vargs);
>> -
>> + va_format_call(fmt, __dev_probe_failed, dev, err, true, fmt, va_format_arg);
>> return err;
>> }
>> EXPORT_SYMBOL_GPL(dev_err_probe);
>> @@ -5090,15 +5062,7 @@ EXPORT_SYMBOL_GPL(dev_err_probe);
>> */
>> int dev_warn_probe(const struct device *dev, int err, const char *fmt, ...)
>> {
>> - va_list vargs;
>> -
>> - va_start(vargs, fmt);
>> -
>> - /* Use dev_warn() for logging when err doesn't equal -EPROBE_DEFER */
>> - __dev_probe_failed(dev, err, false, fmt, vargs);
>> -
>> - va_end(vargs);
>> -
>> + va_format_call(fmt, __dev_probe_failed, dev, err, false, fmt, va_format_arg);
>> return err;
>> }
>> EXPORT_SYMBOL_GPL(dev_warn_probe);
> Best Regards,
> Petr
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
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@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;
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().
>
© 2016 - 2026 Red Hat, Inc.