[PATCH v1 1/3] regmap: trace: Remove useless check for NULL for bulk ops

[PATCH v1 1/3] regmap: trace: Remove useless check for NULL for bulk ops

[Andy Shevchenko]

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 <andriy.shevchenko@linux.intel.com>
---
 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

[PATCH v1 2/3] regmap: trace: Remove explicit castings

[Andy Shevchenko]

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 <andriy.shevchenko@linux.intel.com>
---
 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

[PATCH v1 3/3] regmap: trace: Remove unneeded blank lines

[Andy Shevchenko]

There is a few unneeded blank lines in some of event definitions,
remove them in order to make those definitions consistent with
the rest.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/regmap/trace.h | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/base/regmap/trace.h b/drivers/base/regmap/trace.h
index a0f83e44a9d1..704e106e5dbd 100644
--- a/drivers/base/regmap/trace.h
+++ b/drivers/base/regmap/trace.h
@@ -41,7 +41,6 @@ DEFINE_EVENT(regmap_reg, regmap_reg_write,
 		 unsigned int val),
 
 	TP_ARGS(map, reg, val)
-
 );
 
 DEFINE_EVENT(regmap_reg, regmap_reg_read,
@@ -50,7 +49,6 @@ DEFINE_EVENT(regmap_reg, regmap_reg_read,
 		 unsigned int val),
 
 	TP_ARGS(map, reg, val)
-
 );
 
 DEFINE_EVENT(regmap_reg, regmap_reg_read_cache,
@@ -59,7 +57,6 @@ DEFINE_EVENT(regmap_reg, regmap_reg_read_cache,
 		 unsigned int val),
 
 	TP_ARGS(map, reg, val)
-
 );
 
 DECLARE_EVENT_CLASS(regmap_bulk,
@@ -199,7 +196,6 @@ DEFINE_EVENT(regmap_bool, regmap_cache_only,
 	TP_PROTO(struct regmap *map, bool flag),
 
 	TP_ARGS(map, flag)
-
 );
 
 DEFINE_EVENT(regmap_bool, regmap_cache_bypass,
@@ -207,7 +203,6 @@ DEFINE_EVENT(regmap_bool, regmap_cache_bypass,
 	TP_PROTO(struct regmap *map, bool flag),
 
 	TP_ARGS(map, flag)
-
 );
 
 DECLARE_EVENT_CLASS(regmap_async,
@@ -239,7 +234,6 @@ DEFINE_EVENT(regmap_async, regmap_async_io_complete,
 	TP_PROTO(struct regmap *map),
 
 	TP_ARGS(map)
-
 );
 
 DEFINE_EVENT(regmap_async, regmap_async_complete_start,
@@ -247,7 +241,6 @@ DEFINE_EVENT(regmap_async, regmap_async_complete_start,
 	TP_PROTO(struct regmap *map),
 
 	TP_ARGS(map)
-
 );
 
 DEFINE_EVENT(regmap_async, regmap_async_complete_done,
@@ -255,7 +248,6 @@ DEFINE_EVENT(regmap_async, regmap_async_complete_done,
 	TP_PROTO(struct regmap *map),
 
 	TP_ARGS(map)
-
 );
 
 TRACE_EVENT(regcache_drop_region,
-- 
2.35.1

Re: [PATCH v1 2/3] regmap: trace: Remove explicit castings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 305 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 2/3] regmap: trace: Remove explicit castings

[Andy Shevchenko]

On Thu, Sep 1, 2022 at 5:15 PM Mark Brown <broonie@kernel.org> 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

Re: [PATCH v1 2/3] regmap: trace: Remove explicit castings

[Steven Rostedt]

On Thu, 1 Sep 2022 17:18:01 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Sep 1, 2022 at 5:15 PM Mark Brown <broonie@kernel.org> 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) <rostedt@goodmis.org>

-- Steve

Re: [PATCH v1 2/3] regmap: trace: Remove explicit castings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 682 bytes --]

On Thu, Sep 01, 2022 at 10:46:01AM -0400, Steven Rostedt wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> 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).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 2/3] regmap: trace: Remove explicit castings

[Steven Rostedt]

On Thu, 1 Sep 2022 15:50:17 +0100
Mark Brown <broonie@kernel.org> 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

Re: [PATCH v1 1/3] regmap: trace: Remove useless check for NULL for bulk ops

[Dmitry Rokosov]

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 <andriy.shevchenko@linux.intel.com>
> ---
>  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 <ddrokosov@sberdevices.ru>

-- 
Thank you,
Dmitry

Re: [PATCH v1 2/3] regmap: trace: Remove explicit castings

[Dmitry Rokosov]

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 <andriy.shevchenko@linux.intel.com>
> ---
>  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 <ddrokosov@sberdevices.ru>

-- 
Thank you,
Dmitry

Re: [PATCH v1 3/3] regmap: trace: Remove unneeded blank lines

[Dmitry Rokosov]

On Thu, Sep 01, 2022 at 04:23:36PM +0300, Andy Shevchenko wrote:
> There is a few unneeded blank lines in some of event definitions,
> remove them in order to make those definitions consistent with
> the rest.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/base/regmap/trace.h | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/base/regmap/trace.h b/drivers/base/regmap/trace.h
> index a0f83e44a9d1..704e106e5dbd 100644
> --- a/drivers/base/regmap/trace.h
> +++ b/drivers/base/regmap/trace.h
> @@ -41,7 +41,6 @@ DEFINE_EVENT(regmap_reg, regmap_reg_write,
>  		 unsigned int val),
>  
>  	TP_ARGS(map, reg, val)
> -
>  );
>  
>  DEFINE_EVENT(regmap_reg, regmap_reg_read,
> @@ -50,7 +49,6 @@ DEFINE_EVENT(regmap_reg, regmap_reg_read,
>  		 unsigned int val),
>  
>  	TP_ARGS(map, reg, val)
> -
>  );
>  
>  DEFINE_EVENT(regmap_reg, regmap_reg_read_cache,
> @@ -59,7 +57,6 @@ DEFINE_EVENT(regmap_reg, regmap_reg_read_cache,
>  		 unsigned int val),
>  
>  	TP_ARGS(map, reg, val)
> -
>  );
>  
>  DECLARE_EVENT_CLASS(regmap_bulk,
> @@ -199,7 +196,6 @@ DEFINE_EVENT(regmap_bool, regmap_cache_only,
>  	TP_PROTO(struct regmap *map, bool flag),
>  
>  	TP_ARGS(map, flag)
> -
>  );
>  
>  DEFINE_EVENT(regmap_bool, regmap_cache_bypass,
> @@ -207,7 +203,6 @@ DEFINE_EVENT(regmap_bool, regmap_cache_bypass,
>  	TP_PROTO(struct regmap *map, bool flag),
>  
>  	TP_ARGS(map, flag)
> -
>  );
>  
>  DECLARE_EVENT_CLASS(regmap_async,
> @@ -239,7 +234,6 @@ DEFINE_EVENT(regmap_async, regmap_async_io_complete,
>  	TP_PROTO(struct regmap *map),
>  
>  	TP_ARGS(map)
> -
>  );
>  
>  DEFINE_EVENT(regmap_async, regmap_async_complete_start,
> @@ -247,7 +241,6 @@ DEFINE_EVENT(regmap_async, regmap_async_complete_start,
>  	TP_PROTO(struct regmap *map),
>  
>  	TP_ARGS(map)
> -
>  );
>  
>  DEFINE_EVENT(regmap_async, regmap_async_complete_done,
> @@ -255,7 +248,6 @@ DEFINE_EVENT(regmap_async, regmap_async_complete_done,
>  	TP_PROTO(struct regmap *map),
>  
>  	TP_ARGS(map)
> -
>  );
>  
>  TRACE_EVENT(regcache_drop_region,
> -- 
> 2.35.1
> 

Reviewed-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>

-- 
Thank you,
Dmitry

Re: [PATCH v1 1/3] regmap: trace: Remove useless check for NULL for bulk ops

[Mark Brown]

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