Re: [PATCH 1/5] ath10k: clean up start() callback

[Kalle Valo <kvalo@qca.qualcomm.com>]

Michal Kazior <michal.kazior@tieto.com> writes:

> This fixes failpath when override AC pdev param
> setup fails and makes other pdev params setting
> fail as well.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

[...]

>  	ar->num_started_vdevs = 0;
>  	ath10k_regd_update(ar);
>  	ret = 0;
> +	goto out;
>  
> -exit:
> +err_core_stop:
> +	ath10k_core_stop(ar);
> +err_power_down:
> +	ath10k_hif_power_down(ar);
> +err_off:
> +	ar->state = ATH10K_STATE_OFF;
> +out:
>  	mutex_unlock(&ar->conf_mutex);
>  	return ret;
>  }

Having err_ labels on the "main" code path is not good, the error
handling code should be clearly separated to make it easier to read. I
think you should use the same style as pci.c uses, for example something
like this:
       
 	mutex_unlock(&ar->conf_mutex);
 	return 0;
 
err_core_stop:
	ath10k_core_stop(ar);

err_power_down:
	ath10k_hif_power_down(ar);

err_off:
	ar->state = ATH10K_STATE_OFF;

 	mutex_unlock(&ar->conf_mutex);

 	return ret;

I know this has mutex_unlock() so it's not good either, but I think it's
still better. Other option might be to do like this:

        ret = 0;

out:
 	mutex_unlock(&ar->conf_mutex);
 	return ret;
 
err_core_stop:
	ath10k_core_stop(ar);

err_power_down:
	ath10k_hif_power_down(ar);

err_off:
	ar->state = ATH10K_STATE_OFF;

        goto out;

But not sure if it's any better. More ideas?

Oh yeah, please also add an empty line before each label.

-- 
Kalle Valo