[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <92a89dbb-7031-459a-ad5a-69b29b08cfc8@arm.com>
Date: Fri, 20 Jun 2025 12:23:47 +0530
From: Anshuman Khandual <anshuman.khandual@....com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: linux-mm@...ck.org, Rasmus Villemoes <linux@...musvillemoes.dk>,
Sergey Senozhatsky <senozhatsky@...omium.org>, Petr Mladek
<pmladek@...e.com>, Steven Rostedt <rostedt@...dmis.org>,
Jonathan Corbet <corbet@....net>, Andrew Morton <akpm@...ux-foundation.org>,
David Hildenbrand <david@...hat.com>, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org
Subject: Re: [RFC 1/2] lib/vsprintf: Add support for pte_t
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@...e.com>
>>>> Cc: Steven Rostedt <rostedt@...dmis.org>
>>>> Cc: Jonathan Corbet <corbet@....net>
>>>> Cc: Andrew Morton <akpm@...ux-foundation.org>
>>>> Cc: David Hildenbrand <david@...hat.com>
>>>> Cc: linux-doc@...r.kernel.org
>>>> Cc: linux-kernel@...r.kernel.org
>>>> Cc: linux-mm@...ck.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@....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.
Powered by blists - more mailing lists