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

[Gavin Shan]

Hi James,

On 9/19/20 2:13 AM, James Morse wrote:
> 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.
> 

Yes, I noticed it.

> 
>> 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?
> 

I intended to avoid the unnecessary nested statements, caused
by the "do { ... } while (0)". With that, the code looks a bit
cleaner. It might make the code a bit harder to read, but it's
fine to me. Besides, the unnecessary "do { ... } while (0)" looks
a bit strange to me because the block is executed for once.

So I think it'd better to keep the changes if you don't object
strongly. Otherwise, I can drop this one.

Cheers,
Gavin

> 
>> 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;
>>   }
>>   
>>
>