Message-ID: <94c02fb2-3407-4efc-a80f-305140e64b94@intel.com>
Date: Tue, 2 Dec 2025 19:03:37 +0100
From: "Hajda, Andrzej" <andrzej.hajda@...el.com>
To: Petr Mladek <pmladek@...e.com>
CC: Steven Rostedt <rostedt@...dmis.org>, John Ogness
	<john.ogness@...utronix.de>, Sergey Senozhatsky <senozhatsky@...omium.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki"
	<rafael@...nel.org>, Danilo Krummrich <dakr@...nel.org>, Andrew Morton
	<akpm@...ux-foundation.org>, Vlastimil Babka <vbabka@...e.cz>, "Suren
 Baghdasaryan" <surenb@...gle.com>, Michal Hocko <mhocko@...e.com>, "Brendan
 Jackman" <jackmanb@...gle.com>, Johannes Weiner <hannes@...xchg.org>, Zi Yan
	<ziy@...dia.com>, Christoph Lameter <cl@...two.org>, David Rientjes
	<rientjes@...gle.com>, Roman Gushchin <roman.gushchin@...ux.dev>, Harry Yoo
	<harry.yoo@...cle.com>, <linux-kernel@...r.kernel.org>, <linux-mm@...ck.org>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, Rasmus Villemoes
	<linux@...musvillemoes.dk>
Subject: Re: [PATCH v2 3/5] drivers/core: simplify variadic args handling


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

Powered by blists - more mailing lists