* [PATCH] media: atomisp: ov2722: replace hardcoded function name
@ 2021-01-05 20:29 Filip Kolev
2021-01-06 7:51 ` Greg Kroah-Hartman
0 siblings, 1 reply; 9+ messages in thread
From: Filip Kolev @ 2021-01-05 20:29 UTC (permalink / raw)
Cc: Filip Kolev, Mauro Carvalho Chehab, Sakari Ailus,
Greg Kroah-Hartman, linux-media, devel, linux-kernel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name
2021-01-05 20:29 [PATCH] media: atomisp: ov2722: replace hardcoded function name Filip Kolev
@ 2021-01-06 7:51 ` Greg Kroah-Hartman
2021-01-06 17:43 ` Filip Kolev
0 siblings, 1 reply; 9+ messages in thread
From: Greg Kroah-Hartman @ 2021-01-06 7:51 UTC (permalink / raw)
To: Filip Kolev
Cc: devel, linux-kernel, Sakari Ailus, Mauro Carvalho Chehab, linux-media
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name
2021-01-06 7:51 ` Greg Kroah-Hartman
@ 2021-01-06 17:43 ` Filip Kolev
2021-01-06 17:52 ` Greg Kroah-Hartman
0 siblings, 1 reply; 9+ messages in thread
From: Filip Kolev @ 2021-01-06 17:43 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: devel, linux-kernel, Sakari Ailus, Mauro Carvalho Chehab, linux-media
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name
2021-01-06 17:43 ` Filip Kolev
@ 2021-01-06 17:52 ` Greg Kroah-Hartman
2021-01-06 18:25 ` Joe Perches
0 siblings, 1 reply; 9+ messages in thread
From: Greg Kroah-Hartman @ 2021-01-06 17:52 UTC (permalink / raw)
To: Filip Kolev
Cc: devel, Mauro Carvalho Chehab, linux-kernel, Sakari Ailus, linux-media
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name
2021-01-06 17:52 ` Greg Kroah-Hartman
@ 2021-01-06 18:25 ` Joe Perches
2021-01-06 19:36 ` Dan Carpenter
0 siblings, 1 reply; 9+ messages in thread
From: Joe Perches @ 2021-01-06 18:25 UTC (permalink / raw)
To: Greg Kroah-Hartman, Filip Kolev
Cc: devel, Mauro Carvalho Chehab, linux-kernel, Sakari Ailus,
linux-media, Andrew Morton
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",
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name
2021-01-06 18:25 ` Joe Perches
@ 2021-01-06 19:36 ` Dan Carpenter
2021-01-06 21:17 ` Joe Perches
0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2021-01-06 19:36 UTC (permalink / raw)
To: Joe Perches
Cc: Greg Kroah-Hartman, Filip Kolev, devel, linux-kernel,
Sakari Ailus, Andrew Morton, Mauro Carvalho Chehab, linux-media
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name
2021-01-06 19:36 ` Dan Carpenter
@ 2021-01-06 21:17 ` Joe Perches
2021-01-07 10:53 ` Dan Carpenter
0 siblings, 1 reply; 9+ messages in thread
From: Joe Perches @ 2021-01-06 21:17 UTC (permalink / raw)
To: Dan Carpenter
Cc: Greg Kroah-Hartman, Filip Kolev, devel, linux-kernel,
Sakari Ailus, Andrew Morton, Mauro Carvalho Chehab, linux-media
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__);
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name
2021-01-06 21:17 ` Joe Perches
@ 2021-01-07 10:53 ` Dan Carpenter
2021-01-08 18:32 ` [PATCH] checkpatch: Prefer ftrace over function entry/exit printks Joe Perches
0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2021-01-07 10:53 UTC (permalink / raw)
To: Joe Perches
Cc: Filip Kolev, devel, Greg Kroah-Hartman, linux-kernel,
Sakari Ailus, Andrew Morton, Mauro Carvalho Chehab, linux-media
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] checkpatch: Prefer ftrace over function entry/exit printks
2021-01-07 10:53 ` Dan Carpenter
@ 2021-01-08 18:32 ` Joe Perches
0 siblings, 0 replies; 9+ messages in thread
From: Joe Perches @ 2021-01-08 18:32 UTC (permalink / raw)
To: Andrew Morton; +Cc: Dan Carpenter, Greg Kroah-Hartman, linux-kernel
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",
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-01-08 18:33 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-05 20:29 [PATCH] media: atomisp: ov2722: replace hardcoded function name Filip Kolev
2021-01-06 7:51 ` Greg Kroah-Hartman
2021-01-06 17:43 ` Filip Kolev
2021-01-06 17:52 ` Greg Kroah-Hartman
2021-01-06 18:25 ` Joe Perches
2021-01-06 19:36 ` Dan Carpenter
2021-01-06 21:17 ` Joe Perches
2021-01-07 10:53 ` Dan Carpenter
2021-01-08 18:32 ` [PATCH] checkpatch: Prefer ftrace over function entry/exit printks Joe Perches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).