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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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?

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

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

...

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

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.

-- 
With Best Regards,
Andy Shevchenko