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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201103100440.wo2wkkyr5rs4qhhl@holly.lan>
Date:   Tue, 3 Nov 2020 10:04:40 +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 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:
> > On Sat, Oct 31, 2020 at 05:41:03PM +0100, Tabot Kevin wrote:
> > > This patch fixes the following:
> > > - Uses __func__ macro to print function names.
> > > - Got rid of unnecessary braces around single line if statements.
> > > - End of block comments on a seperate line.
> > > - A spelling mistake of the word "on".
> > > 
> > > Signed-off-by: Tabot Kevin <tabot.kevin@...il.com>
> > > ---
> > >  drivers/staging/media/atomisp/i2c/atomisp-ov2680.c | 25 +++++++++++-----------
> > >  1 file changed, 13 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> > > index c907305..1396a33 100644
> > > --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> > > +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> > > @@ -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__?

[Responses on LKML should be quoted like this rather than top-posted]

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.


> > > @@ -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.