[PATCH v4 14/15] drivers/firmware/sdei: Expose struct sdei_event

Gavin Shan gshan at redhat.com
Thu Jul 30 20:20:06 EDT 2020


Hi Jonathan,

On 7/30/20 8:54 PM, Jonathan Cameron wrote:
> 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!
> 

Yeah, more changes are introduced because of this. However, I do think
it's a good idea to hide these fields and prevent them being exposed
in long run. So I think the effort is still worthy.

> I'm fine with it, but others may well not be!
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron at huwei.com>
> 

Thank you for your comments and time on this. I think you've finished
the review on all the patches :)

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

Agree, I will put this into my TODO list and post a patch to address it
later. I don't want introduce more changes into this patch, considering
the change size.

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

Yeah, but it's fine not to move it.

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

Thanks,
Gavin
  




More information about the linux-arm-kernel mailing list