linux-kernel - Re: [PATCH] Replaced hard coded function names in debug messages with __func_

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20201104100634.4badc2mmau2tk5ct@holly.lan>
Date:   Wed, 4 Nov 2020 10:06:34 +0000
From:   Daniel Thompson <daniel.thompson@...aro.org>
To:     Tabot Kevin <tabot.kevin@...il.com>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Replaced hard coded function names in debug messages
 with __func__ macro.

On Tue, Nov 03, 2020 at 09:36:54PM +0100, Tabot Kevin wrote:
> On Tue, Nov 03, 2020 at 10:04:40AM +0000, Daniel Thompson wrote:
> > On Mon, Nov 02, 2020 at 01:15:56PM +0100, Tabot Kevin wrote:
> > > On Mon, Nov 02, 2020 at 09:33:24AM +0000, Daniel Thompson wrote:
> > > > > @@ -146,7 +146,7 @@ static int ov2680_g_bin_factor_x(struct v4l2_subdev *sd, s32 *val)
> > > > >  	struct ov2680_device *dev = to_ov2680_sensor(sd);
> > > > >  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> > > > >  
> > > > > -	dev_dbg(&client->dev,  "++++ov2680_g_bin_factor_x\n");
> > > > > +	dev_dbg(&client->dev,  "++++%s\n", __func__);
> > > > 
> > > > It might be better just to remove this sort of message.
> > > > 
> > > > They are not "wrong wrong" but are they actually useful one a
> > > > driver's basic functions work? Even where they are useful
> > > > dynamic techniques (ftrace, tracepoints, etc) arguably provide a
> > > > better way to support "did my function actually run" debug
> > > > approaches anyway.
> > >
> > > Thank you very much for the response. So, should I just revert back to
> > > the original all the changes in places where I replace hard coded
> > > functions names with  __func__?
> > 
> > Personally I think it is better to remove them completely from the
> > driver rather than revert to the original form. Naturally if Mauro or
> > Sakari have strong views on this kind of printed message then you
> > need to take that into account but, in general, messages like this
> > add little or no value to the driver and can be removed.
> > 
> I went through the code in an attempt to completely remove all "dev_dbg"
> messages,

The goal should not be to remove all dev_dbg() messages. I have only
suggested removing dev_dbg() that print things that are not useful or
redundantly duplicate what can be achieved with ftrace or tracepoints.

Maybe that will remove the dev_dbg() messages and maybe it won't. That
depends entirely what the function actually prints when executed.


> but I noticed not only are there many "dev_dbg" messages, there
> are also many such messages like (dev_info, dev_err, etc). Should I
> remove them all too?

The resulting patch will have your name on it rather than mine. That
means it is you that must make the decisions here.

Firstly you can review each message output to decide if it is useful.
Only remove message whose output is not useful (same as for dev_dbg() ).

If it is useful then you should think about whether the log level
matches the importance of the message. For example, are the dev_err()
message really covering error conditions? are there warnings for normal"
conditions? is the dev_info() useful to someone who is not the driver
author?).


Daniel.




> > 
> > > > > @@ -251,8 +251,8 @@ static long __ov2680_set_exposure(struct v4l2_subdev *sd, int coarse_itg,
> > > > >  	int ret, exp_val;
> > > > >  
> > > > >  	dev_dbg(&client->dev,
> > > > > -		"+++++++__ov2680_set_exposure coarse_itg %d, gain %d, digitgain %d++\n",
> > > > > -		coarse_itg, gain, digitgain);
> > > > > +		"+++++++%s coarse_itg %d, gain %d, digitgain %d++\n",
> > > > > +		__func__, coarse_itg, gain, digitgain);
> > 
> > This case is a little less clear cut since the printed message does show
> > some elements of internal state. However AFAICT this function just writes
> > some state to the hardware so I still take the view that dynamic
> > tools (I2C tracepoints for example) provide a better way to debug the
> > driver.
> > 
> > 
> > Daniel.