Re: [RFC 1/2] lib/vsprintf: Add support for pte_t

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 18/06/25 11:16 PM, Andy Shevchenko wrote:
> On Wed, Jun 18, 2025 at 09:42:34AM +0530, Anshuman Khandual wrote:
>> Add a new format for printing page table entries.
> 
>> Cc: Petr Mladek <pmladek@suse.com>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Jonathan Corbet <corbet@lwn.net>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: linux-doc@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: linux-mm@kvack.org
> 
> Please. move these to be after the '---' cutter line below. Just leave SoB tag
> alone. This will have the same effect w/o polluting commit message.
> 
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
> 
> (somewhere here is a good place for all your Cc: tags)

Is not it better to also capture the Cc: list in the commit message.
Seems like such has been the practice for various patches on the MM
list. But not sure if that is an expected standard for all patches.

> 
> ...
> 
>> +        %ppte
> 
> I believe you can take %pte.

Yes - that should be possible.

> 
> ...
> 
>> +Print standard page table entry pte_t.
>> +
>> +Passed by reference.
>> +
>> +Examples for a 64 bit page table entry, given &(u64)0xc0ffee::
> 
> What does this mean?

64 bit address containing value the 0xc0ffee

> 
>> +        %ppte   0x00c0ffee
> 
> Can it be ever 64-bit?
I am sorry - did not get that. pte_t contained value can be 64
bits if that's what you meant.

> 
> ...
> 
>> +			spec.field_width = 10;
>> +			spec.precision = 8;
>> +			spec.base = 16;
>> +			spec.flags = SPECIAL | SMALL | ZEROPAD;
> 
> Do not duplicate code we have already in the file.
I am sorry - did not get that. Is the above flag combination some
how wrong ?

> 
>> +			if (sizeof(pte_t) == sizeof(u64)) {
>> +				u64 val = pte_val(*pte);
>> +
>> +				return number(buf, end, val, spec);
>> +			}
> 
> Ditto.
> 
>> +			WARN_ONCE(1, "Non standard pte_t\n");
> 
> (almost) Ditto,
> 
>> +			return error_string(buf, end, "(einval)", spec);
> 
> Ditto.
> 
>> +		}
>> +		fallthrough;
> 
> Please, avoid this, it makes code much harder to read and maintain.
> See above how.
> 

Could you please kindly elaborate on the code duplication problem
you have mentioned earlier. I might not understand your concern
here correctly.