If the buffer pointer is NULL we already are in troubles since
regmap bulk API expects caller to provide valid parameters,
it dereferences that without any checks before we call for
traces.
Moreover, the current code will print garbage in the case of
buffer is NULL and length is not 0.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/base/regmap/trace.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/base/regmap/trace.h b/drivers/base/regmap/trace.h
index 04329ba68ec5..e92edc4f4ca5 100644
--- a/drivers/base/regmap/trace.h
+++ b/drivers/base/regmap/trace.h
@@ -82,8 +82,7 @@ DECLARE_EVENT_CLASS(regmap_bulk,
__assign_str(name, regmap_name(map));
__entry->reg = reg;
__entry->val_len = val_len;
- if (val)
- memcpy(__get_dynamic_array(buf), val, val_len);
+ memcpy(__get_dynamic_array(buf), val, val_len);
),
TP_printk("%s reg=%x val=%s", __get_str(name),
--
2.35.1
There is no need to have explicit castings to the same type the
variables are of. Remove the explicit castings.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/base/regmap/trace.h | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/base/regmap/trace.h b/drivers/base/regmap/trace.h
index e92edc4f4ca5..a0f83e44a9d1 100644
--- a/drivers/base/regmap/trace.h
+++ b/drivers/base/regmap/trace.h
@@ -32,9 +32,7 @@ DECLARE_EVENT_CLASS(regmap_reg,
__entry->val = val;
),
- TP_printk("%s reg=%x val=%x", __get_str(name),
- (unsigned int)__entry->reg,
- (unsigned int)__entry->val)
+ TP_printk("%s reg=%x val=%x", __get_str(name), __entry->reg, __entry->val)
);
DEFINE_EVENT(regmap_reg, regmap_reg_write,
@@ -85,8 +83,7 @@ DECLARE_EVENT_CLASS(regmap_bulk,
memcpy(__get_dynamic_array(buf), val, val_len);
),
- TP_printk("%s reg=%x val=%s", __get_str(name),
- (unsigned int)__entry->reg,
+ TP_printk("%s reg=%x val=%s", __get_str(name), __entry->reg,
__print_hex(__get_dynamic_array(buf), __entry->val_len))
);
@@ -124,9 +121,7 @@ DECLARE_EVENT_CLASS(regmap_block,
__entry->count = count;
),
- TP_printk("%s reg=%x count=%d", __get_str(name),
- (unsigned int)__entry->reg,
- (int)__entry->count)
+ TP_printk("%s reg=%x count=%d", __get_str(name), __entry->reg, __entry->count)
);
DEFINE_EVENT(regmap_block, regmap_hw_read_start,
@@ -196,8 +191,7 @@ DECLARE_EVENT_CLASS(regmap_bool,
__entry->flag = flag;
),
- TP_printk("%s flag=%d", __get_str(name),
- (int)__entry->flag)
+ TP_printk("%s flag=%d", __get_str(name), __entry->flag)
);
DEFINE_EVENT(regmap_bool, regmap_cache_only,
@@ -283,8 +277,7 @@ TRACE_EVENT(regcache_drop_region,
__entry->to = to;
),
- TP_printk("%s %u-%u", __get_str(name), (unsigned int)__entry->from,
- (unsigned int)__entry->to)
+ TP_printk("%s %u-%u", __get_str(name), __entry->from, __entry->to)
);
#endif /* _TRACE_REGMAP_H */
--
2.35.1
On Thu, Sep 01, 2022 at 04:23:35PM +0300, Andy Shevchenko wrote:
> There is no need to have explicit castings to the same type the
> variables are of. Remove the explicit castings.
IIRC this was an idiom that the trace code was using for some deep magic
reason to do with some fiddly preprocessor stuff.
On Thu, Sep 1, 2022 at 5:15 PM Mark Brown <[email protected]> wrote:
> On Thu, Sep 01, 2022 at 04:23:35PM +0300, Andy Shevchenko wrote:
> > There is no need to have explicit castings to the same type the
> > variables are of. Remove the explicit castings.
>
> IIRC this was an idiom that the trace code was using for some deep magic
> reason to do with some fiddly preprocessor stuff.
Perhaps that (dark) magic disappeared a long time ago since in my
practice of adding trace events this is the first (and probably
oldest) one which has these castings. Perhaps Steven can shed a light.
--
With Best Regards,
Andy Shevchenko
On Thu, Sep 01, 2022 at 10:46:01AM -0400, Steven Rostedt wrote:
> Andy Shevchenko <[email protected]> wrote:
> > Perhaps that (dark) magic disappeared a long time ago since in my
> > practice of adding trace events this is the first (and probably
> > oldest) one which has these castings. Perhaps Steven can shed a light.
> There was no dark magic for those castings. The trace events never needed
> them. It was added from the original commit:
> fb2736bbaee0e ("regmap: Add basic tracepoints")
Right, they were added in the original commit because as far as I could
tell at that time they were needed for things to build (or possibly
build cleanly, it's been a while).
On Thu, 1 Sep 2022 17:18:01 +0300
Andy Shevchenko <[email protected]> wrote:
> On Thu, Sep 1, 2022 at 5:15 PM Mark Brown <[email protected]> wrote:
> > On Thu, Sep 01, 2022 at 04:23:35PM +0300, Andy Shevchenko wrote:
> > > There is no need to have explicit castings to the same type the
> > > variables are of. Remove the explicit castings.
> >
> > IIRC this was an idiom that the trace code was using for some deep magic
> > reason to do with some fiddly preprocessor stuff.
>
> Perhaps that (dark) magic disappeared a long time ago since in my
> practice of adding trace events this is the first (and probably
> oldest) one which has these castings. Perhaps Steven can shed a light.
>
There was no dark magic for those castings. The trace events never needed
them. It was added from the original commit:
fb2736bbaee0e ("regmap: Add basic tracepoints")
So, I'm all for dropping them.
Reviewed-by: Steven Rostedt (Google) <[email protected]>
-- Steve
On Thu, 1 Sep 2022 15:50:17 +0100
Mark Brown <[email protected]> wrote:
> > fb2736bbaee0e ("regmap: Add basic tracepoints")
>
> Right, they were added in the original commit because as far as I could
> tell at that time they were needed for things to build (or possibly
> build cleanly, it's been a while).
I'm guessing what happened as things were probably a bit confusing back
then, it was something else that didn't build, and you probably tried all
sorts of things and when it worked, you just left it as is ;-)
-- Steve
On Thu, Sep 01, 2022 at 04:23:34PM +0300, Andy Shevchenko wrote:
> If the buffer pointer is NULL we already are in troubles since
> regmap bulk API expects caller to provide valid parameters,
> it dereferences that without any checks before we call for
> traces.
>
> Moreover, the current code will print garbage in the case of
> buffer is NULL and length is not 0.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/base/regmap/trace.h | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/base/regmap/trace.h b/drivers/base/regmap/trace.h
> index 04329ba68ec5..e92edc4f4ca5 100644
> --- a/drivers/base/regmap/trace.h
> +++ b/drivers/base/regmap/trace.h
> @@ -82,8 +82,7 @@ DECLARE_EVENT_CLASS(regmap_bulk,
> __assign_str(name, regmap_name(map));
> __entry->reg = reg;
> __entry->val_len = val_len;
> - if (val)
> - memcpy(__get_dynamic_array(buf), val, val_len);
> + memcpy(__get_dynamic_array(buf), val, val_len);
> ),
>
> TP_printk("%s reg=%x val=%s", __get_str(name),
> --
> 2.35.1
>
Reviewed-by: Dmitry Rokosov <[email protected]>
--
Thank you,
Dmitry
On Thu, Sep 01, 2022 at 04:23:35PM +0300, Andy Shevchenko wrote:
> There is no need to have explicit castings to the same type the
> variables are of. Remove the explicit castings.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/base/regmap/trace.h | 17 +++++------------
> 1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/base/regmap/trace.h b/drivers/base/regmap/trace.h
> index e92edc4f4ca5..a0f83e44a9d1 100644
> --- a/drivers/base/regmap/trace.h
> +++ b/drivers/base/regmap/trace.h
> @@ -32,9 +32,7 @@ DECLARE_EVENT_CLASS(regmap_reg,
> __entry->val = val;
> ),
>
> - TP_printk("%s reg=%x val=%x", __get_str(name),
> - (unsigned int)__entry->reg,
> - (unsigned int)__entry->val)
> + TP_printk("%s reg=%x val=%x", __get_str(name), __entry->reg, __entry->val)
> );
>
> DEFINE_EVENT(regmap_reg, regmap_reg_write,
> @@ -85,8 +83,7 @@ DECLARE_EVENT_CLASS(regmap_bulk,
> memcpy(__get_dynamic_array(buf), val, val_len);
> ),
>
> - TP_printk("%s reg=%x val=%s", __get_str(name),
> - (unsigned int)__entry->reg,
> + TP_printk("%s reg=%x val=%s", __get_str(name), __entry->reg,
> __print_hex(__get_dynamic_array(buf), __entry->val_len))
> );
>
> @@ -124,9 +121,7 @@ DECLARE_EVENT_CLASS(regmap_block,
> __entry->count = count;
> ),
>
> - TP_printk("%s reg=%x count=%d", __get_str(name),
> - (unsigned int)__entry->reg,
> - (int)__entry->count)
> + TP_printk("%s reg=%x count=%d", __get_str(name), __entry->reg, __entry->count)
> );
>
> DEFINE_EVENT(regmap_block, regmap_hw_read_start,
> @@ -196,8 +191,7 @@ DECLARE_EVENT_CLASS(regmap_bool,
> __entry->flag = flag;
> ),
>
> - TP_printk("%s flag=%d", __get_str(name),
> - (int)__entry->flag)
> + TP_printk("%s flag=%d", __get_str(name), __entry->flag)
> );
>
> DEFINE_EVENT(regmap_bool, regmap_cache_only,
> @@ -283,8 +277,7 @@ TRACE_EVENT(regcache_drop_region,
> __entry->to = to;
> ),
>
> - TP_printk("%s %u-%u", __get_str(name), (unsigned int)__entry->from,
> - (unsigned int)__entry->to)
> + TP_printk("%s %u-%u", __get_str(name), __entry->from, __entry->to)
> );
>
> #endif /* _TRACE_REGMAP_H */
> --
> 2.35.1
>
Reviewed-by: Dmitry Rokosov <[email protected]>
--
Thank you,
Dmitry
On Thu, 1 Sep 2022 16:23:34 +0300, Andy Shevchenko wrote:
> If the buffer pointer is NULL we already are in troubles since
> regmap bulk API expects caller to provide valid parameters,
> it dereferences that without any checks before we call for
> traces.
>
> Moreover, the current code will print garbage in the case of
> buffer is NULL and length is not 0.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next
Thanks!
[1/3] regmap: trace: Remove useless check for NULL for bulk ops
commit: f78d5e1168e08429bc948d09b6dc11fec5e019f7
[2/3] regmap: trace: Remove explicit castings
commit: d10268a50bdbc03ebb6d340d63bf78c44d7c66a8
[3/3] regmap: trace: Remove unneeded blank lines
commit: 6ed406ef9f74372282c3b515e64986120823f769
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