[PATCH v3 14/15] drivers/firmware/sdei: Expose struct sdei_event
Gavin Shan
gshan at redhat.com
Wed Jul 29 20:34:21 EDT 2020
Hi Jonathan,
On 7/29/20 1:48 AM, Jonathan Cameron wrote:
> On Tue, 28 Jul 2020 12:59:54 +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 shouldn't cause functional changes.
>>
>> Signed-off-by: Gavin Shan <gshan at redhat.com>
>
> Hi Gavin,
>
> This ended up cleaner than I expected :)
>
> You may have already tried it, but if not it might be worth experimenting with
> a bit of naming changing and local variables.
>
> In most places you are operating only on either the
> struct sdei_event, or the wrapping struct sdei_internal_event
>
> It may help both readability and size of patch to give naming such as
>
> struct sdei_internal_event *event_el =...;
> struct sdei_event *event = &event_el->event;
>
>>From eyeballing the code I'm not sure if that's actually beneficial or not.
> It is tidier in the various loops over the list, but requires a bunch of
> renames elsewhere.
>
> What do you think?
>
I was thinking of this sort of changes and more code changes are needed
to do so. However, I think it's fine to use @event_el to dereference
the "struct sdei_internal_event" instance and @event for "struct sdei_event".
It makes the variable names meaningful and I think the effort is worthy.
So lets have more changes, combined to this patch in v4:
(1) Use @event_el to dereference SDEI internal event. Note that
we can't do this for sdei_do_{local,cross}_call() because
CROSSCALL_INIT stops us from doing this.
(2) Add @event in function where the SDEI event is deferrenced
for multiple times in the particular function. Otherwise,
@event won't be added.
By the way, here is the code change statistics without/with the renaming.
I think it's fine to do the renaming :)
drivers/firmware/arm_sdei.c | 116 ++++++++++++++++++------------------
include/linux/arm_sdei.h | 6 ++
2 files changed, 63 insertions(+), 59 deletions(-)
drivers/firmware/arm_sdei.c | 158 ++++++++++++++++++++++++++++++++---------------------------
include/linux/arm_sdei.h | 6 +++
2 files changed, 93 insertions(+), 71 deletions(-)
>
>
>> ---
>> v3: Split struct sdei_event into struct sdei_{internal,}_event and
>> struct sdei_internal_event is used by client driver only
>> ---
>> drivers/firmware/arm_sdei.c | 116 ++++++++++++++++++------------------
>> include/linux/arm_sdei.h | 6 ++
>> 2 files changed, 63 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
>> index 2678475940e6..c80085e30db1 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;
>> 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,15 +160,15 @@ 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 *e, *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) {
>> + if (e->event.event_num == event_num) {
>> found = e;
>> break;
>> }
>> @@ -193,13 +191,13 @@ 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_event *event;
>> + struct sdei_internal_event *event;
>> struct sdei_registered_event *reg;
>>
>> lockdep_assert_held(&sdei_events_lock);
>> @@ -211,29 +209,29 @@ static struct sdei_event *sdei_event_create(u32 event_num,
>> }
>>
>> INIT_LIST_HEAD(&event->list);
>> - event->event_num = event_num;
>> + event->event.event_num = event_num;
>>
>> err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_PRIORITY,
>> &result);
>> if (err)
>> goto fail;
>> - event->priority = result;
>> + event->event.priority = result;
>>
>> err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_TYPE,
>> &result);
>> if (err)
>> goto fail;
>> - event->type = result;
>> + event->event.type = result;
>>
>> - if (event->type == SDEI_EVENT_TYPE_SHARED) {
>> + if (event->event.type == SDEI_EVENT_TYPE_SHARED) {
>> reg = kzalloc(sizeof(*reg), GFP_KERNEL);
>> if (!reg) {
>> err = -ENOMEM;
>> goto fail;
>> }
>>
>> - reg->event_num = event->event_num;
>> - reg->priority = event->priority;
>> + reg->event_num = event->event.event_num;
>> + reg->priority = event->event.priority;
>>
>> reg->callback = cb;
>> reg->callback_arg = cb_arg;
>> @@ -251,8 +249,8 @@ static struct sdei_event *sdei_event_create(u32 event_num,
>> for_each_possible_cpu(cpu) {
>> reg = per_cpu_ptr(regs, cpu);
>>
>> - reg->event_num = event->event_num;
>> - reg->priority = event->priority;
>> + reg->event_num = event->event.event_num;
>> + reg->priority = event->event.priority;
>> reg->callback = cb;
>> reg->callback_arg = cb_arg;
>> }
>> @@ -271,14 +269,14 @@ static struct sdei_event *sdei_event_create(u32 event_num,
>> 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)
>> {
>> lockdep_assert_held(&sdei_events_lock);
>> lockdep_assert_held(&sdei_list_lock);
>>
>> list_del(&event->list);
>>
>> - if (event->type == SDEI_EVENT_TYPE_SHARED)
>> + if (event->event.type == SDEI_EVENT_TYPE_SHARED)
>> kfree(event->registered);
>> else
>> free_percpu(event->private_registered);
>> @@ -286,7 +284,7 @@ static void sdei_event_destroy_llocked(struct sdei_event *event)
>> kfree(event);
>> }
>>
>> -static void sdei_event_destroy(struct sdei_event *event)
>> +static void sdei_event_destroy(struct sdei_internal_event *event)
>> {
>> spin_lock(&sdei_list_lock);
>> sdei_event_destroy_llocked(event);
>> @@ -392,7 +390,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,7 +398,7 @@ static void _local_event_enable(void *data)
>> int sdei_event_enable(u32 event_num)
>> {
>> int err = -EINVAL;
>> - struct sdei_event *event;
>> + struct sdei_internal_event *event;
>>
>> mutex_lock(&sdei_events_lock);
>> event = sdei_event_find(event_num);
>> @@ -411,8 +409,8 @@ int sdei_event_enable(u32 event_num)
>>
>>
>> cpus_read_lock();
>> - if (event->type == SDEI_EVENT_TYPE_SHARED)
>> - err = sdei_api_event_enable(event->event_num);
>> + if (event->event.type == SDEI_EVENT_TYPE_SHARED)
>> + err = sdei_api_event_enable(event->event.event_num);
>> else
>> err = sdei_do_cross_call(_local_event_enable, event);
>>
>> @@ -438,7 +436,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,7 +444,7 @@ static void _ipi_event_disable(void *data)
>> int sdei_event_disable(u32 event_num)
>> {
>> int err = -EINVAL;
>> - struct sdei_event *event;
>> + struct sdei_internal_event *event;
>>
>> mutex_lock(&sdei_events_lock);
>> event = sdei_event_find(event_num);
>> @@ -459,8 +457,8 @@ int sdei_event_disable(u32 event_num)
>> event->reenable = false;
>> spin_unlock(&sdei_list_lock);
>>
>> - if (event->type == SDEI_EVENT_TYPE_SHARED)
>> - err = sdei_api_event_disable(event->event_num);
>> + if (event->event.type == SDEI_EVENT_TYPE_SHARED)
>> + err = sdei_api_event_disable(event->event.event_num);
>> else
>> err = sdei_do_cross_call(_ipi_event_disable, event);
>> mutex_unlock(&sdei_events_lock);
>> @@ -482,7 +480,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,7 +488,7 @@ static void _local_event_unregister(void *data)
>> int sdei_event_unregister(u32 event_num)
>> {
>> int err;
>> - struct sdei_event *event;
>> + struct sdei_internal_event *event;
>>
>> WARN_ON(in_nmi());
>>
>> @@ -507,8 +505,8 @@ int sdei_event_unregister(u32 event_num)
>> event->reenable = false;
>> spin_unlock(&sdei_list_lock);
>>
>> - if (event->type == SDEI_EVENT_TYPE_SHARED)
>> - err = sdei_api_event_unregister(event->event_num);
>> + if (event->event.type == SDEI_EVENT_TYPE_SHARED)
>> + err = sdei_api_event_unregister(event->event.event_num);
>> else
>> err = sdei_do_cross_call(_local_event_unregister, event);
>>
>> @@ -529,15 +527,15 @@ int sdei_event_unregister(u32 event_num)
>> static int sdei_unregister_shared(void)
>> {
>> int err = 0;
>> - struct sdei_event *event;
>> + struct sdei_internal_event *event;
>>
>> mutex_lock(&sdei_events_lock);
>> spin_lock(&sdei_list_lock);
>> list_for_each_entry(event, &sdei_list, list) {
>> - if (event->type != SDEI_EVENT_TYPE_SHARED)
>> + if (event->event.type != SDEI_EVENT_TYPE_SHARED)
>> continue;
>>
>> - err = sdei_api_event_unregister(event->event_num);
>> + err = sdei_api_event_unregister(event->event.event_num);
>> if (err)
>> break;
>> }
>> @@ -565,8 +563,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,7 +572,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_event *event;
>> + struct sdei_internal_event *event;
>>
>> WARN_ON(in_nmi());
>>
>> @@ -593,8 +591,8 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>> }
>>
>> cpus_read_lock();
>> - if (event->type == SDEI_EVENT_TYPE_SHARED) {
>> - err = sdei_api_event_register(event->event_num,
>> + if (event->event.type == SDEI_EVENT_TYPE_SHARED) {
>> + err = sdei_api_event_register(event->event.event_num,
>> sdei_entry_point,
>> event->registered,
>> SDEI_EVENT_REGISTER_RM_ANY, 0);
>> @@ -623,31 +621,31 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>> static int sdei_reregister_shared(void)
>> {
>> int err = 0;
>> - struct sdei_event *event;
>> + struct sdei_internal_event *event;
>>
>> mutex_lock(&sdei_events_lock);
>> spin_lock(&sdei_list_lock);
>> list_for_each_entry(event, &sdei_list, list) {
>
> Perhaps worth a local variable for event->event?
> If you were to do
> list_for_each_entry(event_el, &sdei_list, list) {
> sdei_event *event = &event_el->event;
>
> Would reduce the patch size and might give slightly more readable code.
>
> The issue is that you'd then need to be very careful with naming of
> event vs event_el (for element of list) to keep it consistent.
> It's possible it will cause more mess than the current approach!
>
Yep, will do in v4 :)
>> - if (event->type != SDEI_EVENT_TYPE_SHARED)
>> + if (event->event.type != SDEI_EVENT_TYPE_SHARED)
>> continue;
>>
>> if (event->reregister) {
>> - err = sdei_api_event_register(event->event_num,
>> + err = sdei_api_event_register(event->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);
>> + event->event.event_num);
>> break;
>> }
>> }
>>
>> if (event->reenable) {
>> - err = sdei_api_event_enable(event->event_num);
>> + err = sdei_api_event_enable(event->event.event_num);
>> if (err) {
>> pr_err("Failed to re-enable event %u\n",
>> - event->event_num);
>> + event->event.event_num);
>> break;
>> }
>> }
>> @@ -660,19 +658,19 @@ static int sdei_reregister_shared(void)
>>
>> static int sdei_cpuhp_down(unsigned int cpu)
>> {
>> - struct sdei_event *event;
>> + struct sdei_internal_event *event;
>> int err;
>>
>> /* un-register private events */
>> spin_lock(&sdei_list_lock);
>> list_for_each_entry(event, &sdei_list, list) {
>> - if (event->type == SDEI_EVENT_TYPE_SHARED)
>> + if (event->event.type == SDEI_EVENT_TYPE_SHARED)
>> continue;
>>
>> err = sdei_do_local_call(_local_event_unregister, event);
>> if (err) {
>> pr_err("Failed to unregister event %u: %d\n",
>> - event->event_num, err);
>> + event->event.event_num, err);
>> }
>> }
>> spin_unlock(&sdei_list_lock);
>> @@ -682,20 +680,20 @@ static int sdei_cpuhp_down(unsigned int cpu)
>>
>> static int sdei_cpuhp_up(unsigned int cpu)
>> {
>> - struct sdei_event *event;
>> + struct sdei_internal_event *event;
>> int err;
>>
>> /* re-register/enable private events */
>> spin_lock(&sdei_list_lock);
>> list_for_each_entry(event, &sdei_list, list) {
>> - if (event->type == SDEI_EVENT_TYPE_SHARED)
>> + if (event->event.type == SDEI_EVENT_TYPE_SHARED)
>> continue;
>>
>> if (event->reregister) {
>> err = sdei_do_local_call(_local_event_register, event);
>> if (err) {
>> pr_err("Failed to re-register event %u: %d\n",
>> - event->event_num, err);
>> + event->event.event_num, err);
>> }
>> }
>>
>> @@ -703,7 +701,7 @@ static int sdei_cpuhp_up(unsigned int cpu)
>> err = sdei_do_local_call(_local_event_enable, event);
>> if (err) {
>> pr_err("Failed to re-enable event %u: %d\n",
>> - event->event_num, err);
>> + event->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.
Thanks,
Gavin
More information about the linux-arm-kernel
mailing list