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

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

On 19/06/25 5:30 PM, Andy Shevchenko wrote:
> On Thu, Jun 19, 2025 at 03:05:10PM +0530, Anshuman Khandual wrote:
>> 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.
> 
> No it's worse. One may easily get the same from lore. Can you give a good
> justification for the polluting message with 8 lines over a single line of the
> useful information, please?

Will drop the Cc: list from the commit message and move it below '---'
cutter line as suggested earlier.

> 
>> 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.
> 
> It's not an MM subsystem.
> 
> ...
> 
>>>> +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
> 
> Please, make it 64-bit address. The example as is is quite confusing.
Agreed it is some what confusing - will fix it.

> 
>>>> +        %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.
> 
> Yes, see above why I have such a question.

Got it.

> 
> ...
> 
>>>> +			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 ?
> 
> It's dup. Please, take your time to find the very similar piece of code in one
> of the helper functions we have.

Are you referring to special_hex_number() ?

> 
> I recommend you to look at the history of the changes in this file for when the
> new specifier was added and how it is implemented> 
> ...
> 
>> Could you please kindly elaborate on the code duplication problem
>> you have mentioned earlier. I might not understand your concern
>> here correctly.
> 
> Just find the same or similar pieces of code elsewhere in the same file.
> Use them.
> Will go through previous print format additions and re-work the patches
accommodating various suggestions. Thanks for your review.