Re: [PATCH v2 14/17] drivers/firmware/sdei: Move struct sdei_event to header file

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

On Wed, 22 Jul 2020 19:57:37 +1000
Gavin Shan <gshan@redhat.com> wrote:

> This moves struct sdei_event to the header file so that it can be
> dereferenced by external modules. This is needed by the code to
> virtualize SDEI functionality, as part of the arm64/kvm.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
Hi Gavin,

One question inline.

Jonathan

> ---
> v2: Derived from "drivers/firmware/sdei: Identify event by struct"
> ---
>  drivers/firmware/arm_sdei.c | 20 ------------
>  include/linux/arm_sdei.h    | 61 ++++++++++++++++++++++++-------------
>  2 files changed, 40 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index a52dcff59a20..bdd2de0149c0 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -44,26 +44,6 @@ 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 {
> -	/* 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 */
> -		struct sdei_registered_event *registered;
> -
> -		/* CPU private events */
> -		struct sdei_registered_event __percpu *private_registered;
> -	};
> -};
> -
>  /* Take the mutex for any API call or modification. Take the mutex first. */
>  static DEFINE_MUTEX(sdei_events_lock);
>  
> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
> index 0a241c5c911d..fdc2f868d84b 100644
> --- a/include/linux/arm_sdei.h
> +++ b/include/linux/arm_sdei.h
> @@ -22,6 +22,46 @@
>   */
>  typedef int (sdei_event_callback)(u32 event, struct pt_regs *regs, void *arg);
>  
> +/*
> + * This struct represents an event that has been registered. The driver
> + * maintains a list of all events, and which ones are registered. (Private
> + * events have one entry in the list, but are registered on each CPU).
> + * A pointer to this struct is passed to firmware, and back to the event
> + * handler. The event handler can then use this to invoke the registered
> + * callback, without having to walk the list.
> + *
> + * For CPU private events, this structure is per-cpu.
> + */
> +struct sdei_registered_event {
> +	/* For use by arch code: */
> +	struct pt_regs          interrupted_regs;
> +
> +	sdei_event_callback	*callback;
> +	void			*callback_arg;
> +	u32			 event_num;
> +	u8			 priority;
> +};
> +
> +struct sdei_event {
> +	/* These three are protected by the sdei_list_lock */

As this patch leaves the sdei_list_lock as local to arm_sdei.c, is this comment still valid?

> +	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 */
> +		struct sdei_registered_event *registered;
> +
> +		/* CPU private events */
> +		struct sdei_registered_event __percpu *private_registered;
> +	};
> +};
> +
>  /*
>   * Register your callback to claim an event. The event must be described
>   * by firmware.
> @@ -51,27 +91,6 @@ static inline int sdei_mask_local_cpu(void) { return 0; }
>  static inline int sdei_unmask_local_cpu(void) { return 0; }
>  #endif /* CONFIG_ARM_SDE_INTERFACE */
>  
> -
> -/*
> - * This struct represents an event that has been registered. The driver
> - * maintains a list of all events, and which ones are registered. (Private
> - * events have one entry in the list, but are registered on each CPU).
> - * A pointer to this struct is passed to firmware, and back to the event
> - * handler. The event handler can then use this to invoke the registered
> - * callback, without having to walk the list.
> - *
> - * For CPU private events, this structure is per-cpu.
> - */
> -struct sdei_registered_event {
> -	/* For use by arch code: */
> -	struct pt_regs          interrupted_regs;
> -
> -	sdei_event_callback	*callback;
> -	void			*callback_arg;
> -	u32			 event_num;
> -	u8			 priority;
> -};
> -
>  /* The arch code entry point should then call this when an event arrives. */
>  int notrace sdei_event_handler(struct pt_regs *regs,
>  			       struct sdei_registered_event *arg);



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