Ira Weiny <ira.weiny@xxxxxxxxx> writes:
> ---
> Documentation/core-api/printk-formats.rst | 13 ++++++++
> lib/test_printf.c | 26 +++++++++++++++
> lib/vsprintf.c | 55 +++++++++++++++++++++++++++----
> 3 files changed, 88 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index 14e093da3ccd..03b102fc60bb 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -231,6 +231,19 @@ width of the CPU data path.
>
> Passed by reference.
>
> +Struct Range
> +------------
Probably neither of those words should be capitalized.
> +
> +::
> +
> + %pra [range 0x0000000060000000-0x000000006fffffff]
> + %pra [range 0x0000000060000000]
> +
> +For printing struct range. struct range holds an arbitrary range of u64
> +values. If start is equal to end only 1 value is printed.
> +
> +Passed by reference.
> +
> DMA address types dma_addr_t
> ----------------------------
>
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 5afdf5efc627..e3e75b6d10a0 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -432,6 +432,31 @@ struct_resource(void)
> "%pR", &test_resource);
> }
>
> +static void __init
> +struct_range(void)
> +{
> + struct range test_range = {
> + .start = 0xc0ffee00ba5eba11,
> + .end = 0xc0ffee00ba5eba11,
> + };
> +
> + 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);
> +}
> +
Thanks for including tests!
Rather than the struct assignments, I think it's easier to read if you
just do
struct range r;
r.start = 0xc0ffee00ba5eba11;
r.end = r.start;
...
r.start = 0xc0ffee;
r.end = 0xba5eba11;
...
which saves two lines per test and for the first one makes it more
obvious that the start and end values are identical.
> static void __init
> addr(void)
> {
> @@ -807,6 +832,7 @@ test_pointer(void)
> symbol_ptr();
> kernel_ptr();
> struct_resource();
> + struct_range();
> addr();
> escaped_str();
> hex_string();
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 09f022ba1c05..f8f5ed8f4d39 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1039,6 +1039,19 @@ static const struct printf_spec default_dec04_spec = {
> .flags = ZEROPAD,
> };
>
> +static noinline_for_stack
> +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++ = '-';
No. Either all your callers pass a (probably stack-allocated) buffer
which is guaranteed to be big enough, in which case you don't need the
"if (buf < end)", or if some callers may "print" directly to the buffer
passed to vsnprintf(), the buf++ must still be done unconditionally in
order that vsnprintf(NULL, 0, ...) [used by fx kasprintf] can accurately
determine how large the output string would be.
So, either
*buf++ = '-'
or
if (buf < end)
*buf = '-';
buf++;
Please don't mix the two.
> + buf = number(buf, end, end_val, spec);
> + }
> + return buf;
> +}
> +
> static noinline_for_stack
> char *resource_string(char *buf, char *end, struct resource *res,
> struct printf_spec spec, const char *fmt)
> @@ -1115,11 +1128,7 @@ char *resource_string(char *buf, char *end, struct resource *res,
> p = string_nocheck(p, pend, "size ", str_spec);
> p = number(p, pend, resource_size(res), *specp);
> } else {
> - p = number(p, pend, res->start, *specp);
> - if (res->start != res->end) {
> - *p++ = '-';
> - p = number(p, pend, res->end, *specp);
> - }
> + p = hex_range(p, pend, res->start, res->end, *specp);
> }
> if (decode) {
> if (res->flags & IORESOURCE_MEM_64)
> @@ -1140,6 +1149,34 @@ char *resource_string(char *buf, char *end, struct resource *res,
> return string_nocheck(buf, end, sym, spec);
> }
>
> +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];
I don't think these names or the split in two constants helps
convincing that's the right amount. I have to think quite a bit to see
that 2*sizeof is because struct range has two u64 and we're printing in
hex so four-bits-per-char and probably the +4 are for two time "0x".
Why not just size the buffer directly using an "example" string?
char sym[sizeof("[range 0x0123456789abcdef-0x0123456789abcdef]")]
> + 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);
We really should have mempcpy or stpcpy. I don't see the point of using
string_nocheck here, or not including the [ in the string copy (however
it's done). But yeah, without stpcpy() that's a bit awkward.
Rasmus
