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
