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

[Gavin Shan]

Hi Jonathan,

On 7/24/20 1:19 AM, Jonathan Cameron wrote:
> On Wed, 22 Jul 2020 19:57:37 +1000
> Gavin Shan <gshan at 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 at redhat.com>

[...]

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

Yes, the comment is still valid. @sdei_list_lock is used to protect
the linked list (@sdei_list) and all elements (@event) in the list.
For example, the lock is taken before updating @event->reenabled in
function sdei_event_enable().

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

Thanks,
Gavin