Re: [PATCH v4 02/28] printk: Add print format (%pra) for struct range

[Andy Shevchenko]

On Mon, Oct 07, 2024 at 06:16:08PM -0500, Ira Weiny wrote:
> The use of struct range in the CXL subsystem is growing.  In particular,
> the addition of Dynamic Capacity devices uses struct range in a number
> of places which are reported in debug and error messages.
> 
> To wit requiring the printing of the start/end fields in each print
> became cumbersome.  Dan Williams mentions in [1] that it might be time
> to have a print specifier for struct range similar to struct resource
> 
> A few alternatives were considered including '%par', '%r', and '%pn'.
> %pra follows that struct range is similar to struct resource (%p[rR])
> but need to be different.  Based on discussions with Petr and Andy
> '%pra' was chosen.[2]
> 
> Andy also suggested to keep the range prints similar to struct resource
> though combined code.  Add hex_range() to handle printing for both
> pointer types.

...

> +static void __init
> +struct_range(void)
> +{
> +	struct range test_range = {
> +		.start = 0xc0ffee00ba5eba11,
> +		.end = 0xc0ffee00ba5eba11,
> +	};

A side note, can we add something like

#define DEFINE_RANGE(start, end)	\
	(struct range) {		\
		.start = (start),	\
		.end = (end),		\
	}

in range.h and use here and in the similar cases?

> +	test("[range 0xc0ffee00ba5eba11]", "%pra", &test_range);
> +
> +	test_range = (struct range) {
> +		.start = 0xc0ffee,
> +		.end = 0xba5eba11,
> +	};
> +	test("[range 0x0000000000c0ffee-0x00000000ba5eba11]",
> +	     "%pra", &test_range);
> +
> +	test_range = (struct range) {
> +		.start = 0xba5eba11,
> +		.end = 0xc0ffee,
> +	};
> +	test("[range 0x00000000ba5eba11-0x0000000000c0ffee]",
> +	     "%pra", &test_range);
> +}

...


> +char *hex_range(char *buf, char *end, u64 start_val, u64 end_val,
> +		struct printf_spec spec)
> +{
> +	buf = number(buf, end, start_val, spec);
> +	if (start_val != end_val) {
> +		if (buf < end)
> +			*buf++ = '-';
> +		buf = number(buf, end, end_val, spec);
> +	}
> +	return buf;
> +}

Perhaps

	buf = number(buf, end, start_val, spec);
	if (start_val == end_val)
		return buf;

	if (buf < end)
		*buf++ = '-';
	return number(buf, end, end_val, spec);

(yes, I have seen the original code)?


> +static noinline_for_stack
> +char *range_string(char *buf, char *end, const struct range *range,
> +		   struct printf_spec spec, const char *fmt)
> +{
> +#define RANGE_DECODED_BUF_SIZE		((2 * sizeof(struct range)) + 4)
> +#define RANGE_PRINT_BUF_SIZE		sizeof("[range -]")
> +	char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE];
> +	char *p = sym, *pend = sym + sizeof(sym);
> +
> +	struct printf_spec range_spec = {
> +		.field_width = 2 + 2 * sizeof(range->start), /* 0x + 2 * 8 */
> +		.flags = SPECIAL | SMALL | ZEROPAD,
> +		.base = 16,
> +		.precision = -1,
> +	};
> +
> +	if (check_pointer(&buf, end, range, spec))
> +		return buf;
> +
> +	*p++ = '[';
> +	p = string_nocheck(p, pend, "range ", default_str_spec);
> +	p = hex_range(p, pend, range->start, range->end, range_spec);
> +	*p++ = ']';
> +	*p = '\0';
> +
> +	return string_nocheck(buf, end, sym, spec);
> +}

...

> + * - 'ra' struct ranges [range 0x00 - 0xff]

Is it possible to get only bytes out of this? I thought we have always
64-bit values here, no?

...

>  	case 'B':
>  		return symbol_string(buf, end, ptr, spec, fmt);
> -	case 'R':
>  	case 'r':
> +		switch (fmt[1]) {
> +		case 'a':
> +			return range_string(buf, end, ptr, spec, fmt);
> +		}
> +		fallthrough;
> +	case 'R':
>  		return resource_string(buf, end, ptr, spec, fmt);

Do we have default-less switches in the code (in this file)?

Actually I would suggest to move this to a wrapper like time_and_date().

-- 
With Best Regards,
Andy Shevchenko