[PATCH v4 14/15] drivers/firmware/sdei: Expose struct sdei_event
Jonathan Cameron
Jonathan.Cameron at Huawei.com
Thu Jul 30 06:54:16 EDT 2020
On Thu, 30 Jul 2020 11:45:29 +1000
Gavin Shan <gshan at redhat.com> wrote:
> This splits struct sdei_event into two structs: sdei_event and
> sdei_internal_event. The former one can be dereferenced from external
> module like arm64/kvm when SDEI virtualization is supported. The later
> one is used by the client driver only.
>
> This also renames local variables from @event to @event_el if their
> type is "struct sdei_internal_event" to make the variable name a bit
> meaningful. Note that it's not applied to sdei_do_{local,cross}_call()
> because CROSSCALL_INIT stops us doing this unless much more changes
> are needed. I'm avoiding to do that.
>
> Besides, @event is added as "struct sdei_event" to cache @event_el->event
> if it's accessed for multiple times in the particular function to reduce
> the code change size.
>
> This shouldn't cause functional changes.
>
> Signed-off-by: Gavin Shan <gshan at redhat.com>
> ---
> v4: Rename variable @event to @event_el if it's internal event
Looks like a few cases were missed.
> Use @event to cache @event_el->event if it's used for multiple times
I'm not totally sure this splitting of the structure was worth the pain :(
(even though I suggested it :)
It perhaps makes things easier in the long run though. Until we get the
user in place, it does look a bit over architected!
I'm fine with it, but others may well not be!
Reviewed-by: Jonathan Cameron <Jonathan.Cameron at huwei.com>
> ---
> drivers/firmware/arm_sdei.c | 160 ++++++++++++++++++++----------------
> include/linux/arm_sdei.h | 6 ++
> 2 files changed, 94 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index 2678475940e6..f9827c096275 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -44,16 +44,14 @@ static asmlinkage void (*sdei_firmware_call)(unsigned long function_id,
> /* entry point from firmware to arch asm code */
> static unsigned long sdei_entry_point;
>
> -struct sdei_event {
> +struct sdei_internal_event {
> + struct sdei_event event;
> +
> /* These three are protected by the sdei_list_lock */
> struct list_head list;
> bool reregister;
> bool reenable;
>
> - u32 event_num;
> - u8 type;
> - u8 priority;
> -
> /* This pointer is handed to firmware as the event argument. */
> union {
> /* Shared events */
> @@ -73,7 +71,7 @@ static LIST_HEAD(sdei_list);
>
> /* Private events are registered/enabled via IPI passing one of these */
> struct sdei_crosscall_args {
> - struct sdei_event *event;
> + struct sdei_internal_event *event;
Even though it is a bit painful should probably aim for consistent naming so
this and the other cases related to crosscall should be event_el;
> atomic_t errors;
> int first_error;
> };
> @@ -86,7 +84,7 @@ struct sdei_crosscall_args {
> } while (0)
>
> static inline int sdei_do_local_call(smp_call_func_t fn,
> - struct sdei_event *event)
> + struct sdei_internal_event *event)
> {
> struct sdei_crosscall_args arg;
>
> @@ -97,7 +95,7 @@ static inline int sdei_do_local_call(smp_call_func_t fn,
> }
>
> static inline int sdei_do_cross_call(smp_call_func_t fn,
> - struct sdei_event *event)
> + struct sdei_internal_event *event)
> {
> struct sdei_crosscall_args arg;
>
> @@ -162,16 +160,16 @@ static int invoke_sdei_fn(unsigned long function_id, unsigned long arg0,
> }
> NOKPROBE_SYMBOL(invoke_sdei_fn);
>
> -static struct sdei_event *sdei_event_find(u32 event_num)
> +static struct sdei_internal_event *sdei_event_find(u32 event_num)
> {
> - struct sdei_event *e, *found = NULL;
> + struct sdei_internal_event *event_el, *found = NULL;
>
> lockdep_assert_held(&sdei_events_lock);
>
> spin_lock(&sdei_list_lock);
> - list_for_each_entry(e, &sdei_list, list) {
> - if (e->event_num == event_num) {
> - found = e;
> + list_for_each_entry(event_el, &sdei_list, list) {
> + if (event_el->event.event_num == event_num) {
> + found = event_el;
> break;
> }
> }
> @@ -193,24 +191,26 @@ static int sdei_api_event_get_info(u32 event, u32 info, u64 *result)
> 0, 0, result);
> }
>
> -static struct sdei_event *sdei_event_create(u32 event_num,
> - sdei_event_callback *cb,
> - void *cb_arg)
> +static struct sdei_internal_event *sdei_event_create(u32 event_num,
> + sdei_event_callback *cb,
> + void *cb_arg)
> {
> int err;
> u64 result;
> + struct sdei_internal_event *event_el;
> struct sdei_event *event;
> struct sdei_registered_event *reg;
>
> lockdep_assert_held(&sdei_events_lock);
>
> - event = kzalloc(sizeof(*event), GFP_KERNEL);
> - if (!event) {
> + event_el = kzalloc(sizeof(*event_el), GFP_KERNEL);
> + if (!event_el) {
> err = -ENOMEM;
> goto fail;
> }
>
> - INIT_LIST_HEAD(&event->list);
> + event = &event_el->event;
> + INIT_LIST_HEAD(&event_el->list);
> event->event_num = event_num;
>
> err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_PRIORITY,
> @@ -237,7 +237,7 @@ static struct sdei_event *sdei_event_create(u32 event_num,
>
> reg->callback = cb;
> reg->callback_arg = cb_arg;
> - event->registered = reg;
> + event_el->registered = reg;
> } else {
> int cpu;
> struct sdei_registered_event __percpu *regs;
> @@ -257,39 +257,39 @@ static struct sdei_event *sdei_event_create(u32 event_num,
> reg->callback_arg = cb_arg;
> }
>
> - event->private_registered = regs;
> + event_el->private_registered = regs;
> }
>
> spin_lock(&sdei_list_lock);
> - list_add(&event->list, &sdei_list);
> + list_add(&event_el->list, &sdei_list);
> spin_unlock(&sdei_list_lock);
>
> - return event;
> + return event_el;
>
> fail:
> - kfree(event);
> + kfree(event_el);
> return ERR_PTR(err);
> }
>
> -static void sdei_event_destroy_llocked(struct sdei_event *event)
> +static void sdei_event_destroy_llocked(struct sdei_internal_event *event_el)
> {
> lockdep_assert_held(&sdei_events_lock);
> lockdep_assert_held(&sdei_list_lock);
>
> - list_del(&event->list);
> + list_del(&event_el->list);
>
> - if (event->type == SDEI_EVENT_TYPE_SHARED)
> - kfree(event->registered);
> + if (event_el->event.type == SDEI_EVENT_TYPE_SHARED)
> + kfree(event_el->registered);
> else
> - free_percpu(event->private_registered);
> + free_percpu(event_el->private_registered);
>
> - kfree(event);
> + kfree(event_el);
> }
>
> -static void sdei_event_destroy(struct sdei_event *event)
> +static void sdei_event_destroy(struct sdei_internal_event *event_el)
> {
> spin_lock(&sdei_list_lock);
> - sdei_event_destroy_llocked(event);
> + sdei_event_destroy_llocked(event_el);
> spin_unlock(&sdei_list_lock);
> }
>
> @@ -392,7 +392,7 @@ static void _local_event_enable(void *data)
>
> WARN_ON_ONCE(preemptible());
>
> - err = sdei_api_event_enable(arg->event->event_num);
> + err = sdei_api_event_enable(arg->event->event.event_num);
>
> sdei_cross_call_return(arg, err);
> }
> @@ -400,25 +400,27 @@ static void _local_event_enable(void *data)
> int sdei_event_enable(u32 event_num)
> {
> int err = -EINVAL;
> + struct sdei_internal_event *event_el;
> struct sdei_event *event;
>
> mutex_lock(&sdei_events_lock);
> - event = sdei_event_find(event_num);
> - if (!event) {
> + event_el = sdei_event_find(event_num);
> + if (!event_el) {
> mutex_unlock(&sdei_events_lock);
> return -ENOENT;
> }
>
>
> cpus_read_lock();
> + event = &event_el->event;
> 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);
> + err = sdei_do_cross_call(_local_event_enable, event_el);
>
> if (!err) {
> spin_lock(&sdei_list_lock);
> - event->reenable = true;
> + event_el->reenable = true;
> spin_unlock(&sdei_list_lock);
> }
> cpus_read_unlock();
> @@ -438,7 +440,7 @@ static void _ipi_event_disable(void *data)
> int err;
> struct sdei_crosscall_args *arg = data;
>
> - err = sdei_api_event_disable(arg->event->event_num);
> + err = sdei_api_event_disable(arg->event->event.event_num);
>
> sdei_cross_call_return(arg, err);
> }
> @@ -446,23 +448,25 @@ static void _ipi_event_disable(void *data)
> int sdei_event_disable(u32 event_num)
> {
> int err = -EINVAL;
> + struct sdei_internal_event *event_el;
> struct sdei_event *event;
>
> mutex_lock(&sdei_events_lock);
> - event = sdei_event_find(event_num);
> - if (!event) {
> + event_el = sdei_event_find(event_num);
> + if (!event_el) {
> mutex_unlock(&sdei_events_lock);
> return -ENOENT;
> }
>
> spin_lock(&sdei_list_lock);
> - event->reenable = false;
> + event_el->reenable = false;
> spin_unlock(&sdei_list_lock);
>
> + event = &event_el->event;
> if (event->type == SDEI_EVENT_TYPE_SHARED)
> err = sdei_api_event_disable(event->event_num);
> else
> - err = sdei_do_cross_call(_ipi_event_disable, event);
> + err = sdei_do_cross_call(_ipi_event_disable, event_el);
> mutex_unlock(&sdei_events_lock);
>
> return err;
> @@ -482,7 +486,7 @@ static void _local_event_unregister(void *data)
>
> WARN_ON_ONCE(preemptible());
>
> - err = sdei_api_event_unregister(arg->event->event_num);
> + err = sdei_api_event_unregister(arg->event->event.event_num);
>
> sdei_cross_call_return(arg, err);
> }
> @@ -490,32 +494,34 @@ static void _local_event_unregister(void *data)
> int sdei_event_unregister(u32 event_num)
> {
> int err;
> + struct sdei_internal_event *event_el;
> struct sdei_event *event;
>
> WARN_ON(in_nmi());
>
> mutex_lock(&sdei_events_lock);
> - event = sdei_event_find(event_num);
> - if (!event) {
> + event_el = sdei_event_find(event_num);
> + if (!event_el) {
> pr_warn("Event %u not registered\n", event_num);
> err = -ENOENT;
> goto unlock;
> }
>
> spin_lock(&sdei_list_lock);
> - event->reregister = false;
> - event->reenable = false;
> + event_el->reregister = false;
> + event_el->reenable = false;
> spin_unlock(&sdei_list_lock);
>
> + event = &event_el->event;
> if (event->type == SDEI_EVENT_TYPE_SHARED)
> err = sdei_api_event_unregister(event->event_num);
> else
> - err = sdei_do_cross_call(_local_event_unregister, event);
> + err = sdei_do_cross_call(_local_event_unregister, event_el);
>
> if (err)
> goto unlock;
>
> - sdei_event_destroy(event);
> + sdei_event_destroy(event_el);
> unlock:
> mutex_unlock(&sdei_events_lock);
>
> @@ -529,11 +535,13 @@ int sdei_event_unregister(u32 event_num)
> static int sdei_unregister_shared(void)
> {
> int err = 0;
> + struct sdei_internal_event *event_el;
> struct sdei_event *event;
>
> mutex_lock(&sdei_events_lock);
> spin_lock(&sdei_list_lock);
> - list_for_each_entry(event, &sdei_list, list) {
> + list_for_each_entry(event_el, &sdei_list, list) {
> + event = &event_el->event;
> if (event->type != SDEI_EVENT_TYPE_SHARED)
> continue;
>
> @@ -565,8 +573,8 @@ static void _local_event_register(void *data)
> WARN_ON(preemptible());
>
> reg = per_cpu_ptr(arg->event->private_registered, smp_processor_id());
> - err = sdei_api_event_register(arg->event->event_num, sdei_entry_point,
> - reg, 0, 0);
> + err = sdei_api_event_register(arg->event->event.event_num,
> + sdei_entry_point, reg, 0, 0);
>
> sdei_cross_call_return(arg, err);
> }
> @@ -574,6 +582,7 @@ static void _local_event_register(void *data)
> int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
> {
> int err;
> + struct sdei_internal_event *event_el;
> struct sdei_event *event;
>
> WARN_ON(in_nmi());
> @@ -585,33 +594,34 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
> goto unlock;
> }
>
> - event = sdei_event_create(event_num, cb, arg);
> - if (IS_ERR(event)) {
> - err = PTR_ERR(event);
> + event_el = sdei_event_create(event_num, cb, arg);
> + if (IS_ERR(event_el)) {
> + err = PTR_ERR(event_el);
> pr_warn("Failed to create event %u: %d\n", event_num, err);
> goto unlock;
> }
>
> cpus_read_lock();
> + event = &event_el->event;
> if (event->type == SDEI_EVENT_TYPE_SHARED) {
> err = sdei_api_event_register(event->event_num,
> sdei_entry_point,
> - event->registered,
> + event_el->registered,
> SDEI_EVENT_REGISTER_RM_ANY, 0);
> } else {
> - err = sdei_do_cross_call(_local_event_register, event);
> + err = sdei_do_cross_call(_local_event_register, event_el);
> if (err)
> - sdei_do_cross_call(_local_event_unregister, event);
> + sdei_do_cross_call(_local_event_unregister, event_el);
> }
>
> if (err) {
> - sdei_event_destroy(event);
> + sdei_event_destroy(event_el);
> pr_warn("Failed to register event %u: %d\n", event_num, err);
> goto cpu_unlock;
> }
>
> spin_lock(&sdei_list_lock);
> - event->reregister = true;
> + event_el->reregister = true;
> spin_unlock(&sdei_list_lock);
> cpu_unlock:
> cpus_read_unlock();
> @@ -623,27 +633,29 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
> static int sdei_reregister_shared(void)
> {
> int err = 0;
> + struct sdei_internal_event *event_el;
> struct sdei_event *event;
Could reduce the scope by moving this in the loop. Not that important though.
>
> mutex_lock(&sdei_events_lock);
> spin_lock(&sdei_list_lock);
> - list_for_each_entry(event, &sdei_list, list) {
> + list_for_each_entry(event_el, &sdei_list, list) {
> + event = &event_el->event;
> if (event->type != SDEI_EVENT_TYPE_SHARED)
> continue;
>
> - if (event->reregister) {
> + if (event_el->reregister) {
> err = sdei_api_event_register(event->event_num,
> - sdei_entry_point, event->registered,
> + sdei_entry_point, event_el->registered,
> SDEI_EVENT_REGISTER_RM_ANY, 0);
> if (err) {
> - sdei_event_destroy_llocked(event);
> + sdei_event_destroy_llocked(event_el);
> pr_err("Failed to re-register event %u\n",
> event->event_num);
> break;
> }
> }
>
> - if (event->reenable) {
> + if (event_el->reenable) {
> err = sdei_api_event_enable(event->event_num);
> if (err) {
> pr_err("Failed to re-enable event %u\n",
> @@ -660,16 +672,18 @@ static int sdei_reregister_shared(void)
>
> static int sdei_cpuhp_down(unsigned int cpu)
> {
> + struct sdei_internal_event *event_el;
> struct sdei_event *event;
> int err;
>
> /* un-register private events */
> spin_lock(&sdei_list_lock);
> - list_for_each_entry(event, &sdei_list, list) {
> + list_for_each_entry(event_el, &sdei_list, list) {
> + event = &event_el->event;
> if (event->type == SDEI_EVENT_TYPE_SHARED)
> continue;
>
> - err = sdei_do_local_call(_local_event_unregister, event);
> + err = sdei_do_local_call(_local_event_unregister, event_el);
> if (err) {
> pr_err("Failed to unregister event %u: %d\n",
> event->event_num, err);
> @@ -682,25 +696,27 @@ static int sdei_cpuhp_down(unsigned int cpu)
>
> static int sdei_cpuhp_up(unsigned int cpu)
> {
> + struct sdei_internal_event *event_el;
> struct sdei_event *event;
> int err;
>
> /* re-register/enable private events */
> spin_lock(&sdei_list_lock);
> - list_for_each_entry(event, &sdei_list, list) {
> + list_for_each_entry(event_el, &sdei_list, list) {
> + event = &event_el->event;
> if (event->type == SDEI_EVENT_TYPE_SHARED)
> continue;
>
> - if (event->reregister) {
> - err = sdei_do_local_call(_local_event_register, event);
> + if (event_el->reregister) {
> + err = sdei_do_local_call(_local_event_register, event_el);
> if (err) {
> pr_err("Failed to re-register event %u: %d\n",
> event->event_num, err);
> }
> }
>
> - if (event->reenable) {
> - err = sdei_do_local_call(_local_event_enable, event);
> + if (event_el->reenable) {
> + err = sdei_do_local_call(_local_event_enable, event_el);
> if (err) {
> pr_err("Failed to re-enable event %u: %d\n",
> event->event_num, err);
> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
> index 0a241c5c911d..d2464a18b6ff 100644
> --- a/include/linux/arm_sdei.h
> +++ b/include/linux/arm_sdei.h
> @@ -22,6 +22,12 @@
> */
> typedef int (sdei_event_callback)(u32 event, struct pt_regs *regs, void *arg);
>
> +struct sdei_event {
> + u32 event_num;
> + u8 type;
> + u8 priority;
> +};
> +
> /*
> * Register your callback to claim an event. The event must be described
> * by firmware.
More information about the linux-arm-kernel
mailing list