[v2,12/17] drivers/firmware/sdei: Remove _sdei_event_register()
diff mbox series

Message ID 20200722095740.28560-13-gshan@redhat.com
State New
Headers show
Series
  • Refactor SDEI client driver
Related show

Commit Message

Gavin Shan July 22, 2020, 9:57 a.m. UTC
The function _sdei_event_register() is called by sdei_event_register()
and sdei_device_thaw() as the following functional call chain shows.
_sdei_event_register() covers the shared and privte events, but
sdei_device_thaw() only covers the shared events. So the logic to
cover the private events in _sdei_event_register() isn't needed by
sdei_device_thaw().

Similarly, sdei_reregister_event_llocked() covers the shared and
private events in the regard of reenablement. The logic to cover
the private events isn't needed by sdei_device_thaw() either.

   sdei_event_register          sdei_device_thaw
      _sdei_event_register         sdei_reregister_shared
                                      sdei_reregister_event_llocked
                                         _sdei_event_register

This removes _sdei_event_register() and sdei_reregister_event_llocked().
Their logic is moved to sdei_event_register() and sdei_reregister_shared().
This shouldn't cause any logicial changes.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
v2: Improved commit log
    Drop changes to reorder the variables
---
 drivers/firmware/arm_sdei.c | 77 ++++++++++++++-----------------------
 1 file changed, 28 insertions(+), 49 deletions(-)

Comments

Jonathan Cameron July 23, 2020, 3:25 p.m. UTC | #1
On Wed, 22 Jul 2020 19:57:35 +1000
Gavin Shan <gshan@redhat.com> wrote:

> The function _sdei_event_register() is called by sdei_event_register()
> and sdei_device_thaw() as the following functional call chain shows.
> _sdei_event_register() covers the shared and privte events, but

private

> sdei_device_thaw() only covers the shared events. So the logic to
> cover the private events in _sdei_event_register() isn't needed by
> sdei_device_thaw().
> 
> Similarly, sdei_reregister_event_llocked() covers the shared and
> private events in the regard of reenablement. The logic to cover
> the private events isn't needed by sdei_device_thaw() either.
> 
>    sdei_event_register          sdei_device_thaw
>       _sdei_event_register         sdei_reregister_shared
>                                       sdei_reregister_event_llocked
>                                          _sdei_event_register
> 
> This removes _sdei_event_register() and sdei_reregister_event_llocked().
> Their logic is moved to sdei_event_register() and sdei_reregister_shared().
> This shouldn't cause any logicial changes.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>

Otherwise looks good to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> v2: Improved commit log
>     Drop changes to reorder the variables
> ---
>  drivers/firmware/arm_sdei.c | 77 ++++++++++++++-----------------------
>  1 file changed, 28 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index da0e0d5591a8..f4cd9791242f 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -575,25 +575,6 @@ static void _local_event_register(void *data)
>  	sdei_cross_call_return(arg, err);
>  }
>  
> -static int _sdei_event_register(struct sdei_event *event)
> -{
> -	int err;
> -
> -	lockdep_assert_held(&sdei_events_lock);
> -
> -	if (event->type == SDEI_EVENT_TYPE_SHARED)
> -		return sdei_api_event_register(event->event_num,
> -					       sdei_entry_point,
> -					       event->registered,
> -					       SDEI_EVENT_REGISTER_RM_ANY, 0);
> -
> -	err = sdei_do_cross_call(_local_event_register, event);
> -	if (err)
> -		sdei_do_cross_call(_local_event_unregister, event);
> -
> -	return err;
> -}
> -
>  int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>  {
>  	int err;
> @@ -616,7 +597,17 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>  	}
>  
>  	cpus_read_lock();
> -	err = _sdei_event_register(event);
> +	if (event->type == SDEI_EVENT_TYPE_SHARED) {
> +		err = sdei_api_event_register(event->event_num,
> +					      sdei_entry_point,
> +					      event->registered,
> +					      SDEI_EVENT_REGISTER_RM_ANY, 0);
> +	} else {
> +		err = sdei_do_cross_call(_local_event_register, event);
> +		if (err)
> +			sdei_do_cross_call(_local_event_unregister, event);
> +	}
> +
>  	if (err) {
>  		sdei_event_destroy(event);
>  		pr_warn("Failed to register event %u: %d\n", event_num, err);
> @@ -633,33 +624,6 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>  	return err;
>  }
>  
> -static int sdei_reregister_event_llocked(struct sdei_event *event)
> -{
> -	int err;
> -
> -	lockdep_assert_held(&sdei_events_lock);
> -	lockdep_assert_held(&sdei_list_lock);
> -
> -	err = _sdei_event_register(event);
> -	if (err) {
> -		pr_err("Failed to re-register event %u\n", event->event_num);
> -		sdei_event_destroy_llocked(event);
> -		return err;
> -	}
> -
> -	if (event->reenable) {
> -		if (event->type == SDEI_EVENT_TYPE_SHARED)
> -			err = sdei_api_event_enable(event->event_num);
> -		else
> -			err = sdei_do_cross_call(_local_event_enable, event);
> -	}
> -
> -	if (err)
> -		pr_err("Failed to re-enable event %u\n", event->event_num);
> -
> -	return err;
> -}
> -
>  static int sdei_reregister_shared(void)
>  {
>  	int err = 0;
> @@ -672,9 +636,24 @@ static int sdei_reregister_shared(void)
>  			continue;
>  
>  		if (event->reregister) {
> -			err = sdei_reregister_event_llocked(event);
> -			if (err)
> +			err = sdei_api_event_register(event->event_num,
> +					sdei_entry_point, event->registered,
> +					SDEI_EVENT_REGISTER_RM_ANY, 0);
> +			if (err) {
> +				sdei_event_destroy_llocked(event);
> +				pr_err("Failed to re-register event %u\n",
> +				       event->event_num);
>  				break;
> +			}
> +		}
> +
> +		if (event->reenable) {
> +			err = sdei_api_event_enable(event->event_num);
> +			if (err) {
> +				pr_err("Failed to re-enable event %u\n",
> +				       event->event_num);
> +				break;
> +			}
>  		}
>  	}
>  	spin_unlock(&sdei_list_lock);
Gavin Shan July 27, 2020, 12:42 a.m. UTC | #2
Hi Jonathan,

On 7/24/20 1:25 AM, Jonathan Cameron wrote:
> On Wed, 22 Jul 2020 19:57:35 +1000
> Gavin Shan <gshan@redhat.com> wrote:
> 
>> The function _sdei_event_register() is called by sdei_event_register()
>> and sdei_device_thaw() as the following functional call chain shows.
>> _sdei_event_register() covers the shared and privte events, but
> 
> private
> 

Will be fixed in v3.

>> sdei_device_thaw() only covers the shared events. So the logic to
>> cover the private events in _sdei_event_register() isn't needed by
>> sdei_device_thaw().
>>
>> Similarly, sdei_reregister_event_llocked() covers the shared and
>> private events in the regard of reenablement. The logic to cover
>> the private events isn't needed by sdei_device_thaw() either.
>>
>>     sdei_event_register          sdei_device_thaw
>>        _sdei_event_register         sdei_reregister_shared
>>                                        sdei_reregister_event_llocked
>>                                           _sdei_event_register
>>
>> This removes _sdei_event_register() and sdei_reregister_event_llocked().
>> Their logic is moved to sdei_event_register() and sdei_reregister_shared().
>> This shouldn't cause any logicial changes.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
> 
> Otherwise looks good to me.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Will be picked up in v3.

>> ---
>> v2: Improved commit log
>>      Drop changes to reorder the variables
>> ---
>>   drivers/firmware/arm_sdei.c | 77 ++++++++++++++-----------------------
>>   1 file changed, 28 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
>> index da0e0d5591a8..f4cd9791242f 100644
>> --- a/drivers/firmware/arm_sdei.c
>> +++ b/drivers/firmware/arm_sdei.c
>> @@ -575,25 +575,6 @@ static void _local_event_register(void *data)
>>   	sdei_cross_call_return(arg, err);
>>   }
>>   
>> -static int _sdei_event_register(struct sdei_event *event)
>> -{
>> -	int err;
>> -
>> -	lockdep_assert_held(&sdei_events_lock);
>> -
>> -	if (event->type == SDEI_EVENT_TYPE_SHARED)
>> -		return sdei_api_event_register(event->event_num,
>> -					       sdei_entry_point,
>> -					       event->registered,
>> -					       SDEI_EVENT_REGISTER_RM_ANY, 0);
>> -
>> -	err = sdei_do_cross_call(_local_event_register, event);
>> -	if (err)
>> -		sdei_do_cross_call(_local_event_unregister, event);
>> -
>> -	return err;
>> -}
>> -
>>   int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>>   {
>>   	int err;
>> @@ -616,7 +597,17 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>>   	}
>>   
>>   	cpus_read_lock();
>> -	err = _sdei_event_register(event);
>> +	if (event->type == SDEI_EVENT_TYPE_SHARED) {
>> +		err = sdei_api_event_register(event->event_num,
>> +					      sdei_entry_point,
>> +					      event->registered,
>> +					      SDEI_EVENT_REGISTER_RM_ANY, 0);
>> +	} else {
>> +		err = sdei_do_cross_call(_local_event_register, event);
>> +		if (err)
>> +			sdei_do_cross_call(_local_event_unregister, event);
>> +	}
>> +
>>   	if (err) {
>>   		sdei_event_destroy(event);
>>   		pr_warn("Failed to register event %u: %d\n", event_num, err);
>> @@ -633,33 +624,6 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>>   	return err;
>>   }
>>   
>> -static int sdei_reregister_event_llocked(struct sdei_event *event)
>> -{
>> -	int err;
>> -
>> -	lockdep_assert_held(&sdei_events_lock);
>> -	lockdep_assert_held(&sdei_list_lock);
>> -
>> -	err = _sdei_event_register(event);
>> -	if (err) {
>> -		pr_err("Failed to re-register event %u\n", event->event_num);
>> -		sdei_event_destroy_llocked(event);
>> -		return err;
>> -	}
>> -
>> -	if (event->reenable) {
>> -		if (event->type == SDEI_EVENT_TYPE_SHARED)
>> -			err = sdei_api_event_enable(event->event_num);
>> -		else
>> -			err = sdei_do_cross_call(_local_event_enable, event);
>> -	}
>> -
>> -	if (err)
>> -		pr_err("Failed to re-enable event %u\n", event->event_num);
>> -
>> -	return err;
>> -}
>> -
>>   static int sdei_reregister_shared(void)
>>   {
>>   	int err = 0;
>> @@ -672,9 +636,24 @@ static int sdei_reregister_shared(void)
>>   			continue;
>>   
>>   		if (event->reregister) {
>> -			err = sdei_reregister_event_llocked(event);
>> -			if (err)
>> +			err = sdei_api_event_register(event->event_num,
>> +					sdei_entry_point, event->registered,
>> +					SDEI_EVENT_REGISTER_RM_ANY, 0);
>> +			if (err) {
>> +				sdei_event_destroy_llocked(event);
>> +				pr_err("Failed to re-register event %u\n",
>> +				       event->event_num);
>>   				break;
>> +			}
>> +		}
>> +
>> +		if (event->reenable) {
>> +			err = sdei_api_event_enable(event->event_num);
>> +			if (err) {
>> +				pr_err("Failed to re-enable event %u\n",
>> +				       event->event_num);
>> +				break;
>> +			}
>>   		}
>>   	}
>>   	spin_unlock(&sdei_list_lock);

Thanks,
Gavin

Patch
diff mbox series

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index da0e0d5591a8..f4cd9791242f 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -575,25 +575,6 @@  static void _local_event_register(void *data)
 	sdei_cross_call_return(arg, err);
 }
 
-static int _sdei_event_register(struct sdei_event *event)
-{
-	int err;
-
-	lockdep_assert_held(&sdei_events_lock);
-
-	if (event->type == SDEI_EVENT_TYPE_SHARED)
-		return sdei_api_event_register(event->event_num,
-					       sdei_entry_point,
-					       event->registered,
-					       SDEI_EVENT_REGISTER_RM_ANY, 0);
-
-	err = sdei_do_cross_call(_local_event_register, event);
-	if (err)
-		sdei_do_cross_call(_local_event_unregister, event);
-
-	return err;
-}
-
 int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
 {
 	int err;
@@ -616,7 +597,17 @@  int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
 	}
 
 	cpus_read_lock();
-	err = _sdei_event_register(event);
+	if (event->type == SDEI_EVENT_TYPE_SHARED) {
+		err = sdei_api_event_register(event->event_num,
+					      sdei_entry_point,
+					      event->registered,
+					      SDEI_EVENT_REGISTER_RM_ANY, 0);
+	} else {
+		err = sdei_do_cross_call(_local_event_register, event);
+		if (err)
+			sdei_do_cross_call(_local_event_unregister, event);
+	}
+
 	if (err) {
 		sdei_event_destroy(event);
 		pr_warn("Failed to register event %u: %d\n", event_num, err);
@@ -633,33 +624,6 @@  int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
 	return err;
 }
 
-static int sdei_reregister_event_llocked(struct sdei_event *event)
-{
-	int err;
-
-	lockdep_assert_held(&sdei_events_lock);
-	lockdep_assert_held(&sdei_list_lock);
-
-	err = _sdei_event_register(event);
-	if (err) {
-		pr_err("Failed to re-register event %u\n", event->event_num);
-		sdei_event_destroy_llocked(event);
-		return err;
-	}
-
-	if (event->reenable) {
-		if (event->type == SDEI_EVENT_TYPE_SHARED)
-			err = sdei_api_event_enable(event->event_num);
-		else
-			err = sdei_do_cross_call(_local_event_enable, event);
-	}
-
-	if (err)
-		pr_err("Failed to re-enable event %u\n", event->event_num);
-
-	return err;
-}
-
 static int sdei_reregister_shared(void)
 {
 	int err = 0;
@@ -672,9 +636,24 @@  static int sdei_reregister_shared(void)
 			continue;
 
 		if (event->reregister) {
-			err = sdei_reregister_event_llocked(event);
-			if (err)
+			err = sdei_api_event_register(event->event_num,
+					sdei_entry_point, event->registered,
+					SDEI_EVENT_REGISTER_RM_ANY, 0);
+			if (err) {
+				sdei_event_destroy_llocked(event);
+				pr_err("Failed to re-register event %u\n",
+				       event->event_num);
 				break;
+			}
+		}
+
+		if (event->reenable) {
+			err = sdei_api_event_enable(event->event_num);
+			if (err) {
+				pr_err("Failed to re-enable event %u\n",
+				       event->event_num);
+				break;
+			}
 		}
 	}
 	spin_unlock(&sdei_list_lock);