[PATCH] trace, RAS: remove unnecessary const

[PATCH] trace, RAS: remove unnecessary const

[Xie XiuQi]

These parameters are passed by value. There's no need to make them const.

Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
---
 include/ras/ras_event.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
index 79abb9c..b714b22 100644
--- a/include/ras/ras_event.h
+++ b/include/ras/ras_event.h
@@ -95,16 +95,16 @@ TRACE_EVENT(extlog_mem_event,
  */
 TRACE_EVENT(mc_event,
 
-	TP_PROTO(const unsigned int err_type,
+	TP_PROTO(unsigned int err_type,
 		 const char *error_msg,
 		 const char *label,
-		 const int error_count,
-		 const u8 mc_index,
-		 const s8 top_layer,
-		 const s8 mid_layer,
-		 const s8 low_layer,
+		 int error_count,
+		 u8 mc_index,
+		 s8 top_layer,
+		 s8 mid_layer,
+		 s8 low_layer,
 		 unsigned long address,
-		 const u8 grain_bits,
+		 u8 grain_bits,
 		 unsigned long syndrome,
 		 const char *driver_detail),
 
@@ -205,8 +205,8 @@ TRACE_EVENT(mc_event,
 
 TRACE_EVENT(aer_event,
 	TP_PROTO(const char *dev_name,
-		 const u32 status,
-		 const u8 severity),
+		 u32 status,
+		 u8 severity),
 
 	TP_ARGS(dev_name, status, severity),
 
-- 
1.8.3.1

Re: [PATCH] trace, RAS: remove unnecessary const

[Borislav Petkov]

On Thu, Mar 19, 2015 at 04:50:04PM +0800, Xie XiuQi wrote:
> These parameters are passed by value. There's no need to make them const.

I can think of a reason:

include/trace/../../include/ras/ras_event.h: In function ‘ftrace_raw_event_mc_event’:
include/trace/../../include/ras/ras_event.h:136:35: error: assignment of read-only parameter ‘top_layer’
   __entry->top_layer  = top_layer = 12;
                                   ^
---
diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
index 79abb9c71772..e4721eac3e25 100644
--- a/include/ras/ras_event.h
+++ b/include/ras/ras_event.h
@@ -133,7 +133,7 @@ TRACE_EVENT(mc_event,
                __assign_str(label, label);
                __entry->error_count            = error_count;
                __entry->mc_index               = mc_index;
-               __entry->top_layer              = top_layer;
+               __entry->top_layer              = top_layer = 12;
                __entry->middle_layer           = mid_layer;
                __entry->lower_layer            = low_layer;
                __entry->address                = address;
---

I'm not saying it is a particularly sane reason and no one would even
*think* of changing TP parameters passed on from higher layers in the TP
itself but I've seen people do lotsa crazy things - things they normally
would never do - so if it doesn't hurt having the const here, what's the
downside of having the compiler do that sanity checking for us too?

Steve, this is an invitation for your crazy fantasy! :-P

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] trace, RAS: remove unnecessary const

[Xie XiuQi]

On 2015/3/19 18:33, Borislav Petkov wrote:
> On Thu, Mar 19, 2015 at 04:50:04PM +0800, Xie XiuQi wrote:
>> These parameters are passed by value. There's no need to make them const.
> 
> I can think of a reason:
> 
> include/trace/../../include/ras/ras_event.h: In function ‘ftrace_raw_event_mc_event’:
> include/trace/../../include/ras/ras_event.h:136:35: error: assignment of read-only parameter ‘top_layer’
>    __entry->top_layer  = top_layer = 12;

Oh, indeed. Thanks, Boris!

>                                    ^
> ---
> diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
> index 79abb9c71772..e4721eac3e25 100644
> --- a/include/ras/ras_event.h
> +++ b/include/ras/ras_event.h
> @@ -133,7 +133,7 @@ TRACE_EVENT(mc_event,
>                 __assign_str(label, label);
>                 __entry->error_count            = error_count;
>                 __entry->mc_index               = mc_index;
> -               __entry->top_layer              = top_layer;
> +               __entry->top_layer              = top_layer = 12;
>                 __entry->middle_layer           = mid_layer;
>                 __entry->lower_layer            = low_layer;
>                 __entry->address                = address;
> ---
> 
> I'm not saying it is a particularly sane reason and no one would even
> *think* of changing TP parameters passed on from higher layers in the TP
> itself but I've seen people do lotsa crazy things - things they normally
> would never do - so if it doesn't hurt having the const here, what's the
> downside of having the compiler do that sanity checking for us too?
> 
> Steve, this is an invitation for your crazy fantasy! :-P
> 

Re: [PATCH] trace, RAS: remove unnecessary const

[Steven Rostedt]

On Thu, 19 Mar 2015 19:57:17 +0800
Xie XiuQi <xiexiuqi@huawei.com> wrote:

> On 2015/3/19 18:33, Borislav Petkov wrote:
> > On Thu, Mar 19, 2015 at 04:50:04PM +0800, Xie XiuQi wrote:
> >> These parameters are passed by value. There's no need to make them const.
> > 
> > I can think of a reason:
> > 
> > include/trace/../../include/ras/ras_event.h: In function ‘ftrace_raw_event_mc_event’:
> > include/trace/../../include/ras/ras_event.h:136:35: error: assignment of read-only parameter ‘top_layer’
> >    __entry->top_layer  = top_layer = 12;
> 
> Oh, indeed. Thanks, Boris!
> 

Don't get too excited about that answer. If this is indeed the case,
then all functions with parameters that do not get modify later should
be set to const. Do we really want that? And how does this fix:

	__entry->top_layer = 12;

-- Steve

Re: [PATCH] trace, RAS: remove unnecessary const

[Xie XiuQi]

On 2015/3/19 21:00, Steven Rostedt wrote:
> On Thu, 19 Mar 2015 19:57:17 +0800
> Xie XiuQi <xiexiuqi@huawei.com> wrote:
> 
>> On 2015/3/19 18:33, Borislav Petkov wrote:
>>> On Thu, Mar 19, 2015 at 04:50:04PM +0800, Xie XiuQi wrote:
>>>> These parameters are passed by value. There's no need to make them const.
>>>
>>> I can think of a reason:
>>>
>>> include/trace/../../include/ras/ras_event.h: In function ‘ftrace_raw_event_mc_event’:
>>> include/trace/../../include/ras/ras_event.h:136:35: error: assignment of read-only parameter ‘top_layer’
>>>    __entry->top_layer  = top_layer = 12;
>>
>> Oh, indeed. Thanks, Boris!
>>
> 
> Don't get too excited about that answer. If this is indeed the case,
> then all functions with parameters that do not get modify later should
> be set to const. Do we really want that? And how does this fix:
> 
> 	__entry->top_layer = 12;

Yes, I agree. I've retrieved the entire kernel source tree, and did not find
elsewhere such usage. Both sounds reasonable. Now, I've no idea about this patch.

Thanks Steve and Boris!

--
Xie XiuQi

> 
> -- Steve
> 
> .
> 

Re: [PATCH] trace, RAS: remove unnecessary const

[Steven Rostedt]

On Thu, 19 Mar 2015 11:33:30 +0100
Borislav Petkov <bp@suse.de> wrote:

> On Thu, Mar 19, 2015 at 04:50:04PM +0800, Xie XiuQi wrote:
> > These parameters are passed by value. There's no need to make them const.
> 
> I can think of a reason:
> 
> include/trace/../../include/ras/ras_event.h: In function ‘ftrace_raw_event_mc_event’:
> include/trace/../../include/ras/ras_event.h:136:35: error: assignment of read-only parameter ‘top_layer’
>    __entry->top_layer  = top_layer = 12;
>                                    ^
> ---
> diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
> index 79abb9c71772..e4721eac3e25 100644
> --- a/include/ras/ras_event.h
> +++ b/include/ras/ras_event.h
> @@ -133,7 +133,7 @@ TRACE_EVENT(mc_event,
>                 __assign_str(label, label);
>                 __entry->error_count            = error_count;
>                 __entry->mc_index               = mc_index;
> -               __entry->top_layer              = top_layer;
> +               __entry->top_layer              = top_layer = 12;
>                 __entry->middle_layer           = mid_layer;
>                 __entry->lower_layer            = low_layer;
>                 __entry->address                = address;
> ---
> 
> I'm not saying it is a particularly sane reason and no one would even
> *think* of changing TP parameters passed on from higher layers in the TP
> itself but I've seen people do lotsa crazy things - things they normally
> would never do - so if it doesn't hurt having the const here, what's the
> downside of having the compiler do that sanity checking for us too?

That's a bit of a stretch. But sure, there's no real downside to having
it, except that it makes one take a double take when seeing it, and
thinking about why it would even be needed (like I did).

> 
> Steve, this is an invitation for your crazy fantasy! :-P
> 

I think you had the crazy fantasy with the above patch!


-- Steve

Re: [PATCH] trace, RAS: remove unnecessary const

[Borislav Petkov]

On Thu, Mar 19, 2015 at 08:57:56AM -0400, Steven Rostedt wrote:
> That's a bit of a stretch. But sure, there's no real downside to having
> it, except that it makes one take a double take when seeing it, and
> thinking about why it would even be needed (like I did).

Why? It simply declares the contract that this argument won't be
changed. And if there's no downside, there's no need to do anything
here.

> I think you had the crazy fantasy with the above patch!

Wasn't me.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--