Re: [PATCH v3 02/25] printk: Add print format (%par) for struct range

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Aug 26, 2024 at 04:17:52PM -0500, Ira Weiny wrote:
> Andy Shevchenko wrote:
> > On Mon, Aug 26, 2024 at 03:23:50PM +0200, Petr Mladek wrote:
> > > On Thu 2024-08-22 21:10:25, Andy Shevchenko wrote:
> > > > On Thu, Aug 22, 2024 at 12:53:32PM -0500, Ira Weiny wrote:
> > > > > Petr Mladek wrote:
> > > > > > On Fri 2024-08-16 09:44:10, Ira Weiny wrote:

...

> > > > > > > +	%par	[range 0x60000000-0x6fffffff] or
> > > > > > 
> > > > > > It seems that it is always 64-bit. It prints:
> > > > > > 
> > > > > > struct range {
> > > > > > 	u64   start;
> > > > > > 	u64   end;
> > > > > > };
> > > > > 
> > > > > Indeed.  Thanks I should not have just copied/pasted.
> > > > 
> > > > With that said, I'm not sure the %pa is a good placeholder for this ('a' stands
> > > > to "address" AFAIU). Perhaps this should go somewhere under %pr/%pR?
> 
> I'm speaking a bit for Dan here but also the logical way I thought of
> things.
> 
> 1) %p does not dictate anything about the format of the data.  Rather
>    indicates that what is passed is a pointer.  Because we are passing a
>    pointer to a range struct %pXX makes sense.

There is no objection to that.

> 2) %pa indicates what follows is 'address'.  This was a bit of creative
>    license because, as I said in the commit message most of the time
>    struct range contains an address range.  So for this narrow use case it
>    also makes sense.

As in the discussion it was pointed out that struct range is always 64-bit,
limiting it to the "address" is a wrong assumption as we are talking generic
printing routine here. We don't know what users will be in the future on 32-bit
platforms, or what data (semantically) is being held by this structure.

> 3) %par r for range.

I understand, but again struct range != address.

> %p[rR] is taken.
> %pra confuses things IMO.

It doesn't confuse me. :-) But I believe Petr also has a rationale behind this
proposal as he described earlier.

> > > The r/R in %pr/%pR actually stands for "resource".
> > > 
> > > But "%ra" really looks like a better choice than "%par". Both
> > > "resource"  and "range" starts with 'r'. Also the struct resource
> > > is printed as a range of values.
> 
> %r could be used I think.  But this breaks with the convention of passing a
> pointer and how to interpret it.  The other idea I had, mentioned in the commit
> message was %pn.  Meaning passed by pointer 'raNge'.

No, we can't use %r or anything else that is documented for the standard
printf() format specifiers, otherwise you will get a compiler warning and
basically it means no go.

> I think that follows better than %r.  That would be another break from C99.
> But we don't have to follow that.
> 
> > Fine with me as long as it:
> > 1) doesn't collide with %pa namespace
> > 2) tries to deduplicate existing code as much as possible.
> 
> Andy, I'm not quite following how you expect to share the code between
> resource_string() and range_string()?
> 
> There is very little duplicated code.  In fact with Petr's suggestions and some
> more work range_string() is quite simple:
> 
> +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);


Missing check for pointer, but it's not that I wanted to tell.

> +       *p++ = '[';
> +       p = string_nocheck(p, pend, "range ", default_str_spec);

Hmm... %pr uses str_spec, what the difference can be here?

> +       p = special_hex_number(p, pend, range->start, sizeof(range->start));
> +       *p++ = '-';
> +       p = special_hex_number(p, pend, range->end, sizeof(range->end));

This is basically the copy of %pr implementation.

	p = number(p, pend, res->start, *specp);
	if (res->start != res->end) {
		*p++ = '-';
		p = number(p, pend, res->end, *specp);
	}

Would it be possible to unify? I think so, but it requires a bit of thinking.

That's why testing is very important in this kind of generic code.

> +       *p++ = ']';
> +       *p = '\0';
> +
> +       return string_nocheck(buf, end, sym, spec);
> +}
> 
> Also this is the bulk of the patch except for documentation and the new
> testing code.  [new patch below]
> 
> Am I missing your point somehow?

See above.

> I considered cramming a struct range into a
> struct resource to let resource_string() process the data.  But that would
> involve creating a new IORESOURCE_* flag (not ideal) and also does not allow
> for the larger u64 data in struct range should this be a 32 bit physical
> address config.

No, that's not what I was expecting.

> Most importantly that would not be much less code AFAICT.

...

> +       %par    [range 0x0000000060000000-0x000000006fffffff]

I still think this is not okay to use %pa namespace.

...

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

Case when start == end?
Case when end < start?

> +}

...

> +       *p++ = '[';
> +       p = string_nocheck(p, pend, "range ", default_str_spec);
> +       p = special_hex_number(p, pend, range->start, sizeof(range->start));
> +       *p++ = '-';
> +       p = special_hex_number(p, pend, range->end, sizeof(range->end));
> +       *p++ = ']';
> +       *p = '\0';

As per above comments.

-- 
With Best Regards,
Andy Shevchenko