* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.