[PATCH] eudyptula challenge

[PATCH] eudyptula challenge

[Chewie Lin]

Hi all:

Better and improved changelog version 3. 

I'm submitting this patch as part of Eudyptula challenge to fix a
coding style problem.  Thanks for taking time on this trivial patch.

sl424

Chewie Lin (1):
	drivers/staging/vt6656/main_usb.c: checkpatch warning

 drivers/staging/vt6656/main_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.10.0

[PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Chewie Lin]

Replace string with formatted arguments in the dev_warn() call. It removes
the checkpatch warning:

	WARNING: Prefer using "%s", __func__ to embedded function names
	#417: FILE: main_usb.c:417:
	+			 "usb_device_reset fail status=%d\n", status);

	total: 0 errors, 1 warnings, 1058 lines checked

And after fix:

	main_usb.c has no obvious style problems and is ready for submission.

Signed-off-by: Chewie Lin <linsh@oregonstate.edu>
---
 drivers/staging/vt6656/main_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
index 9e074e9..2d9e7af 100644
--- a/drivers/staging/vt6656/main_usb.c
+++ b/drivers/staging/vt6656/main_usb.c
@@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv)
 	status = usb_reset_device(priv->usb);
 	if (status)
 		dev_warn(&priv->usb->dev,
-			 "usb_device_reset fail status=%d\n", status);
+			 "%s=%d\n", "usb_device_reset fail status", status);
 }
 
 static void vnt_free_int_bufs(struct vnt_private *priv)
-- 
2.10.0

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Randy Dunlap]

On 03/31/17 18:59, Chewie Lin wrote:
> Replace string with formatted arguments in the dev_warn() call. It removes
> the checkpatch warning:
> 
> 	WARNING: Prefer using "%s", __func__ to embedded function names
> 	#417: FILE: main_usb.c:417:
> 	+			 "usb_device_reset fail status=%d\n", status);
> 
> 	total: 0 errors, 1 warnings, 1058 lines checked
> 
> And after fix:
> 
> 	main_usb.c has no obvious style problems and is ready for submission.
> 
> Signed-off-by: Chewie Lin <linsh@oregonstate.edu>
> ---
>  drivers/staging/vt6656/main_usb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
> index 9e074e9..2d9e7af 100644
> --- a/drivers/staging/vt6656/main_usb.c
> +++ b/drivers/staging/vt6656/main_usb.c
> @@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv)
>  	status = usb_reset_device(priv->usb);
>  	if (status)
>  		dev_warn(&priv->usb->dev,
> -			 "usb_device_reset fail status=%d\n", status);
> +			 "%s=%d\n", "usb_device_reset fail status", status);
>  }
>  
>  static void vnt_free_int_bufs(struct vnt_private *priv)
> 

As other people have said:

This function is usb_device_reset().  If that function name string is to be
used in the message, it should be done so by using __func__.
See http://marc.info/?l=linux-driver-devel&m=149095639202492&w=2

Or is the called (failing) function name is to be kept in the message (as it
is currently), then the message should contain "usb_reset_device" instead of
"usb_device_reset".  See http://marc.info/?l=linux-driver-devel&m=149098680312723&w=2



-- 
~Randy

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Joe Perches]

On Fri, 2017-03-31 at 18:59 -0700, Chewie Lin wrote:
> Replace string with formatted arguments in the dev_warn() call. It removes
> the checkpatch warning:
> 
> 	WARNING: Prefer using "%s", __func__ to embedded function names
> 	#417: FILE: main_usb.c:417:
> 	+			 "usb_device_reset fail status=%d\n", status);
> 
> 	total: 0 errors, 1 warnings, 1058 lines checked
> 
> And after fix:
> 
> 	main_usb.c has no obvious style problems and is ready for submission.
> 
> Signed-off-by: Chewie Lin <linsh@oregonstate.edu>
> ---
>  drivers/staging/vt6656/main_usb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
> index 9e074e9..2d9e7af 100644
> --- a/drivers/staging/vt6656/main_usb.c
> +++ b/drivers/staging/vt6656/main_usb.c
> @@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv)
>  	status = usb_reset_device(priv->usb);
>  	if (status)
>  		dev_warn(&priv->usb->dev,
> -			 "usb_device_reset fail status=%d\n", status);
> +			 "%s=%d\n", "usb_device_reset fail status", status);

So, what is failing?  usb_reset_device or usb_device_reset?

And this function is used exactly once, is trivial, and
would likely be better as direct code in the one place
it's used.

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Joe Perches]

On Fri, 2017-03-31 at 19:15 -0700, Randy Dunlap wrote:
> On 03/31/17 18:59, Chewie Lin wrote:
> > Replace string with formatted arguments in the dev_warn() call. It removes
> > the checkpatch warning:
> > 
> > 	WARNING: Prefer using "%s", __func__ to embedded function names
> > 	#417: FILE: main_usb.c:417:
> > 	+			 "usb_device_reset fail status=%d\n", status);
> > 
> > 	total: 0 errors, 1 warnings, 1058 lines checked
> > 
> > And after fix:
> > 
> > 	main_usb.c has no obvious style problems and is ready for submission.
> > 
> > Signed-off-by: Chewie Lin <linsh@oregonstate.edu>
> > ---
> >  drivers/staging/vt6656/main_usb.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
> > index 9e074e9..2d9e7af 100644
> > --- a/drivers/staging/vt6656/main_usb.c
> > +++ b/drivers/staging/vt6656/main_usb.c
> > @@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv)
> >  	status = usb_reset_device(priv->usb);
> >  	if (status)
> >  		dev_warn(&priv->usb->dev,
> > -			 "usb_device_reset fail status=%d\n", status);
> > +			 "%s=%d\n", "usb_device_reset fail status", status);
> >  }
> >  
> >  static void vnt_free_int_bufs(struct vnt_private *priv)
> > 
> 
> As other people have said:
> 
> This function is usb_device_reset().  If that function name string is to be
> used in the message, it should be done so by using __func__.
> See http://marc.info/?l=linux-driver-devel&m=149095639202492&w=2
> 
> Or is the called (failing) function name is to be kept in the message (as it
> is currently), then the message should contain "usb_reset_device" instead of
> "usb_device_reset".  See http://marc.info/?l=linux-driver-devel&m=149098680312723&w=2

If this is to be changed at all, I suggest
just getting rid of the function.
---
 drivers/staging/vt6656/main_usb.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
index 9e074e9daf4e..799aeeb6812d 100644
--- a/drivers/staging/vt6656/main_usb.c
+++ b/drivers/staging/vt6656/main_usb.c
@@ -407,16 +407,6 @@ static void vnt_free_rx_bufs(struct vnt_private *priv)
 	}
 }
 
-static void usb_device_reset(struct vnt_private *priv)
-{
-	int status;
-
-	status = usb_reset_device(priv->usb);
-	if (status)
-		dev_warn(&priv->usb->dev,
-			 "usb_device_reset fail status=%d\n", status);
-}
-
 static void vnt_free_int_bufs(struct vnt_private *priv)
 {
 	kfree(priv->int_buf.data_buf);
@@ -995,7 +985,9 @@ vt6656_probe(struct usb_interface *intf, const struct usb_device_id *id)
 
 	SET_IEEE80211_DEV(priv->hw, &intf->dev);
 
-	usb_device_reset(priv);
+	rc = usb_reset_device(priv->usb);
+	if (rc)
+		dev_warn(&priv->usb->dev, "usb_reset_device failed: %d\n", rc);
 
 	clear_bit(DEVICE_FLAGS_DISCONNECTED, &priv->flags);
 	vnt_reset_command_timer(priv);

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Chewie Lin]

On Fri, Mar 31, 2017 at 07:45:09PM -0700, Joe Perches wrote:
> On Fri, 2017-03-31 at 19:15 -0700, Randy Dunlap wrote:
> > On 03/31/17 18:59, Chewie Lin wrote:
> > > Replace string with formatted arguments in the dev_warn() call. It removes
> > > the checkpatch warning:
> > > 
> > > 	WARNING: Prefer using "%s", __func__ to embedded function names
> > > 	#417: FILE: main_usb.c:417:
> > > 	+			 "usb_device_reset fail status=%d\n", status);
> > > 
> > > 	total: 0 errors, 1 warnings, 1058 lines checked
> > > 
> > > And after fix:
> > > 
> > > 	main_usb.c has no obvious style problems and is ready for submission.
> > > 
> > > Signed-off-by: Chewie Lin <linsh@oregonstate.edu>
> > > ---
> > >  drivers/staging/vt6656/main_usb.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
> > > index 9e074e9..2d9e7af 100644
> > > --- a/drivers/staging/vt6656/main_usb.c
> > > +++ b/drivers/staging/vt6656/main_usb.c
> > > @@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv)
> > >  	status = usb_reset_device(priv->usb);
> > >  	if (status)
> > >  		dev_warn(&priv->usb->dev,
> > > -			 "usb_device_reset fail status=%d\n", status);
> > > +			 "%s=%d\n", "usb_device_reset fail status", status);
> > >  }
> > >  
> > >  static void vnt_free_int_bufs(struct vnt_private *priv)
> > > 
> > 
> > As other people have said:
> > 
> > This function is usb_device_reset().  If that function name string is to be
> > used in the message, it should be done so by using __func__.
> > See http://marc.info/?l=linux-driver-devel&m=149095639202492&w=2
> > 
> > Or is the called (failing) function name is to be kept in the message (as it
> > is currently), then the message should contain "usb_reset_device" instead of
> > "usb_device_reset".  See http://marc.info/?l=linux-driver-devel&m=149098680312723&w=2
> 
> If this is to be changed at all, I suggest
> just getting rid of the function.

These are good points, but any feedback on the dev_warn() call itself? 
I was trying to fix the checkpatch warning on my first try.

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Al Viro]

On Fri, Mar 31, 2017 at 06:59:19PM -0700, Chewie Lin wrote:
> Replace string with formatted arguments in the dev_warn() call. It removes
> the checkpatch warning:
> 
> 	WARNING: Prefer using "%s", __func__ to embedded function names
> 	#417: FILE: main_usb.c:417:
> 	+			 "usb_device_reset fail status=%d\n", status);
> 
> 	total: 0 errors, 1 warnings, 1058 lines checked
> 
> And after fix:
> 
> 	main_usb.c has no obvious style problems and is ready for submission.


> -			 "usb_device_reset fail status=%d\n", status);
> +			 "%s=%d\n", "usb_device_reset fail status", status);

Would you mind explaining the meaning of that warning?  Incidentally, why not
			  "%se_reset fail status=%d\n", "usb_devic", status);
- it would produce the identical output and silences checkpatch even more
reliably.  Discuss.

Sarcasm aside, when you are proposing a change, there are several questions you
really must ask yourself and be able to answer.  First and foremost, of course,
is
	* Why is it an improvement?
If the answer to that was "it makes a warning go away", the next questions are
	* What are we warned about?
	* What is problematic in the original variant?
	* Is the replacement free from the problem we had in the original?
and if the answer to any of that is "I don't know", the next step is _not_
"send it anyway".  "Try to figure out" might be a good idea, with usual
heuristics regarding the reading comprehension ("if a sentence remains
a mystery, see if there are any unfamiliar words skipped at the first reading
and find what they mean", etc.) applying.

In this particular case the part you've apparently skipped was, indeed,
critical - "__func__".  I am not trying to defend the quality of checkpatch -
the warning is badly worded, the tool is badly written and more often than
not its suggestions reflect nothing but authors' arbitrary preferences.

This one, however, does have some rationale behind it.  Namely, if some
debugging output contains the name of a function that has produced it,
having that name spelled out in format string is not a good idea.  If
that code gets moved elsewhere one would have to replace the name in format
string or face confusing messages refering to the function where that
code used to be.  Since __func__ is interpreted as a constant string
initialized with the name of the function it's used in, it is possible
to avoid spelling the function name out - instead of
"my_function: pink elephants; reduce the daily intake to %d glasses\n", n
one could use
"%s: pink elephants; reduce the daily intake to %d glasses\n", __func__, n
producing the same output and avoiding the need to adjust anything when
the whole thing is moved to another function.  It's not particularly big
deal, but it's a more or less reasonable suggestion.

Your change, of course, has achieved only one thing - it has moved the
explicitly spelled out function name out of format string.  It's still
just as explicitly spelled out, still would need to be adjusted if moved
to another function and the whole thing has become harder to read and
understand since you've buried other parts of message in the place where
it's harder to follow.

Again, checkpatch warning is badly written, but the main problem with
your posting is not that you had been confused by checkpatch - it's
that you have posted it based on an incomprehensible message with no better
rationale than "it shuts checkpatch up, dunno why and what about".

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Joe Perches]

On Fri, 2017-03-31 at 20:18 -0700, Chewie Lin wrote:
> These are good points, but any feedback on the dev_warn() call itself?
> I was trying to fix the checkpatch warning on my first try.

Using "%s" with a long string is generally inefficient.

Compare the compiled object size of
	printf("%s is %d\n", "Long string", 1);
to
	printf("Long string is %d\n", 1); 

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Joe Perches]

On Sat, 2017-04-01 at 04:32 +0100, Al Viro wrote:
> On Fri, Mar 31, 2017 at 06:59:19PM -0700, Chewie Lin wrote:
> > Replace string with formatted arguments in the dev_warn() call. It removes
> > the checkpatch warning:
> > 
> > 	WARNING: Prefer using "%s", __func__ to embedded function names
[]
> Again, checkpatch warning is badly written

In your opinion, what wording would be better?

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Al Viro]

On Fri, Mar 31, 2017 at 08:36:22PM -0700, Joe Perches wrote:
> On Sat, 2017-04-01 at 04:32 +0100, Al Viro wrote:
> > On Fri, Mar 31, 2017 at 06:59:19PM -0700, Chewie Lin wrote:
> > > Replace string with formatted arguments in the dev_warn() call. It removes
> > > the checkpatch warning:
> > > 
> > > 	WARNING: Prefer using "%s", __func__ to embedded function names
> []
> > Again, checkpatch warning is badly written
> 
> In your opinion, what wording would be better?

MILD SUGGESTION: don't spell the function name out in format strings;
	"this_function: foo is %d", n
might be better off as
	"%s: foo is %d", __func__, n
in case you ever move it to another function or rename your function.

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Joe Perches]

On Sat, 2017-04-01 at 04:46 +0100, Al Viro wrote:
> On Fri, Mar 31, 2017 at 08:36:22PM -0700, Joe Perches wrote:
> > On Sat, 2017-04-01 at 04:32 +0100, Al Viro wrote:
> > > On Fri, Mar 31, 2017 at 06:59:19PM -0700, Chewie Lin wrote:
> > > > Replace string with formatted arguments in the dev_warn() call. It removes
> > > > the checkpatch warning:
> > > > 
> > > > 	WARNING: Prefer using "%s", __func__ to embedded function names
> > 
> > []
> > > Again, checkpatch warning is badly written
> > 
> > In your opinion, what wording would be better?
> 
> MILD SUGGESTION: don't spell the function name out in format strings;
> 	"this_function: foo is %d", n
> might be better off as
> 	"%s: foo is %d", __func__, n
> in case you ever move it to another function or rename your function.

Thank you sir, may I have another.

checkpatch messages are single line.

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Al Viro]

On Fri, Mar 31, 2017 at 08:52:50PM -0700, Joe Perches wrote:

> > MILD SUGGESTION: don't spell the function name out in format strings;
> > 	"this_function: foo is %d", n
> > might be better off as
> > 	"%s: foo is %d", __func__, n
> > in case you ever move it to another function or rename your function.
> 
> Thank you sir, may I have another.
> 
> checkpatch messages are single line.

Too bad... Incidentally, being able to get more detailed explanation of
a warning might be a serious improvement, especially if it contains
the rationale.  Hell, something like TeX handling of errors might be
a good idea - warning printed, offered actions include 'give more help',
'continue', 'exit', 'from now on suppress this kind of warning', 'from
now on just dump this kind of warning into log and keep going', 'from
now on dump all warnings into log and keep going'.

And yes, I'm serious about having something like "mild suggestion" as
possible severity - people are using that thing to look for potential
improvements to make and 'such and such change might be useful for such
and such reasons' is a lot more useful than 'this needs to be thus because
it must be thus or I'll keep warning'.

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Joe Perches]

On Sat, 2017-04-01 at 05:08 +0100, Al Viro wrote:
> On Fri, Mar 31, 2017 at 08:52:50PM -0700, Joe Perches wrote:
> 
> > > MILD SUGGESTION: don't spell the function name out in format strings;
> > > 	"this_function: foo is %d", n
> > > might be better off as
> > > 	"%s: foo is %d", __func__, n
> > > in case you ever move it to another function or rename your function.
> > 
> > Thank you sir, may I have another.
> > 
> > checkpatch messages are single line.
> 
> Too bad... Incidentally, being able to get more detailed explanation of
> a warning might be a serious improvement, especially if it contains
> the rationale.  Hell, something like TeX handling of errors might be
> a good idea - warning printed, offered actions include 'give more help',
> 'continue', 'exit', 'from now on suppress this kind of warning', 'from
> now on just dump this kind of warning into log and keep going', 'from
> now on dump all warnings into log and keep going'.

Well, there is the possibility to have longer messages.
It's just the --terse option has to be somewhat sensible.

> And yes, I'm serious about having something like "mild suggestion" as
> possible severity - people are using that thing to look for potential
> improvements to make and 'such and such change might be useful for such
> and such reasons' is a lot more useful than 'this needs to be thus because
> it must be thus or I'll keep warning'.

I agree about checkpatch and ERROR/WARNING/CHECK vs some other wording.

https://lkml.org/lkml/2016/8/27/180
https://lkml.org/lkml/2015/7/16/568

The other thing that might help is for people to take
the warnings the script produces less seriously.

Maybe convert:

ERROR -> defect
WARNING -> unstylish
CHECK -> nitpick

or some such.

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Chewie Lin]

On Sat, Apr 01, 2017 at 04:32:39AM +0100, Al Viro wrote:
> On Fri, Mar 31, 2017 at 06:59:19PM -0700, Chewie Lin wrote:
> > Replace string with formatted arguments in the dev_warn() call. It removes
> > the checkpatch warning:
> > 
> > 	WARNING: Prefer using "%s", __func__ to embedded function names
> > 	#417: FILE: main_usb.c:417:
> > 	+			 "usb_device_reset fail status=%d\n", status);
> > 
> > 	total: 0 errors, 1 warnings, 1058 lines checked
> > 
> > And after fix:
> > 
> > 	main_usb.c has no obvious style problems and is ready for submission.
> 
> 
> > -			 "usb_device_reset fail status=%d\n", status);
> > +			 "%s=%d\n", "usb_device_reset fail status", status);
> 
> Would you mind explaining the meaning of that warning?  Incidentally, why not
> 			  "%se_reset fail status=%d\n", "usb_devic", status);
> - it would produce the identical output and silences checkpatch even more
> reliably.  Discuss.
> 
> Sarcasm aside, when you are proposing a change, there are several questions you
> really must ask yourself and be able to answer.  First and foremost, of course,
> is
> 	* Why is it an improvement?
> If the answer to that was "it makes a warning go away", the next questions are
> 	* What are we warned about?
> 	* What is problematic in the original variant?
> 	* Is the replacement free from the problem we had in the original?
> and if the answer to any of that is "I don't know", the next step is _not_
> "send it anyway".  "Try to figure out" might be a good idea, with usual
> heuristics regarding the reading comprehension ("if a sentence remains
> a mystery, see if there are any unfamiliar words skipped at the first reading
> and find what they mean", etc.) applying.
> 
> In this particular case the part you've apparently skipped was, indeed,
> critical - "__func__".  I am not trying to defend the quality of checkpatch -
> the warning is badly worded, the tool is badly written and more often than
> not its suggestions reflect nothing but authors' arbitrary preferences.
> 
> This one, however, does have some rationale behind it.  Namely, if some
> debugging output contains the name of a function that has produced it,
> having that name spelled out in format string is not a good idea.  If
> that code gets moved elsewhere one would have to replace the name in format
> string or face confusing messages refering to the function where that
> code used to be.  Since __func__ is interpreted as a constant string
> initialized with the name of the function it's used in, it is possible
> to avoid spelling the function name out - instead of
> "my_function: pink elephants; reduce the daily intake to %d glasses\n", n
> one could use
> "%s: pink elephants; reduce the daily intake to %d glasses\n", __func__, n
> producing the same output and avoiding the need to adjust anything when
> the whole thing is moved to another function.  It's not particularly big
> deal, but it's a more or less reasonable suggestion.
> 
> Your change, of course, has achieved only one thing - it has moved the
> explicitly spelled out function name out of format string.  It's still
> just as explicitly spelled out, still would need to be adjusted if moved
> to another function and the whole thing has become harder to read and
> understand since you've buried other parts of message in the place where
> it's harder to follow.
> 
> Again, checkpatch warning is badly written, but the main problem with
> your posting is not that you had been confused by checkpatch - it's
> that you have posted it based on an incomprehensible message with no better
> rationale than "it shuts checkpatch up, dunno why and what about".

Al, brilliant, that's exactly what I was trying to do on my first try. 
The checkpatch *is* confusing. It's fine with a simple string but doesn't 
like it when it's formatted string. From what you said, I 
think this may work better and more portable: 

"%s: fail status = %d\n", "usb_device_reset", status)