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

[Ira Weiny <ira.weiny@intel.com>]

Petr Mladek wrote:
> On Mon 2024-08-26 16:17:52, 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.
> > 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.
> > 3) %par r for range.
> 
> Yes. I got it.
> 
> Well, is struct range really used for addresses?

Commonly yes.  But I agree with Andy that it is not always.

> It rather looks like
> a range of any 64-bit values.
> 
> > %p[rR] is taken.  %pra confuses things IMO.
> 
> Another variants might be %pr64 or %prange.
> 
> IMHO, there is no good solution. We are trying to find the least
> bad one. The meaning should be as obvious and as least confusing
> as possible.

Yep.

> 
> Honestly, I do not have a strong opinion. I kind of like %prange ;-)
> But I could live with all other variants, except for %pn mentioned below.
> 
> > > > 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.
> 
> How exactly does it break the convention, please?
> 
> Do you passing a pointer to struct range instead of a pointer to
> struct resource?

Yes a pointer is passed as the parameter.  This is what %p means AFAIU.
Then the modifier is applied to know what we are pointing to.

> 
> It should not be a big problem as long as the vsprintf() code is
> able to guess the right pointer type from the %pXX modifier.
> 
> > The other idea I had, mentioned in the commit
> > message was %pn.  Meaning passed by pointer 'raNge'.
> 
> This looks like the worst variant to me.

Fair enough.

> 
> > > 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);
> > +
> > +       *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';
> > +
> > +       return string_nocheck(buf, end, sym, spec);
> > +}
> 
> I agree that there is not much duplicated code in the end.
> 
> > 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?  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.
> 
> This would be nasty. I believe that this is not what Andy meant.

Nope.

> 
> Best Regards,
> Petr
> 
> PS: I have vacation until the end of the week, so my next eventual
>     reaction would be delayed.

No hurry.  I'm still mucking around with it,
Ira