| Message ID | 20200722095740.28560-13-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor SDEI client driver | expand |
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);
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
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);
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(-)