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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Tue, 28 Jul 2020 12:59:54 +1000
Gavin Shan <gshan@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@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?

Thanks,

Jonathan



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

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



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel