[PATCH v4 08/15] drivers/firmware/sdei: Remove while loop in sdei_event_register()

[James Morse]

Hi Gavin,

On 30/07/2020 02:45, Gavin Shan wrote:
> This removes the unnecessary while loop in sdei_event_register().

Did you notice how this code doesn't need to unwind anything to clean up?
It only unlocks the mutex, and the indentation tells you it always does that.


> This shouldn't cause any functional changes.

Why is it better? This complicates the flow by jumping around with goto.

If the unwinding were necessary, I agree this would be clearer, but as its not ... why do
we need to make this harder to read?


Thanks,

James


> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index 03b1179da9b4..d04329f98922 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -590,36 +590,34 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>  	WARN_ON(in_nmi());
>  
>  	mutex_lock(&sdei_events_lock);
> -	do {
> -		if (sdei_event_find(event_num)) {
> -			pr_warn("Event %u already registered\n", event_num);
> -			err = -EBUSY;
> -			break;
> -		}
> +	if (sdei_event_find(event_num)) {
> +		pr_warn("Event %u already registered\n", event_num);
> +		err = -EBUSY;
> +		goto unlock;
> +	}
>  
> -		event = sdei_event_create(event_num, cb, arg);
> -		if (IS_ERR(event)) {
> -			err = PTR_ERR(event);
> -			pr_warn("Failed to create event %u: %d\n", event_num,
> -				err);
> -			break;
> -		}
> +	event = sdei_event_create(event_num, cb, arg);
> +	if (IS_ERR(event)) {
> +		err = PTR_ERR(event);
> +		pr_warn("Failed to create event %u: %d\n", event_num, err);
> +		goto unlock;
> +	}
>  
> -		cpus_read_lock();
> -		err = _sdei_event_register(event);
> -		if (err) {
> -			sdei_event_destroy(event);
> -			pr_warn("Failed to register event %u: %d\n", event_num,
> -				err);
> -		} else {
> -			spin_lock(&sdei_list_lock);
> -			event->reregister = true;
> -			spin_unlock(&sdei_list_lock);
> -		}
> -		cpus_read_unlock();
> -	} while (0);
> -	mutex_unlock(&sdei_events_lock);
> +	cpus_read_lock();
> +	err = _sdei_event_register(event);
> +	if (err) {
> +		sdei_event_destroy(event);
> +		pr_warn("Failed to register event %u: %d\n", event_num, err);
> +		goto cpu_unlock;
> +	}
>  
> +	spin_lock(&sdei_list_lock);
> +	event->reregister = true;
> +	spin_unlock(&sdei_list_lock);
> +cpu_unlock:
> +	cpus_read_unlock();
> +unlock:
> +	mutex_unlock(&sdei_events_lock);
>  	return err;
>  }
>  
>