Re: [PATCH v2 1/3] hw/block/pflash: Remove dynamic field width from trace events

[Philippe Mathieu-Daudé]

On 11/18/19 8:21 PM, Eric Blake wrote:
> On 11/14/19 3:26 PM, Philippe Mathieu-Daudé wrote:
>
> > > > -    trace_pflash_data_read(offset, width << 1, ret);
> > > > +    trace_pflash_data_read(offset, width << 3, ret);
> > >
> > > Umm, why is width changing?  That's not mentioned in the commit message.
> >
> > Previously it was used to set the format width: [1, 2, 4] -> [2, 4, 8].
> >
> > We usually log the width in byte (accessed at memory location) or bits 
> > (used by the bus). When using this device I'm custom to think in bus 
> > access width.
> >
> > Regardless whichever format we prefer, a change is needed.
>
> > Do you prefer using a "-bit" suffix? As
> >
> > "offset:0x%04"PRIx64" width:%d-bit value:0x%04x cmd:0x%02x wcycle:%u"
> >
> > I can also simply remove this information. Ideally I'd revert this 
> > patch once the we get this format parsable by the SystemTap backend.
>
> Reporting either 'width:8-bit'/'width:16-bit' (explicit bits) or 
> 'width:1'/'width:2' (implying byte) is fine by me.  Showing a bus width 
> in bytes adequately explains why you are using <<3 (aka converting bits 
> to bytes), and how it compares to the previous <<1 (converting bits to 
> number of hex characters).  But whichever you pick (tracing bit width 
> vs. byte width, and how it differs from previous usage of width as 
> output-character count), documenting it in the commit message will make 
> life easier to understand the change.

Yes you are right, I should have documented to avoid wasting review time 
clearing the confusion.