[PATCH] media: atomisp: ov2722: replace hardcoded function name

[PATCH] media: atomisp: ov2722: replace hardcoded function name

[Filip Kolev]

There is a debug message using hardcoded function name instead of the
__func__ macro. Replace it.

Report from checkpatch.pl on the file:

WARNING: Prefer using '"%s...", __func__' to using 'ov2722_remove', this function's name, in a string
+	dev_dbg(&client->dev, "ov2722_remove...\n");

Signed-off-by: Filip Kolev <fil.kolev@gmail.com>
---
 drivers/staging/media/atomisp/i2c/atomisp-ov2722.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
index eecefcd734d0e..21d6bc62d452a 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
@@ -1175,7 +1175,7 @@ static int ov2722_remove(struct i2c_client *client)
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
 	struct ov2722_device *dev = to_ov2722_sensor(sd);
 
-	dev_dbg(&client->dev, "ov2722_remove...\n");
+	dev_dbg(&client->dev, "%s...\n", __func__);
 
 	dev->platform_data->csi_cfg(sd, 0);
 	v4l2_ctrl_handler_free(&dev->ctrl_handler);
-- 
2.30.0

Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name

[Greg Kroah-Hartman]

On Tue, Jan 05, 2021 at 10:29:18PM +0200, Filip Kolev wrote:
> There is a debug message using hardcoded function name instead of the
> __func__ macro. Replace it.
> 
> Report from checkpatch.pl on the file:
> 
> WARNING: Prefer using '"%s...", __func__' to using 'ov2722_remove', this function's name, in a string
> +	dev_dbg(&client->dev, "ov2722_remove...\n");
> 
> Signed-off-by: Filip Kolev <fil.kolev@gmail.com>
> ---
>  drivers/staging/media/atomisp/i2c/atomisp-ov2722.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> index eecefcd734d0e..21d6bc62d452a 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> @@ -1175,7 +1175,7 @@ static int ov2722_remove(struct i2c_client *client)
>  	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>  	struct ov2722_device *dev = to_ov2722_sensor(sd);
>  
> -	dev_dbg(&client->dev, "ov2722_remove...\n");
> +	dev_dbg(&client->dev, "%s...\n", __func__);

dev_dbg() provides the function name already, and this is just a "trace"
call, and ftrace should be used instead, so the whole line should be
removed entirely.

thanks,

greg k-h

Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name

[Filip Kolev]

On 06-Jan-21 09:51, Greg Kroah-Hartman wrote:
> On Tue, Jan 05, 2021 at 10:29:18PM +0200, Filip Kolev wrote:
>> There is a debug message using hardcoded function name instead of the
>> __func__ macro. Replace it.
>>
>> Report from checkpatch.pl on the file:
>>
>> WARNING: Prefer using '"%s...", __func__' to using 'ov2722_remove', this function's name, in a string
>> +	dev_dbg(&client->dev, "ov2722_remove...\n");
>>
>> Signed-off-by: Filip Kolev <fil.kolev@gmail.com>
>> ---
>>   drivers/staging/media/atomisp/i2c/atomisp-ov2722.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
>> index eecefcd734d0e..21d6bc62d452a 100644
>> --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
>> +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
>> @@ -1175,7 +1175,7 @@ static int ov2722_remove(struct i2c_client *client)
>>   	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>>   	struct ov2722_device *dev = to_ov2722_sensor(sd);
>>   
>> -	dev_dbg(&client->dev, "ov2722_remove...\n");
>> +	dev_dbg(&client->dev, "%s...\n", __func__);
> 
> dev_dbg() provides the function name already, and this is just a "trace"
> call, and ftrace should be used instead, so the whole line should be
> removed entirely.

Thank you for the review!

How do I go about this? Do I amend the patch and re-send as v2 or create 
a new patch entirely?
Newbie here, doing this as part of the Eudyptula challenge, so I very 
much appreciate everyone's patience.

> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name

[Greg Kroah-Hartman]

On Wed, Jan 06, 2021 at 07:43:42PM +0200, Filip Kolev wrote:
> 
> 
> On 06-Jan-21 09:51, Greg Kroah-Hartman wrote:
> > On Tue, Jan 05, 2021 at 10:29:18PM +0200, Filip Kolev wrote:
> > > There is a debug message using hardcoded function name instead of the
> > > __func__ macro. Replace it.
> > > 
> > > Report from checkpatch.pl on the file:
> > > 
> > > WARNING: Prefer using '"%s...", __func__' to using 'ov2722_remove', this function's name, in a string
> > > +	dev_dbg(&client->dev, "ov2722_remove...\n");
> > > 
> > > Signed-off-by: Filip Kolev <fil.kolev@gmail.com>
> > > ---
> > >   drivers/staging/media/atomisp/i2c/atomisp-ov2722.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> > > index eecefcd734d0e..21d6bc62d452a 100644
> > > --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> > > +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> > > @@ -1175,7 +1175,7 @@ static int ov2722_remove(struct i2c_client *client)
> > >   	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > >   	struct ov2722_device *dev = to_ov2722_sensor(sd);
> > > -	dev_dbg(&client->dev, "ov2722_remove...\n");
> > > +	dev_dbg(&client->dev, "%s...\n", __func__);
> > 
> > dev_dbg() provides the function name already, and this is just a "trace"
> > call, and ftrace should be used instead, so the whole line should be
> > removed entirely.
> 
> Thank you for the review!
> 
> How do I go about this? Do I amend the patch and re-send as v2 or create a
> new patch entirely?

New patch entirely please.

thanks,

greg k-h

Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name

[Joe Perches]

On Wed, 2021-01-06 at 18:52 +0100, Greg Kroah-Hartman wrote:
> On Wed, Jan 06, 2021 at 07:43:42PM +0200, Filip Kolev wrote: 
> > On 06-Jan-21 09:51, Greg Kroah-Hartman wrote:
> > > On Tue, Jan 05, 2021 at 10:29:18PM +0200, Filip Kolev wrote:
> > > > There is a debug message using hardcoded function name instead of the
> > > > __func__ macro. Replace it.
> > > > 
> > > > Report from checkpatch.pl on the file:
> > > > 
> > > > WARNING: Prefer using '"%s...", __func__' to using 'ov2722_remove', this function's name, in a string
> > > > +	dev_dbg(&client->dev, "ov2722_remove...\n");
[]
> > > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
[]
> > > > @@ -1175,7 +1175,7 @@ static int ov2722_remove(struct i2c_client *client)
> > > >   	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > >   	struct ov2722_device *dev = to_ov2722_sensor(sd);
> > > > -	dev_dbg(&client->dev, "ov2722_remove...\n");
> > > > +	dev_dbg(&client->dev, "%s...\n", __func__);
> > > 
> > > dev_dbg() provides the function name already, and this is just a "trace"
> > > call, and ftrace should be used instead, so the whole line should be
> > > removed entirely.
> > 
> > Thank you for the review!
> > 
> > How do I go about this? Do I amend the patch and re-send as v2 or create a
> > new patch entirely?
> 
> New patch entirely please.

There are quite a lot of these relatively useless function tracing like
uses in the kernel:

$ git grep -P '"%s[\.\!]*\\n"\s*,\s*__func__\s*\)' | wc -l
1065

Perhaps yet another checkpatch warning would be useful:
---
 scripts/checkpatch.pl | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e6857bdfcb2d..46b8ec8fe9e1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5981,6 +5981,14 @@ sub process {
 			     "Prefer using '\"%s...\", __func__' to using '$context_function', this function's name, in a string\n" . $herecurr);
 		}
 
+# check for unnecessary function tracing like uses
+		if ($rawline =~ /^\+\s*$logFunctions\s*\([^"]*"%s[\.\!]*\\n"\s*,\s*__func__\s*\)\s*;\s*$/) {
+			if (WARN("TRACING_LOGGING",
+				 "Unnecessary ftrace-like logging - prefer using ftrace\n" . $herecurr) &&
+			    $fix) {
+                                fix_delete_line($fixlinenr, $rawline);
+			}
+		}
 # check for spaces before a quoted newline
 		if ($rawline =~ /^.*\".*\s\\n/) {
 			if (WARN("QUOTED_WHITESPACE_BEFORE_NEWLINE",

Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name

[Dan Carpenter]

On Wed, Jan 06, 2021 at 10:25:26AM -0800, Joe Perches wrote:
> On Wed, 2021-01-06 at 18:52 +0100, Greg Kroah-Hartman wrote:
> > On Wed, Jan 06, 2021 at 07:43:42PM +0200, Filip Kolev wrote: 
> > > On 06-Jan-21 09:51, Greg Kroah-Hartman wrote:
> > > > On Tue, Jan 05, 2021 at 10:29:18PM +0200, Filip Kolev wrote:
> > > > > There is a debug message using hardcoded function name instead of the
> > > > > __func__ macro. Replace it.
> > > > > 
> > > > > Report from checkpatch.pl on the file:
> > > > > 
> > > > > WARNING: Prefer using '"%s...", __func__' to using 'ov2722_remove', this function's name, in a string
> > > > > +	dev_dbg(&client->dev, "ov2722_remove...\n");
> []
> > > > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> []
> > > > > @@ -1175,7 +1175,7 @@ static int ov2722_remove(struct i2c_client *client)
> > > > >   	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > > >   	struct ov2722_device *dev = to_ov2722_sensor(sd);
> > > > > -	dev_dbg(&client->dev, "ov2722_remove...\n");
> > > > > +	dev_dbg(&client->dev, "%s...\n", __func__);
> > > > 
> > > > dev_dbg() provides the function name already, and this is just a "trace"
> > > > call, and ftrace should be used instead, so the whole line should be
> > > > removed entirely.
> > > 
> > > Thank you for the review!
> > > 
> > > How do I go about this? Do I amend the patch and re-send as v2 or create a
> > > new patch entirely?
> > 
> > New patch entirely please.
> 
> There are quite a lot of these relatively useless function tracing like
> uses in the kernel:
> 
> $ git grep -P '"%s[\.\!]*\\n"\s*,\s*__func__\s*\)' | wc -l
> 1065

These are printing other stuff besides just the function name.  Maybe
grep for '", __func__\)'?

regards,
dan carpenter

Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name

[Joe Perches]

On Wed, 2021-01-06 at 22:36 +0300, Dan Carpenter wrote:
> On Wed, Jan 06, 2021 at 10:25:26AM -0800, Joe Perches wrote:
> > On Wed, 2021-01-06 at 18:52 +0100, Greg Kroah-Hartman wrote:
> > > On Wed, Jan 06, 2021 at 07:43:42PM +0200, Filip Kolev wrote: 
> > > > On 06-Jan-21 09:51, Greg Kroah-Hartman wrote:
> > > > > On Tue, Jan 05, 2021 at 10:29:18PM +0200, Filip Kolev wrote:
> > > > > > There is a debug message using hardcoded function name instead of the
> > > > > > __func__ macro. Replace it.
> > > > > > 
> > > > > > Report from checkpatch.pl on the file:
> > > > > > 
> > > > > > WARNING: Prefer using '"%s...", __func__' to using 'ov2722_remove', this function's name, in a string
> > > > > > +	dev_dbg(&client->dev, "ov2722_remove...\n");
[]
> > There are quite a lot of these relatively useless function tracing like
> > uses in the kernel:
> > 
> > $ git grep -P '"%s[\.\!]*\\n"\s*,\s*__func__\s*\)' | wc -l
> > 1065
> 
> These are printing other stuff besides just the function name.

No, these are printing _only_ the function name.

> Maybe grep for '", __func__\)'?

No, that wouldn't work as there are many many uses like:

	printk('%s: some string\n", __func__);

Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name

[Dan Carpenter]

On Wed, Jan 06, 2021 at 01:17:47PM -0800, Joe Perches wrote:
> On Wed, 2021-01-06 at 22:36 +0300, Dan Carpenter wrote:
> > On Wed, Jan 06, 2021 at 10:25:26AM -0800, Joe Perches wrote:
> > > On Wed, 2021-01-06 at 18:52 +0100, Greg Kroah-Hartman wrote:
> > > > On Wed, Jan 06, 2021 at 07:43:42PM +0200, Filip Kolev wrote: 
> > > > > On 06-Jan-21 09:51, Greg Kroah-Hartman wrote:
> > > > > > On Tue, Jan 05, 2021 at 10:29:18PM +0200, Filip Kolev wrote:
> > > > > > > There is a debug message using hardcoded function name instead of the
> > > > > > > __func__ macro. Replace it.
> > > > > > > 
> > > > > > > Report from checkpatch.pl on the file:
> > > > > > > 
> > > > > > > WARNING: Prefer using '"%s...", __func__' to using 'ov2722_remove', this function's name, in a string
> > > > > > > +	dev_dbg(&client->dev, "ov2722_remove...\n");
> []
> > > There are quite a lot of these relatively useless function tracing like
> > > uses in the kernel:
> > > 
> > > $ git grep -P '"%s[\.\!]*\\n"\s*,\s*__func__\s*\)' | wc -l
> > > 1065
> > 
> > These are printing other stuff besides just the function name.
> 
> No, these are printing _only_ the function name.
> 

Oh...  Duh...  I was looking at the complete wrong output.  My bad.

Yeah.  I like this warning.

regards,
dan carpenter

[PATCH] checkpatch: Prefer ftrace over function entry/exit printks

[Joe Perches]

Prefer using ftrace over function entry/exit logging messages.

Warn with various function entry/exit only logging that only
use __func__ with or without descriptive decoration.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e6857bdfcb2d..7c95f23352ae 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -507,6 +507,30 @@ our $signature_tags = qr{(?xi:
 	Cc:
 )};
 
+our $tracing_logging_tags = qr{(?xi:
+	[=-]*> |
+	<[=-]* |
+	\[ |
+	\] |
+	start |
+	called |
+	entered |
+	entry |
+	enter |
+	in |
+	inside |
+	here |
+	begin |
+	exit |
+	end |
+	done |
+	leave |
+	completed |
+	out |
+	return |
+	[\.\!:\s]*
+)};
+
 sub edit_distance_min {
 	my (@arr) = @_;
 	my $len = scalar @arr;
@@ -5981,6 +6005,17 @@ sub process {
 			     "Prefer using '\"%s...\", __func__' to using '$context_function', this function's name, in a string\n" . $herecurr);
 		}
 
+# check for unnecessary function tracing like uses
+# This does not use $logFunctions because there are many instances like
+# 'dprintk(FOO, "%s()\n", __func__);' which do not match $logFunctions
+		if ($rawline =~ /^\+.*\([^"]*"$tracing_logging_tags{0,3}%s(?:\s*\(\s*\)\s*)?$tracing_logging_tags{0,3}(?:\\n)?"\s*,\s*__func__\s*\)\s*;/) {
+			if (WARN("TRACING_LOGGING",
+				 "Unnecessary ftrace-like logging - prefer using ftrace\n" . $herecurr) &&
+			    $fix) {
+                                fix_delete_line($fixlinenr, $rawline);
+			}
+		}
+
 # check for spaces before a quoted newline
 		if ($rawline =~ /^.*\".*\s\\n/) {
 			if (WARN("QUOTED_WHITESPACE_BEFORE_NEWLINE",