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

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

On 19/06/25 7:31 PM, Petr Mladek wrote:
> On Wed 2025-06-18 09:42:34, Anshuman Khandual wrote:
>> Add a new format for printing page table entries.
> 
> How many users do you explect, please?
> 
> This patch adds only one caller. It does not justify the added complexity.

Understood.

The idea is to convert all page table entry prints through out the tree
both in generic and platform code. Added just a single generic example
here for this being a RFC proposal. Will go through similar instances
and be back with more comprehensive change set.

> 
>> @@ -2542,6 +2545,23 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>>  		default:
>>  			return error_string(buf, end, "(einval)", spec);
>>  		}
>> +	case 'p':
> 
> Please, do not opencode this in the already very long switch().
> Move it to a helper function.
Sure, will do.

> 
> 
>> +		if (fmt[1] == 't' && fmt[2] == 'e') {
>> +			pte_t *pte = (pte_t *)ptr;
> 
> If the value (pointer) gets dereferenced then please add a basic
> check:

Sure, will do.

> 
> 	if (check_pointer(&buf, end, ptr, spec))
> 		return buf;
> 
>> +			spec.field_width = 10;
>> +			spec.precision = 8;
> 
> Is she precision = 8 really needed?
> I guess that .field_width + ZEROPAD would do the trick.
> 
> And them maybe special_hex_number() might be used instead of number()
> and safe a lot of code.

Agreed. Andy also might have suggested about special_hex_number() helper
on the other thread. Will try and use the helper instead.

> 
>> +			spec.base = 16;
>> +			spec.flags = SPECIAL | SMALL | ZEROPAD;
>> +			if (sizeof(pte_t) == sizeof(u64)) {
>> +				u64 val = pte_val(*pte);
>> +
>> +				return number(buf, end, val, spec);
>> +			}
>> +			WARN_ONCE(1, "Non standard pte_t\n");
> 
> This is nasty. It should be a compile-time check. And the code should

Something like BUILD_BUG_ON() against pte_t as either u64 or u32 aka all
the sizes the print format is going to support and it should pass on all
platforms ?

> get fixed on all architectures. If it is not easy then> it might be a signal that the generic %ppte flag is not a good idea.

Understood.

> 
>> +			return error_string(buf, end, "(einval)", spec);
>> +		}
>> +		fallthrough;
>>  	default:
>>  		return default_pointer(buf, end, ptr, spec);
>>  	}
> 
> Best Regards,
> Petr

Thanks for your review.