| Message ID | 56DCB16C.80608@maciej.szmigiero.name (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
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; >
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 -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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;
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(-) -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html