[PATCH 1/2] hp-wmi: fix unregister order in hp_wmi_rfkill_setup() once again

[PATCH 1/2] hp-wmi: fix unregister order in hp_wmi_rfkill_setup() once again

[Maciej S. Szmigiero]

rfkill registration order in hp_wmi_rfkill_setup() is:
1) WiFi,
2) BT,
3) WWAN,
5) GPS.

Unregistration when cleaning up on error return should happen
in reverse order.

This means that:
If BT rfkill fails to be allocated we possibly need to
first unregister WiFi rfkill before destroying it.

The same goes with (WWAN, BT) and (GPS, WWAN) pairs.

Also, if WWAN rfkill fails to register we need to (possibly)
unregister BT not the GPS one.
And if GPS rfkill fails to register we need to unregister WWAN
not the BT one.

We never need to unregister GPS rfkill here since if GPS rfkill
registration succeeds this function returns without error so no
cleanup is necessary.

Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
---
 drivers/platform/x86/hp-wmi.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index fb4dd7b3ee71..78cebc0e358c 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -746,7 +746,7 @@ static int __init hp_wmi_rfkill_setup(struct platform_device *device)
 						(void *) HPWMI_BLUETOOTH);
 		if (!bluetooth_rfkill) {
 			err = -ENOMEM;
-			goto register_wifi_error;
+			goto register_bluetooth_error;
 		}
 		rfkill_init_sw_state(bluetooth_rfkill,
 				     hp_wmi_get_sw_state(HPWMI_BLUETOOTH));
@@ -764,7 +764,7 @@ static int __init hp_wmi_rfkill_setup(struct platform_device *device)
 					   (void *) HPWMI_WWAN);
 		if (!wwan_rfkill) {
 			err = -ENOMEM;
-			goto register_bluetooth_error;
+			goto register_wwan_error;
 		}
 		rfkill_init_sw_state(wwan_rfkill,
 				     hp_wmi_get_sw_state(HPWMI_WWAN));
@@ -782,7 +782,7 @@ static int __init hp_wmi_rfkill_setup(struct platform_device *device)
 						(void *) HPWMI_GPS);
 		if (!gps_rfkill) {
 			err = -ENOMEM;
-			goto register_wwan_error;
+			goto register_gps_error;
 		}
 		rfkill_init_sw_state(gps_rfkill,
 				     hp_wmi_get_sw_state(HPWMI_GPS));
@@ -797,13 +797,13 @@ static int __init hp_wmi_rfkill_setup(struct platform_device *device)
 register_gps_error:
 	rfkill_destroy(gps_rfkill);
 	gps_rfkill = NULL;
-	if (bluetooth_rfkill)
-		rfkill_unregister(bluetooth_rfkill);
+	if (wwan_rfkill)
+		rfkill_unregister(wwan_rfkill);
 register_wwan_error:
 	rfkill_destroy(wwan_rfkill);
 	wwan_rfkill = NULL;
-	if (gps_rfkill)
-		rfkill_unregister(gps_rfkill);
+	if (bluetooth_rfkill)
+		rfkill_unregister(bluetooth_rfkill);
 register_bluetooth_error:
 	rfkill_destroy(bluetooth_rfkill);
 	bluetooth_rfkill = NULL;

Re: [PATCH 1/2] hp-wmi: fix unregister order in hp_wmi_rfkill_setup() once again

[Darren Hart]

On Sun, Mar 06, 2016 at 11:38:36PM +0100, Maciej S. Szmigiero wrote:
> rfkill registration order in hp_wmi_rfkill_setup() is:
> 1) WiFi,
> 2) BT,
> 3) WWAN,
> 5) GPS.
> 
> Unregistration when cleaning up on error return should happen
> in reverse order.
> 
> This means that:
> If BT rfkill fails to be allocated we possibly need to
> first unregister WiFi rfkill before destroying it.
> 
> The same goes with (WWAN, BT) and (GPS, WWAN) pairs.
> 
> Also, if WWAN rfkill fails to register we need to (possibly)
> unregister BT not the GPS one.
> And if GPS rfkill fails to register we need to unregister WWAN
> not the BT one.
> 
> We never need to unregister GPS rfkill here since if GPS rfkill
> registration succeeds this function returns without error so no
> cleanup is necessary.

Thank you for clearly articulating the problem.

> 
> Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> ---
>  drivers/platform/x86/hp-wmi.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> index fb4dd7b3ee71..78cebc0e358c 100644
> --- a/drivers/platform/x86/hp-wmi.c
> +++ b/drivers/platform/x86/hp-wmi.c
> @@ -746,7 +746,7 @@ static int __init hp_wmi_rfkill_setup(struct platform_device *device)
>  						(void *) HPWMI_BLUETOOTH);
>  		if (!bluetooth_rfkill) {
>  			err = -ENOMEM;
> -			goto register_wifi_error;
> +			goto register_bluetooth_error;

In this and all cases below, the goto label should match the situation, jumping
to register_bluetooth_error would be incorrect as we experienced a wifi error. A
better solution would be to reorder the labels in the exit block such that they
enforce the necessary reverse order.

>  		}
>  		rfkill_init_sw_state(bluetooth_rfkill,
>  				     hp_wmi_get_sw_state(HPWMI_BLUETOOTH));
> @@ -764,7 +764,7 @@ static int __init hp_wmi_rfkill_setup(struct platform_device *device)
>  					   (void *) HPWMI_WWAN);
>  		if (!wwan_rfkill) {
>  			err = -ENOMEM;
> -			goto register_bluetooth_error;
> +			goto register_wwan_error;
>  		}
>  		rfkill_init_sw_state(wwan_rfkill,
>  				     hp_wmi_get_sw_state(HPWMI_WWAN));
> @@ -782,7 +782,7 @@ static int __init hp_wmi_rfkill_setup(struct platform_device *device)
>  						(void *) HPWMI_GPS);
>  		if (!gps_rfkill) {
>  			err = -ENOMEM;
> -			goto register_wwan_error;
> +			goto register_gps_error;
>  		}
>  		rfkill_init_sw_state(gps_rfkill,
>  				     hp_wmi_get_sw_state(HPWMI_GPS));
> @@ -797,13 +797,13 @@ static int __init hp_wmi_rfkill_setup(struct platform_device *device)
>  register_gps_error:
>  	rfkill_destroy(gps_rfkill);
>  	gps_rfkill = NULL;
> -	if (bluetooth_rfkill)
> -		rfkill_unregister(bluetooth_rfkill);
> +	if (wwan_rfkill)
> +		rfkill_unregister(wwan_rfkill);
>  register_wwan_error:
>  	rfkill_destroy(wwan_rfkill);
>  	wwan_rfkill = NULL;
> -	if (gps_rfkill)
> -		rfkill_unregister(gps_rfkill);
> +	if (bluetooth_rfkill)
> +		rfkill_unregister(bluetooth_rfkill);
>  register_bluetooth_error:
>  	rfkill_destroy(bluetooth_rfkill);
>  	bluetooth_rfkill = NULL;
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 1/2] hp-wmi: fix unregister order in hp_wmi_rfkill_setup() once again

[Maciej S. Szmigiero]

Hi Darren,

Thanks for review, see also my comments below.

On 08.03.2016 13:39, Darren Hart wrote:
> On Sun, Mar 06, 2016 at 11:38:36PM +0100, Maciej S. Szmigiero wrote:
>> --- a/drivers/platform/x86/hp-wmi.c
>> +++ b/drivers/platform/x86/hp-wmi.c
>> @@ -746,7 +746,7 @@ static int __init hp_wmi_rfkill_setup(struct platform_device *device)
>>  						(void *) HPWMI_BLUETOOTH);
>>  		if (!bluetooth_rfkill) {
>>  			err = -ENOMEM;
>> -			goto register_wifi_error;
>> +			goto register_bluetooth_error;
> 
> In this and all cases below, the goto label should match the situation, jumping
> to register_bluetooth_error would be incorrect as we experienced a wifi error. 

Here we experienced an BT error - BT rkill allocation failed,
so jump is to "register_bluetooth_error".

The second jump to "register_bluetooth_error" is in another case of BT error:
when its rfkill registration failed.

It is the same label since if BT rfkill allocation had failed
rfkill_destroy(bluetooth_rfkill) call in cleanup does nothing but we still
need to possibly unregister WiFi rfkill that might have been registered in
a previews block (and then fall through to next label to destroy WiFi rfkill).

It would be possible to have separate jump labels skipping unnecessary
rfkill_destroy() calls on allocation failure, but this would mean
that we would have 7 labels in such small block of cleanup code.

> A better solution would be to reorder the labels in the exit block
> such that they enforce the necessary reverse order.

Cleanup labels already are in reverse order with regard to registration:
Registration:
1) WiFi,
2) BT,
3) WWAN,
5) GPS.

Cleanup:
1) GPS,
2) WWAN,
3) BT,
4) WiFi.

Best regards,
Maciej Szmigiero

Re: [PATCH 1/2] hp-wmi: fix unregister order in hp_wmi_rfkill_setup() once again

[Darren Hart]

On Tue, Mar 08, 2016 at 04:58:40PM +0100, Maciej S. Szmigiero wrote:
> Hi Darren,
> 
> Thanks for review, see also my comments below.
> 
> On 08.03.2016 13:39, Darren Hart wrote:
> > On Sun, Mar 06, 2016 at 11:38:36PM +0100, Maciej S. Szmigiero wrote:
> >> --- a/drivers/platform/x86/hp-wmi.c
> >> +++ b/drivers/platform/x86/hp-wmi.c
> >> @@ -746,7 +746,7 @@ static int __init hp_wmi_rfkill_setup(struct platform_device *device)
> >>  						(void *) HPWMI_BLUETOOTH);
> >>  		if (!bluetooth_rfkill) {
> >>  			err = -ENOMEM;
> >> -			goto register_wifi_error;
> >> +			goto register_bluetooth_error;
> > 
> > In this and all cases below, the goto label should match the situation, jumping
> > to register_bluetooth_error would be incorrect as we experienced a wifi error. 
> 
> Here we experienced an BT error - BT rkill allocation failed,
> so jump is to "register_bluetooth_error".
> 
> The second jump to "register_bluetooth_error" is in another case of BT error:
> when its rfkill registration failed.
> 
> It is the same label since if BT rfkill allocation had failed
> rfkill_destroy(bluetooth_rfkill) call in cleanup does nothing but we still
> need to possibly unregister WiFi rfkill that might have been registered in
> a previews block (and then fall through to next label to destroy WiFi rfkill).
> 
> It would be possible to have separate jump labels skipping unnecessary
> rfkill_destroy() calls on allocation failure, but this would mean
> that we would have 7 labels in such small block of cleanup code.

My apologies, I read through this too quickly. The patch is correct as is.
I have wrapped the commit message at 75 characters and queued to the testing
branch.

The one thing that's missing in the commit message is a description of how this
was failing. It can, however, be inferred.

Thanks for the patient reply, I deserved a slap for my initial response.

-- 
Darren Hart
Intel Open Source Technology Center