[PATCH] checkpatch: add new warning when lookup_symbol_name() is used

[PATCH] checkpatch: add new warning when lookup_symbol_name() is used

[Helge Deller]

In most cases people use lookup_symbol_name() to resolve a kernel symbol
and then print it via printk().

In such cases using the %ps, %pS, %pSR or %pB printk formats are easier
to use and thus should be preferred.

Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index fab38b493cef..0d5515a3d875 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4317,6 +4317,12 @@ sub process {
 			     "LINUX_VERSION_CODE should be avoided, code should be for the version to which it is merged\n" . $herecurr);
 		}

+# avoid lookup_symbol_name()
+		if ($line =~ /\blookup_symbol_name\b/) {
+			WARN("PREFER_PRINTK_FORMAT",
+			     "If possible prefer %ps or %pS printk format string to print symbol name instead of using lookup_symbol_name()\n" . $herecurr);
+		}
+
 # check for uses of printk_ratelimit
 		if ($line =~ /\bprintk_ratelimit\s*\(/) {
 			WARN("PRINTK_RATELIMITED",

Re: [PATCH] checkpatch: add new warning when lookup_symbol_name() is used

[Joe Perches]

On Thu, 2020-12-17 at 18:11 +0100, Helge Deller wrote:
> In most cases people use lookup_symbol_name() to resolve a kernel symbol
> and then print it via printk().
> 
> In such cases using the %ps, %pS, %pSR or %pB printk formats are easier
> to use and thus should be preferred.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -4317,6 +4317,12 @@ sub process {
>  			     "LINUX_VERSION_CODE should be avoided, code should be for the version to which it is merged\n" . $herecurr);
>  		}
> 
> +# avoid lookup_symbol_name()
> +		if ($line =~ /\blookup_symbol_name\b/) {
> +			WARN("PREFER_PRINTK_FORMAT",
> +			     "If possible prefer %ps or %pS printk format string to print symbol name instead of using lookup_symbol_name()\n" . $herecurr);
> +		}
> +
>  # check for uses of printk_ratelimit
>  		if ($line =~ /\bprintk_ratelimit\s*\(/) {
>  			WARN("PRINTK_RATELIMITED",

Huh?  nak.

lookup_symbol_name is used in the kernel a grand total of 3 times.
2 uses are kprobe, the other is fs/proc

None of the existing uses is equivalent to %ps

Why should this be applied?

Re: [PATCH] checkpatch: add new warning when lookup_symbol_name() is used

[Helge Deller]

On 12/17/20 6:27 PM, Joe Perches wrote:
> On Thu, 2020-12-17 at 18:11 +0100, Helge Deller wrote:
>> In most cases people use lookup_symbol_name() to resolve a kernel symbol
>> and then print it via printk().
>>
>> In such cases using the %ps, %pS, %pSR or %pB printk formats are easier
>> to use and thus should be preferred.
> []
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
>> @@ -4317,6 +4317,12 @@ sub process {
>>  			     "LINUX_VERSION_CODE should be avoided, code should be for the version to which it is merged\n" . $herecurr);
>>  		}
>>
>> +# avoid lookup_symbol_name()
>> +		if ($line =~ /\blookup_symbol_name\b/) {
>> +			WARN("PREFER_PRINTK_FORMAT",
>> +			     "If possible prefer %ps or %pS printk format string to print symbol name instead of using lookup_symbol_name()\n" . $herecurr);
>> +		}
>> +
>>  # check for uses of printk_ratelimit
>>  		if ($line =~ /\bprintk_ratelimit\s*\(/) {
>>  			WARN("PRINTK_RATELIMITED",
>
> Huh?  nak.
>
> lookup_symbol_name is used in the kernel a grand total of 3 times.

Yes, there were much more in the past which got fixed by patches I submitted.

> 2 uses are kprobe, the other is fs/proc

Right. For fs/proc see:
https://lore.kernel.org/lkml/20201217165413.GA1959@ls3530.fritz.box/

> None of the existing uses is equivalent to %ps

Yes, those are the remaining legimate users.

> Why should this be applied?

... to prevent people to add new code with possibly unjustified use?

Helge

Re: [PATCH] checkpatch: add new warning when lookup_symbol_name() is used

[Joe Perches]

On Thu, 2020-12-17 at 18:42 +0100, Helge Deller wrote:
> On 12/17/20 6:27 PM, Joe Perches wrote:
> > On Thu, 2020-12-17 at 18:11 +0100, Helge Deller wrote:
> > > In most cases people use lookup_symbol_name() to resolve a kernel symbol
> > > and then print it via printk().
> > > 
> > > In such cases using the %ps, %pS, %pSR or %pB printk formats are easier
> > > to use and thus should be preferred.
> > []
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > []
> > > @@ -4317,6 +4317,12 @@ sub process {
> > >  			     "LINUX_VERSION_CODE should be avoided, code should be for the version to which it is merged\n" . $herecurr);
> > >  		}
> > > 
> > > +# avoid lookup_symbol_name()
> > > +		if ($line =~ /\blookup_symbol_name\b/) {
> > > +			WARN("PREFER_PRINTK_FORMAT",
> > > +			     "If possible prefer %ps or %pS printk format string to print symbol name instead of using lookup_symbol_name()\n" . $herecurr);
> > > +		}
> > > +
> > >  # check for uses of printk_ratelimit
> > >  		if ($line =~ /\bprintk_ratelimit\s*\(/) {
> > >  			WARN("PRINTK_RATELIMITED",
> > 
> > Huh?  nak.
> > 
> > lookup_symbol_name is used in the kernel a grand total of 3 times.
> 
> Yes, there were much more in the past which got fixed by patches I submitted.

Hi Helge.

Much more may be a bit of an overstatement.

I found 3 instances of lookup_symbol_name removals in 2 patches.

commit 36dbca148bf8e3b8658982aa2256bdc7ef040256
-		lookup_symbol_name((ulong)pm_power_off, symname);
-		lookup_symbol_name((ulong)pm_power_off, symname);
commit da88f9b3113620dcd30fc203236aa53d5430ee98
-	if (lookup_symbol_name((unsigned long)sym, symname) < 0)

There's a tension between adding tests and newbies that consider
checkpatch warnings as dicta that must be followed so there would
be patches submitted eventually against the existing correct uses.

So thanks, but given the very few existing all correct uses of
this function and the low probability of new uses I'd prefer not
to apply this.

Re: [PATCH] checkpatch: add new warning when lookup_symbol_name() is used

[Helge Deller]

On 12/17/20 7:15 PM, Joe Perches wrote:
> On Thu, 2020-12-17 at 18:42 +0100, Helge Deller wrote:
>> On 12/17/20 6:27 PM, Joe Perches wrote:
>>> On Thu, 2020-12-17 at 18:11 +0100, Helge Deller wrote:
>>>> In most cases people use lookup_symbol_name() to resolve a kernel symbol
>>>> and then print it via printk().
>>>>
>>>> In such cases using the %ps, %pS, %pSR or %pB printk formats are easier
>>>> to use and thus should be preferred.
>>> []
>>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>> []
>>>> @@ -4317,6 +4317,12 @@ sub process {
>>>>  			     "LINUX_VERSION_CODE should be avoided, code should be for the version to which it is merged\n" . $herecurr);
>>>>  		}
>>>>
>>>> +# avoid lookup_symbol_name()
>>>> +		if ($line =~ /\blookup_symbol_name\b/) {
>>>> +			WARN("PREFER_PRINTK_FORMAT",
>>>> +			     "If possible prefer %ps or %pS printk format string to print symbol name instead of using lookup_symbol_name()\n" . $herecurr);
>>>> +		}
>>>> +
>>>>  # check for uses of printk_ratelimit
>>>>  		if ($line =~ /\bprintk_ratelimit\s*\(/) {
>>>>  			WARN("PRINTK_RATELIMITED",
>>>
>>> Huh?  nak.
>>>
>>> lookup_symbol_name is used in the kernel a grand total of 3 times.
>>
>> Yes, there were much more in the past which got fixed by patches I submitted.
>
> Hi Helge.
>
> Much more may be a bit of an overstatement.
>
> I found 3 instances of lookup_symbol_name removals in 2 patches.
>
> commit 36dbca148bf8e3b8658982aa2256bdc7ef040256
> -		lookup_symbol_name((ulong)pm_power_off, symname);
> -		lookup_symbol_name((ulong)pm_power_off, symname);
> commit da88f9b3113620dcd30fc203236aa53d5430ee98
> -	if (lookup_symbol_name((unsigned long)sym, symname) < 0)
>
> There's a tension between adding tests and newbies that consider
> checkpatch warnings as dicta that must be followed so there would
> be patches submitted eventually against the existing correct uses.
>
> So thanks, but given the very few existing all correct uses of
> this function and the low probability of new uses I'd prefer not
> to apply this.

Ok.

Thanks!
Helge