Currently, only one-register io operations support tracepoints with
value logging. For the regmap bulk operations developer can view
hw_start/hw_done tracepoints with starting reg number and registers
count to be reading or writing. This patch injects tracepoints with
dumping registers values in the hex format to regmap bulk reading
and writing.
Signed-off-by: Dmitry Rokosov <[email protected]>
---
drivers/base/regmap/regmap.c | 7 ++++++
drivers/base/regmap/trace.h | 43 ++++++++++++++++++++++++++++++++++++
2 files changed, 50 insertions(+)
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index c3517ccc3159..673ad37df11f 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -2323,6 +2323,10 @@ int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val,
kfree(wval);
}
+
+ if (!ret)
+ trace_regmap_bulk_write(map, reg, val, val_bytes * val_count);
+
return ret;
}
EXPORT_SYMBOL_GPL(regmap_bulk_write);
@@ -3068,6 +3072,9 @@ int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
map->unlock(map->lock_arg);
}
+ if (!ret)
+ trace_regmap_bulk_read(map, reg, val, val_bytes * val_count);
+
return ret;
}
EXPORT_SYMBOL_GPL(regmap_bulk_read);
diff --git a/drivers/base/regmap/trace.h b/drivers/base/regmap/trace.h
index 9abee14df9ee..04329ba68ec5 100644
--- a/drivers/base/regmap/trace.h
+++ b/drivers/base/regmap/trace.h
@@ -64,6 +64,49 @@ DEFINE_EVENT(regmap_reg, regmap_reg_read_cache,
);
+DECLARE_EVENT_CLASS(regmap_bulk,
+
+ TP_PROTO(struct regmap *map, unsigned int reg,
+ const void *val, int val_len),
+
+ TP_ARGS(map, reg, val, val_len),
+
+ TP_STRUCT__entry(
+ __string(name, regmap_name(map))
+ __field(unsigned int, reg)
+ __dynamic_array(char, buf, val_len)
+ __field(int, val_len)
+ ),
+
+ TP_fast_assign(
+ __assign_str(name, regmap_name(map));
+ __entry->reg = reg;
+ __entry->val_len = val_len;
+ if (val)
+ memcpy(__get_dynamic_array(buf), val, val_len);
+ ),
+
+ TP_printk("%s reg=%x val=%s", __get_str(name),
+ (unsigned int)__entry->reg,
+ __print_hex(__get_dynamic_array(buf), __entry->val_len))
+);
+
+DEFINE_EVENT(regmap_bulk, regmap_bulk_write,
+
+ TP_PROTO(struct regmap *map, unsigned int reg,
+ const void *val, int val_len),
+
+ TP_ARGS(map, reg, val, val_len)
+);
+
+DEFINE_EVENT(regmap_bulk, regmap_bulk_read,
+
+ TP_PROTO(struct regmap *map, unsigned int reg,
+ const void *val, int val_len),
+
+ TP_ARGS(map, reg, val, val_len)
+);
+
DECLARE_EVENT_CLASS(regmap_block,
TP_PROTO(struct regmap *map, unsigned int reg, int count),
--
2.36.0
+Thomas, Rasmus and Marc
On Tue, Aug 16, 2022 at 06:14:48PM +0000, Dmitry Rokosov wrote:
> Currently, only one-register io operations support tracepoints with
> value logging. For the regmap bulk operations developer can view
> hw_start/hw_done tracepoints with starting reg number and registers
> count to be reading or writing. This patch injects tracepoints with
> dumping registers values in the hex format to regmap bulk reading
> and writing.
>
> Signed-off-by: Dmitry Rokosov <[email protected]>
> ---
> drivers/base/regmap/regmap.c | 7 ++++++
> drivers/base/regmap/trace.h | 43 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 50 insertions(+)
>
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index c3517ccc3159..673ad37df11f 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -2323,6 +2323,10 @@ int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val,
>
> kfree(wval);
> }
> +
> + if (!ret)
> + trace_regmap_bulk_write(map, reg, val, val_bytes * val_count);
> +
> return ret;
> }
> EXPORT_SYMBOL_GPL(regmap_bulk_write);
> @@ -3068,6 +3072,9 @@ int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
> map->unlock(map->lock_arg);
> }
>
> + if (!ret)
> + trace_regmap_bulk_read(map, reg, val, val_bytes * val_count);
> +
> return ret;
> }
> EXPORT_SYMBOL_GPL(regmap_bulk_read);
> diff --git a/drivers/base/regmap/trace.h b/drivers/base/regmap/trace.h
> index 9abee14df9ee..04329ba68ec5 100644
> --- a/drivers/base/regmap/trace.h
> +++ b/drivers/base/regmap/trace.h
> @@ -64,6 +64,49 @@ DEFINE_EVENT(regmap_reg, regmap_reg_read_cache,
>
> );
>
> +DECLARE_EVENT_CLASS(regmap_bulk,
> +
> + TP_PROTO(struct regmap *map, unsigned int reg,
> + const void *val, int val_len),
> +
> + TP_ARGS(map, reg, val, val_len),
> +
> + TP_STRUCT__entry(
> + __string(name, regmap_name(map))
> + __field(unsigned int, reg)
> + __dynamic_array(char, buf, val_len)
> + __field(int, val_len)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(name, regmap_name(map));
> + __entry->reg = reg;
> + __entry->val_len = val_len;
> + if (val)
> + memcpy(__get_dynamic_array(buf), val, val_len);
> + ),
> +
> + TP_printk("%s reg=%x val=%s", __get_str(name),
> + (unsigned int)__entry->reg,
> + __print_hex(__get_dynamic_array(buf), __entry->val_len))
> +);
> +
> +DEFINE_EVENT(regmap_bulk, regmap_bulk_write,
> +
> + TP_PROTO(struct regmap *map, unsigned int reg,
> + const void *val, int val_len),
> +
> + TP_ARGS(map, reg, val, val_len)
> +);
> +
> +DEFINE_EVENT(regmap_bulk, regmap_bulk_read,
> +
> + TP_PROTO(struct regmap *map, unsigned int reg,
> + const void *val, int val_len),
> +
> + TP_ARGS(map, reg, val, val_len)
> +);
> +
> DECLARE_EVENT_CLASS(regmap_block,
>
> TP_PROTO(struct regmap *map, unsigned int reg, int count),
> --
> 2.36.0
--
Thank you,
Dmitry
On Thu, 18 Aug 2022 13:15:00 +0100,
Dmitry Rokosov <[email protected]> wrote:
>
> +Thomas, Rasmus and Marc
>
> On Tue, Aug 16, 2022 at 06:14:48PM +0000, Dmitry Rokosov wrote:
> > Currently, only one-register io operations support tracepoints with
> > value logging. For the regmap bulk operations developer can view
> > hw_start/hw_done tracepoints with starting reg number and registers
> > count to be reading or writing. This patch injects tracepoints with
> > dumping registers values in the hex format to regmap bulk reading
> > and writing.
I don't care much about regmap as a MMIO backend, but it strikes me as
odd that you end up with multiple ways of logging the same stuff (with
a memcpy in the middle of it).
Why can't this be done with a small amount of trace post-processing?
M.
--
Without deviation from the norm, progress is not possible.
On Thu, Aug 18, 2022 at 02:49:20PM +0100, Marc Zyngier wrote:
> I don't care much about regmap as a MMIO backend, but it strikes me as
> odd that you end up with multiple ways of logging the same stuff (with
> a memcpy in the middle of it).
> Why can't this be done with a small amount of trace post-processing?
At the minute we don't put the actual data for the bulk transfers into
the trace so the information simply isn't there.
Hello Marc,
Thank you for quick feedback. Please find my comments below.
On Thu, Aug 18, 2022 at 02:49:20PM +0100, Marc Zyngier wrote:
[...]
> > > Currently, only one-register io operations support tracepoints with
> > > value logging. For the regmap bulk operations developer can view
> > > hw_start/hw_done tracepoints with starting reg number and registers
> > > count to be reading or writing. This patch injects tracepoints with
> > > dumping registers values in the hex format to regmap bulk reading
> > > and writing.
>
> I don't care much about regmap as a MMIO backend, but it strikes me as
> odd that you end up with multiple ways of logging the same stuff (with
> a memcpy in the middle of it).
>
> Why can't this be done with a small amount of trace post-processing?
Sorry, actually, I don't get you. What do you mean by "same stuff"?
For now, regmap bulk I/O operations don't log data buffers, because
current regmap trace classes don't have a dynamic trace arrays inside.
We should use dynamic array here because bulk I/O operations can vary
buffer size from call to call.
Function memcpy() is used to copy original buffer data to a trace array
when tracepoint is enabled. In other words, per my understanding,
when tracepoint is disabled we do not call TP_fast_assign instructions.
Trace event documentation says about dynamic array:
* __dynamic_array: This is similar to array, but can vary its size from
* instance to instance of the tracepoint being called.
* Like __array, this too has three elements (type, name, size);
* type is the type of the element, name is the name of the array.
* The size is different than __array. It is not a static number,
* but the algorithm to figure out the length of the array for the
* specific instance of tracepoint. Again, size is the number of
* items in the array, not the total length in bytes.
*
* __dynamic_array( int, foo, bar) is similar to: int foo[bar];
*
* Note, unlike arrays, you must use the __get_dynamic_array() macro
* to access the array.
*
* memcpy(__get_dynamic_array(foo), bar, 10);
*
* Notice, that "__entry" is not needed here.
BTW, I've tried to use the already existing TRACE CLASS regmap_block, but
it's difficult to integrate dynamic array to that, because sometimes we
do not have a reg data (for example, regmap_hw_read_start event).
[...]
--
Thank you,
Dmitry
Hello Mark,
On Thu, Aug 18, 2022 at 04:43:45PM +0100, Mark Brown wrote:
> On Thu, Aug 18, 2022 at 02:49:20PM +0100, Marc Zyngier wrote:
>
> > I don't care much about regmap as a MMIO backend, but it strikes me as
> > odd that you end up with multiple ways of logging the same stuff (with
> > a memcpy in the middle of it).
>
> > Why can't this be done with a small amount of trace post-processing?
>
> At the minute we don't put the actual data for the bulk transfers into
> the trace so the information simply isn't there.
What do you think about the patch? Can we use the separate trace event
class, or do we have to add these tracepoints to some existing class, like
regmap_block?
Appreciate any thoughts and feedback.
--
Thank you,
Dmitry
On Thu, Aug 18, 2022 at 05:44:23PM +0000, Dmitry Rokosov wrote:
> On Thu, Aug 18, 2022 at 04:43:45PM +0100, Mark Brown wrote:
> > At the minute we don't put the actual data for the bulk transfers into
> > the trace so the information simply isn't there.
> What do you think about the patch? Can we use the separate trace event
> class, or do we have to add these tracepoints to some existing class, like
> regmap_block?
I didn't realise that was even a question, but then there seems to be
some discussion I've not seen given the CCing going on. The biggest
issue is do we even want the overhead but I'll need to find time to look
at this properly.
On Fri, Aug 19, 2022 at 01:07:47PM +0100, Mark Brown wrote:
> On Thu, Aug 18, 2022 at 05:44:23PM +0000, Dmitry Rokosov wrote:
> > On Thu, Aug 18, 2022 at 04:43:45PM +0100, Mark Brown wrote:
>
> > > At the minute we don't put the actual data for the bulk transfers into
> > > the trace so the information simply isn't there.
>
> > What do you think about the patch? Can we use the separate trace event
> > class, or do we have to add these tracepoints to some existing class, like
> > regmap_block?
>
> I didn't realise that was even a question, but then there seems to be
> some discussion I've not seen given the CCing going on. The biggest
> issue is do we even want the overhead but I'll need to find time to look
> at this properly.
No any additional discussion before. I've added your address to all emails
which I sent.
I've asked about the bulk tracepoints patch. As I understood Marc's question
about multiple ways of logging the same stuff, the main concern is patch
adding additional trace event class "regmap_block_io" and doesn't use already
existing classes. I've tried to inject bulk transfer data hexdumping to
regmap_block trace event class, but it has some difficult and ugly
conditions should be applied. That's why I would prefer to discuss
implementation proposed by patch if possible.
Why do you think it this patch will bring overhead to regmap? AFAK, when
tracepoint is disabled, tracepoint fast assign operation shouldn't be
executed. In other words, memcpy will not be applied.
--
Thank you,
Dmitry
On 2022-08-18 16:43, Mark Brown wrote:
> On Thu, Aug 18, 2022 at 02:49:20PM +0100, Marc Zyngier wrote:
>
>> I don't care much about regmap as a MMIO backend, but it strikes me as
>> odd that you end up with multiple ways of logging the same stuff (with
>> a memcpy in the middle of it).
>
>> Why can't this be done with a small amount of trace post-processing?
>
> At the minute we don't put the actual data for the bulk transfers into
> the trace so the information simply isn't there.
But isn't that what this patch should do?
We also have recently merged the CONFIG_TRACE_MMIO_ACCESS which
already dumps all sort of MMIO crap^Winformation.
Surely there should be a more common approach to this.
M.
--
Jazz is not dead. It just smells funny...
On Fri, Aug 19, 2022 at 03:25:44PM +0100, Marc Zyngier wrote:
> On 2022-08-18 16:43, Mark Brown wrote:
> > On Thu, Aug 18, 2022 at 02:49:20PM +0100, Marc Zyngier wrote:
> > > I don't care much about regmap as a MMIO backend, but it strikes me as
> > > odd that you end up with multiple ways of logging the same stuff (with
> > > a memcpy in the middle of it).
> > > Why can't this be done with a small amount of trace post-processing?
> > At the minute we don't put the actual data for the bulk transfers into
> > the trace so the information simply isn't there.
> But isn't that what this patch should do?
I'd imagine so based on a quick glance at the description, I've not
actually reviewed it yet, but in that case I'm not sure what your
concern is here?
> We also have recently merged the CONFIG_TRACE_MMIO_ACCESS which
> already dumps all sort of MMIO crap^Winformation.
Yes, that'd also cover it for MMIO based regmaps when enabled but
obviously other buses exist and can also be accessed via regmap.
> Surely there should be a more common approach to this.
There's an argument for tracing at each abstraction layer since they're
generally all doing *something*, people will look to the layer they're
accessing and for things like tracing register accesses with buses like
I2C and SPI regmap is adding the register semantics on top of a bus
that's just a byte stream. Even on buses with a native concept of an
address there's stuff like paging which might be added on depending on
the device. They should probably all follow a similar pattern but I'm
not sure we can do everything at once.
On Fri, Aug 19, 2022 at 01:31:35PM +0000, Dmitry Rokosov wrote:
> On Fri, Aug 19, 2022 at 01:07:47PM +0100, Mark Brown wrote:
> > I didn't realise that was even a question, but then there seems to be
> > some discussion I've not seen given the CCing going on. The biggest
> > issue is do we even want the overhead but I'll need to find time to look
> > at this properly.
> No any additional discussion before. I've added your address to all emails
> which I sent.
I assume you copied in Thomas, Rasmus and Marc for some reason?
> Why do you think it this patch will bring overhead to regmap? AFAK, when
> tracepoint is disabled, tracepoint fast assign operation shouldn't be
> executed. In other words, memcpy will not be applied.
To repeat I have not yet looked at your patch properly, one concern is
how we handle the marshalling which regmap does.
On Fri, Aug 19, 2022 at 03:44:19PM +0100, Mark Brown wrote:
> > > I didn't realise that was even a question, but then there seems to be
> > > some discussion I've not seen given the CCing going on. The biggest
> > > issue is do we even want the overhead but I'll need to find time to look
> > > at this properly.
>
> > No any additional discussion before. I've added your address to all emails
> > which I sent.
>
> I assume you copied in Thomas, Rasmus and Marc for some reason?
No reason, sorry for make misunderstanding here.
I have listed other regmap discussions and added them to get more opinions.
>
> > Why do you think it this patch will bring overhead to regmap? AFAK, when
> > tracepoint is disabled, tracepoint fast assign operation shouldn't be
> > executed. In other words, memcpy will not be applied.
>
> To repeat I have not yet looked at your patch properly, one concern is
> how we handle the marshalling which regmap does.
Okay, no problem
--
Thank you,
Dmitry
On Fri, Aug 19, 2022 at 03:38:27PM +0100, Mark Brown wrote:
> On Fri, Aug 19, 2022 at 03:25:44PM +0100, Marc Zyngier wrote:
> > On 2022-08-18 16:43, Mark Brown wrote:
> > > On Thu, Aug 18, 2022 at 02:49:20PM +0100, Marc Zyngier wrote:
>
> > > > I don't care much about regmap as a MMIO backend, but it strikes me as
> > > > odd that you end up with multiple ways of logging the same stuff (with
> > > > a memcpy in the middle of it).
>
> > > > Why can't this be done with a small amount of trace post-processing?
>
> > > At the minute we don't put the actual data for the bulk transfers into
> > > the trace so the information simply isn't there.
>
> > But isn't that what this patch should do?
>
> I'd imagine so based on a quick glance at the description, I've not
> actually reviewed it yet, but in that case I'm not sure what your
> concern is here?
>
> > We also have recently merged the CONFIG_TRACE_MMIO_ACCESS which
> > already dumps all sort of MMIO crap^Winformation.
>
> Yes, that'd also cover it for MMIO based regmaps when enabled but
> obviously other buses exist and can also be accessed via regmap.
>
> > Surely there should be a more common approach to this.
>
> There's an argument for tracing at each abstraction layer since they're
> generally all doing *something*, people will look to the layer they're
> accessing and for things like tracing register accesses with buses like
> I2C and SPI regmap is adding the register semantics on top of a bus
> that's just a byte stream. Even on buses with a native concept of an
> address there's stuff like paging which might be added on depending on
> the device. They should probably all follow a similar pattern but I'm
> not sure we can do everything at once.
Oh, I see what you mean. Actually, I tested my patch only on I2C-regmap
abstraction only. I thought on the regmap level, it doesn't matter how
byte stream will be interleaved on the bus layer. On the other hand, from
a developer's perspective, we want to see a real bus stream trace with
already rearranged data chunks...
--
Thank you,
Dmitry
On Tue, 16 Aug 2022 18:14:48 +0000, Dmitry Rokosov wrote:
> Currently, only one-register io operations support tracepoints with
> value logging. For the regmap bulk operations developer can view
> hw_start/hw_done tracepoints with starting reg number and registers
> count to be reading or writing. This patch injects tracepoints with
> dumping registers values in the hex format to regmap bulk reading
> and writing.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next
Thanks!
[1/1] regmap: introduce value tracing for regmap bulk operations
commit: 026c99b508f060d3c85fda06b21e010683ef5590
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
On Tue, Aug 16, 2022 at 9:14 PM Dmitry Rokosov <[email protected]> wrote:
>
> Currently, only one-register io operations support tracepoints with
> value logging. For the regmap bulk operations developer can view
> hw_start/hw_done tracepoints with starting reg number and registers
> count to be reading or writing. This patch injects tracepoints with
> dumping registers values in the hex format to regmap bulk reading
> and writing.
Since it's applied, below might be considered for follow-ups.
...
> + if (val)
> + memcpy(__get_dynamic_array(buf), val, val_len);
I'm probably missing something, but what this condition prevents from?
...
> + TP_printk("%s reg=%x val=%s", __get_str(name),
> + (unsigned int)__entry->reg,
Why do you need casting?
--
With Best Regards,
Andy Shevchenko
Hello Andy,
Sorry for late response. I didn't have the ability to reach my laptop during
last week.
[...]
>
> > + if (val)
> > + memcpy(__get_dynamic_array(buf), val, val_len);
>
> I'm probably missing something, but what this condition prevents from?
In general, this condition prevents memcpy from being executed when
tracepoint is called with a null pointed buffer.
[...]
> > + TP_printk("%s reg=%x val=%s", __get_str(name),
> > + (unsigned int)__entry->reg,
>
> Why do you need casting?
>
To be honest, I've made it based on the already existing regmap
tracepoints style. All of them make a cast to unsigned int type when
printout reg number.
--
Thank you,
Dmitry
On Wed, Aug 31, 2022 at 03:45:30AM +0300, Dmitry Rokosov wrote:
...
> > > + if (val)
> > > + memcpy(__get_dynamic_array(buf), val, val_len);
> >
> > I'm probably missing something, but what this condition prevents from?
>
> In general, this condition prevents memcpy from being executed when
> tracepoint is called with a null pointed buffer.
If we got a NULL pointer here, we already in a lot of troubles.
I believe the check is not needed.
Otherwise the function prints garbage.
I will send a series to remove that and clean up the file.
--
With Best Regards,
Andy Shevchenko