[v1,1/1] checkpatch: Add dev_err_probe() to the list of Log Functions

  dev_err_probe() is missing in the list of Log Functions and hence
checkpatch issues a warning in the cases when any other function
in use won't trigger it. Add dev_err_probe() to the list to behave
consistently.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 scripts/checkpatch.pl | 1 +
 1 file changed, 1 insertion(+)
  

On 12/1/23 07:14, Andy Shevchenko wrote:
> dev_err_probe() is missing in the list of Log Functions and hence
> checkpatch issues a warning in the cases when any other function
> in use won't trigger it. Add dev_err_probe() to the list to behave
> consistently.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   scripts/checkpatch.pl | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index a94ed6c46a6d..c40f3f784f7e 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -593,6 +593,7 @@ our $zero_initializer = qr{(?:(?:0[xX])?0+$Int_type?|NULL|false)\b};
>   our $logFunctions = qr{(?x:
>   	printk(?:_ratelimited|_once|_deferred_once|_deferred|)|
>   	(?:[a-z0-9]+_){1,2}(?:printk|emerg|alert|crit|err|warning|warn|notice|info|debug|dbg|vdbg|devel|cont|WARN)(?:_ratelimited|_once|)|
> +	dev_err_probe|
>   	TP_printk|
>   	WARN(?:_RATELIMIT|_ONCE|)|
>   	panic|


Not sure if I agree. The difference here is that dev_err_probe()
has two additional parameters ahead of the string. I would very much prefer
to have those two additional parameters on a separate line if the string is
too long to fit in 100 columns with those two parameters on the same line.
In other words, I very much prefer

	dev_err_probe(dev, -ESOMETHING,
		      "very long string");
over
	dev_err_probe(dev, -ESOMETHING, "very long string");

and I don't really think that the latter has any benefits.

Also note that other dev_xxx() log functions are not included in the above test
and would still generate warnings. Accepting

	dev_err_probe(dev, -ESOMETHING, "very long string");
but not
	dev_err(dev, "very long string");

doesn't really make sense to me.

Guenter
  

On Fri, Dec 01, 2023 at 08:01:28AM -0800, Guenter Roeck wrote:
> On 12/1/23 07:14, Andy Shevchenko wrote:
> > dev_err_probe() is missing in the list of Log Functions and hence
> > checkpatch issues a warning in the cases when any other function
> > in use won't trigger it. Add dev_err_probe() to the list to behave
> > consistently.

...

> Not sure if I agree. The difference here is that dev_err_probe()
> has two additional parameters ahead of the string. I would very much prefer
> to have those two additional parameters on a separate line if the string is
> too long to fit in 100 columns with those two parameters on the same line.
> In other words, I very much prefer
> 
> 	dev_err_probe(dev, -ESOMETHING,
> 		      "very long string");
> over
> 	dev_err_probe(dev, -ESOMETHING, "very long string");
> 
> and I don't really think that the latter has any benefits.
> 
> Also note that other dev_xxx() log functions are not included in the above test
> and would still generate warnings. Accepting
> 
> 	dev_err_probe(dev, -ESOMETHING, "very long string");
> but not
> 	dev_err(dev, "very long string");

They are included, see the line previous to the added one.
(Regexp covers something like x_y_()* and x_*() families with the explicitly
 listed * suffixes.)

That's why _this_ change makes it consistent.

> doesn't really make sense to me.
  

On Fri, Dec 01, 2023 at 06:17:51PM +0200, Andy Shevchenko wrote:
> On Fri, Dec 01, 2023 at 08:01:28AM -0800, Guenter Roeck wrote:
> > On 12/1/23 07:14, Andy Shevchenko wrote:
> > > dev_err_probe() is missing in the list of Log Functions and hence
> > > checkpatch issues a warning in the cases when any other function
> > > in use won't trigger it. Add dev_err_probe() to the list to behave
> > > consistently.

...

> > Not sure if I agree. The difference here is that dev_err_probe()
> > has two additional parameters ahead of the string. I would very much prefer
> > to have those two additional parameters on a separate line if the string is
> > too long to fit in 100 columns with those two parameters on the same line.
> > In other words, I very much prefer
> > 
> > 	dev_err_probe(dev, -ESOMETHING,
> > 		      "very long string");
> > over
> > 	dev_err_probe(dev, -ESOMETHING, "very long string");
> > 
> > and I don't really think that the latter has any benefits.
> > 
> > Also note that other dev_xxx() log functions are not included in the above test
> > and would still generate warnings. Accepting
> > 
> > 	dev_err_probe(dev, -ESOMETHING, "very long string");
> > but not
> > 	dev_err(dev, "very long string");
> 
> They are included, see the line previous to the added one.
> (Regexp covers something like x_y_()* and x_*() families with the explicitly

Should read: x_y_*()

>  listed * suffixes.)
> 
> That's why _this_ change makes it consistent.
> 
> > doesn't really make sense to me.
  

On 12/1/23 08:17, Andy Shevchenko wrote:
> On Fri, Dec 01, 2023 at 08:01:28AM -0800, Guenter Roeck wrote:
>> On 12/1/23 07:14, Andy Shevchenko wrote:
>>> dev_err_probe() is missing in the list of Log Functions and hence
>>> checkpatch issues a warning in the cases when any other function
>>> in use won't trigger it. Add dev_err_probe() to the list to behave
>>> consistently.
> 
> ...
> 
>> Not sure if I agree. The difference here is that dev_err_probe()
>> has two additional parameters ahead of the string. I would very much prefer
>> to have those two additional parameters on a separate line if the string is
>> too long to fit in 100 columns with those two parameters on the same line.
>> In other words, I very much prefer
>>
>> 	dev_err_probe(dev, -ESOMETHING,
>> 		      "very long string");
>> over
>> 	dev_err_probe(dev, -ESOMETHING, "very long string");
>>
>> and I don't really think that the latter has any benefits.
>>
>> Also note that other dev_xxx() log functions are not included in the above test
>> and would still generate warnings. Accepting
>>
>> 	dev_err_probe(dev, -ESOMETHING, "very long string");
>> but not
>> 	dev_err(dev, "very long string");
> 
> They are included, see the line previous to the added one.
> (Regexp covers something like x_y_()* and x_*() families with the explicitly
>   listed * suffixes.)
> 
> That's why _this_ change makes it consistent.
> 

Hmm ok. Still don't like it.

Guenter
  

On Fri, Dec 01, 2023 at 08:34:14AM -0800, Guenter Roeck wrote:
> On 12/1/23 08:17, Andy Shevchenko wrote:
> > On Fri, Dec 01, 2023 at 08:01:28AM -0800, Guenter Roeck wrote:
> > > On 12/1/23 07:14, Andy Shevchenko wrote:
> > > > dev_err_probe() is missing in the list of Log Functions and hence
> > > > checkpatch issues a warning in the cases when any other function
> > > > in use won't trigger it. Add dev_err_probe() to the list to behave
> > > > consistently.

...

> > > Not sure if I agree. The difference here is that dev_err_probe()
> > > has two additional parameters ahead of the string. I would very much prefer
> > > to have those two additional parameters on a separate line if the string is
> > > too long to fit in 100 columns with those two parameters on the same line.
> > > In other words, I very much prefer
> > > 
> > > 	dev_err_probe(dev, -ESOMETHING,
> > > 		      "very long string");
> > > over
> > > 	dev_err_probe(dev, -ESOMETHING, "very long string");
> > > 
> > > and I don't really think that the latter has any benefits.
> > > 
> > > Also note that other dev_xxx() log functions are not included in the above test
> > > and would still generate warnings. Accepting
> > > 
> > > 	dev_err_probe(dev, -ESOMETHING, "very long string");
> > > but not
> > > 	dev_err(dev, "very long string");
> > 
> > They are included, see the line previous to the added one.
> > (Regexp covers something like x_y_()* and x_*() families with the explicitly
> >   listed * suffixes.)
> > 
> > That's why _this_ change makes it consistent.
> > 
> 
> Hmm ok. Still don't like it.

But then it's orthogonal to the change as with consistent behaviour you may
propose a fix that makes sure that long string literal goes to a separate line
(after a threshold) for _all_ of them at once. Currently the behaviour is
inconsistent independently on somebody's preferences...
  

On Fri, 2023-12-01 at 19:07 +0200, Andy Shevchenko wrote:
> Currently the [style] behaviour is
> inconsistent independently on somebody's preferences...

<shrug>  Which is IMO perfectly fine.

The ratio of multiple line and single line uses
of dev_err_probe is ~50:50

$ git grep -w dev_err_probe | grep -P ';\s*$' | wc -l
1532
$ git grep -w dev_err_probe | grep -P ',\s*$' | wc -l
1871
  

On Fri, Dec 01, 2023 at 02:29:57PM -0800, Joe Perches wrote:
> On Fri, 2023-12-01 at 19:07 +0200, Andy Shevchenko wrote:
> > Currently the [style] behaviour is
> > inconsistent independently on somebody's preferences...
> 
> <shrug>  Which is IMO perfectly fine.
> 
> The ratio of multiple line and single line uses
> of dev_err_probe is ~50:50
> 
> $ git grep -w dev_err_probe | grep -P ';\s*$' | wc -l
> 1532
> $ git grep -w dev_err_probe | grep -P ',\s*$' | wc -l
> 1871

My point is that checkpatch won't warn on dev_err(..., "very long line\n");
while doing the same on dev_err_probe() even if the line is shorter than in
dev_err() case. This inconsistency I am talking about.