[PATCH 0/1] Use checkpatch.pl to make thinkpad_acpi.c error free: octal permissions

[PATCH 0/1] Use checkpatch.pl to make thinkpad_acpi.c error free: octal permissions

[Simranjit Singh]

From: Simranjit Singh <simranjit.singh@corebaby.org>

Using the checkpatch.pl script, there were 8 errors in thinkpad_acpi.c. I fixed them by changing permissions to octal,
and by adding parenthesis. 
On the current tree, if you use checkpatch.pl, thinkpad_acpi.c will have 8 errors. With my changes, there are 0 errors.
However, I have added 3 new warnings. 
I'm very new to kernel development, so if I messed up somewhere, please do tell me.
    
Changed binary permissions to octal in accordance with checkpatch.pl in thinkpad_acpi.c

Signed-off-by: Simranjit Singh <simranjit.singh@corebaby.org>
---
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 2242d60..9ad1f5b 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -9574,7 +9574,7 @@ static int __init set_ibm_param(const char *val, struct kernel_param *kp)
 MODULE_PARM_DESC(experimental,
 		 "Enables experimental features when non-zero");
 
-module_param_named(debug, dbg_level, uint, 0);
+module_param_named(debug, dbg_level, uint, 0444;
 MODULE_PARM_DESC(debug, "Sets debug level bit-mask");
 
 module_param(force_load, bool, 0444);
@@ -9638,25 +9638,25 @@ static int __init set_ibm_param(const char *val, struct kernel_param *kp)
 #ifdef CONFIG_THINKPAD_ACPI_DEBUGFACILITIES
 module_param(dbg_wlswemul, uint, 0444);
 MODULE_PARM_DESC(dbg_wlswemul, "Enables WLSW emulation");
-module_param_named(wlsw_state, tpacpi_wlsw_emulstate, bool, 0);
+module_param_named(wlsw_state, tpacpi_wlsw_emulstate, 044);
 MODULE_PARM_DESC(wlsw_state,
 		 "Initial state of the emulated WLSW switch");
 
 module_param(dbg_bluetoothemul, uint, 0444);
 MODULE_PARM_DESC(dbg_bluetoothemul, "Enables bluetooth switch emulation");
-module_param_named(bluetooth_state, tpacpi_bluetooth_emulstate, bool, 0);
+module_param_named(bluetooth_state, tpacpi_bluetooth_emulstate, 0444);
 MODULE_PARM_DESC(bluetooth_state,
 		 "Initial state of the emulated bluetooth switch");
 
 module_param(dbg_wwanemul, uint, 0444);
 MODULE_PARM_DESC(dbg_wwanemul, "Enables WWAN switch emulation");
-module_param_named(wwan_state, tpacpi_wwan_emulstate, bool, 0);
+module_param_named(wwan_state, tpacpi_wwan_emulstate, 0444);
 MODULE_PARM_DESC(wwan_state,
 		 "Initial state of the emulated WWAN switch");
 
 module_param(dbg_uwbemul, uint, 0444);
 MODULE_PARM_DESC(dbg_uwbemul, "Enables UWB switch emulation");
-module_param_named(uwb_state, tpacpi_uwb_emulstate, bool, 0);
+module_param_named(uwb_state, tpacpi_uwb_emulstate, 0444);
 MODULE_PARM_DESC(uwb_state,
 		 "Initial state of the emulated UWB switch");
 #endif

Re: [PATCH 0/1] Use checkpatch.pl to make thinkpad_acpi.c error free: octal permissions

[Randy Dunlap]

On 11/14/2017 03:14 PM, Simranjit Singh wrote:
> From: Simranjit Singh <simranjit.singh@corebaby.org>
> 
> Using the checkpatch.pl script, there were 8 errors in thinkpad_acpi.c. I fixed them by changing permissions to octal,
> and by adding parenthesis. 
> On the current tree, if you use checkpatch.pl, thinkpad_acpi.c will have 8 errors. With my changes, there are 0 errors.
> However, I have added 3 new warnings. 
> I'm very new to kernel development, so if I messed up somewhere, please do tell me.
>     
> Changed binary permissions to octal in accordance with checkpatch.pl in thinkpad_acpi.c
> 
> Signed-off-by: Simranjit Singh <simranjit.singh@corebaby.org>

a. You should have [PATCH 1/2] and [PATCH 2/2].

b. Subject should be something like [PATCH 1/2] thinkpad_acpi: fix octal permissions
(if they need to be fixed)

> ---
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 2242d60..9ad1f5b 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9574,7 +9574,7 @@ static int __init set_ibm_param(const char *val, struct kernel_param *kp)
>  MODULE_PARM_DESC(experimental,
>  		 "Enables experimental features when non-zero");
>  
> -module_param_named(debug, dbg_level, uint, 0);

Is 0 not octal?

> +module_param_named(debug, dbg_level, uint, 0444;

missing closing ')'.  So you did not compile this, right?  boo.

>  MODULE_PARM_DESC(debug, "Sets debug level bit-mask");
>  
>  module_param(force_load, bool, 0444);
> @@ -9638,25 +9638,25 @@ static int __init set_ibm_param(const char *val, struct kernel_param *kp)
>  #ifdef CONFIG_THINKPAD_ACPI_DEBUGFACILITIES
>  module_param(dbg_wlswemul, uint, 0444);
>  MODULE_PARM_DESC(dbg_wlswemul, "Enables WLSW emulation");
> -module_param_named(wlsw_state, tpacpi_wlsw_emulstate, bool, 0);
> +module_param_named(wlsw_state, tpacpi_wlsw_emulstate, 044);

module_param_named() still takes 4 parameters.

>  MODULE_PARM_DESC(wlsw_state,
>  		 "Initial state of the emulated WLSW switch");
>  
>  module_param(dbg_bluetoothemul, uint, 0444);
>  MODULE_PARM_DESC(dbg_bluetoothemul, "Enables bluetooth switch emulation");
> -module_param_named(bluetooth_state, tpacpi_bluetooth_emulstate, bool, 0);
> +module_param_named(bluetooth_state, tpacpi_bluetooth_emulstate, 0444);
'
ditto.

>  MODULE_PARM_DESC(bluetooth_state,
>  		 "Initial state of the emulated bluetooth switch");
>  
>  module_param(dbg_wwanemul, uint, 0444);
>  MODULE_PARM_DESC(dbg_wwanemul, "Enables WWAN switch emulation");
> -module_param_named(wwan_state, tpacpi_wwan_emulstate, bool, 0);
> +module_param_named(wwan_state, tpacpi_wwan_emulstate, 0444);

ditto.

>  MODULE_PARM_DESC(wwan_state,
>  		 "Initial state of the emulated WWAN switch");
>  
>  module_param(dbg_uwbemul, uint, 0444);
>  MODULE_PARM_DESC(dbg_uwbemul, "Enables UWB switch emulation");
> -module_param_named(uwb_state, tpacpi_uwb_emulstate, bool, 0);
> +module_param_named(uwb_state, tpacpi_uwb_emulstate, 0444);

ditto.

>  MODULE_PARM_DESC(uwb_state,
>  		 "Initial state of the emulated UWB switch");
>  #endif
> 
> 
> 


-- 
~Randy

Re: [PATCH 0/1] Use checkpatch.pl to make thinkpad_acpi.c error free: octal permissions

[Andy Shevchenko]

On Wed, Nov 15, 2017 at 1:14 AM, Simranjit Singh
<simranjit.singh@corebaby.org> wrote:
> From: Simranjit Singh <simranjit.singh@corebaby.org>
>
> Using the checkpatch.pl script, there were 8 errors in thinkpad_acpi.c. I fixed them by changing permissions to octal,
> and by adding parenthesis.
> On the current tree, if you use checkpatch.pl, thinkpad_acpi.c will have 8 errors. With my changes, there are 0 errors.
> However, I have added 3 new warnings.
> I'm very new to kernel development, so if I messed up somewhere, please do tell me.
>
> Changed binary permissions to octal in accordance with checkpatch.pl in thinkpad_acpi.c

Besides comments you got already, keep in mind that patch numbering
starts from 1, 0 is a special dedicated value for cover letter which
you apparently miss.
Of course, in case of this "series" you are not supposed to make it as a series.

And one more time, it's a contributor's responsibility to test (*)
changes before sending _by default _.

*) sometimes compilation test is enough, though most of the times the
real run would be required.

As per change 0 to 0444 for the cases you did, you obviously didn't
read the code and thus may not understand the ABI of this module.

So, obvious NAK for this patch.

-- 
With Best Regards,
Andy Shevchenko