[v3] acpi/ghes, cper, cxl: Process CXL CPER Protocol errors

[PATCH v3 5/7] acpi/ghes, cxl: Refactor work registration functions to support multiple workqueues

Posted by Smita Koralahalli 1 year, 2 months ago

Refactor the work registration and unregistration functions in GHES to
enable reuse across different workqueues. This update lays the foundation
for integrating additional workqueues in the CXL subsystem for better
modularity and code reuse.

Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
---
 drivers/acpi/apei/ghes.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 082c409707ba..62ffe6eb5503 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -717,26 +717,42 @@ static void cxl_cper_post_event(enum cxl_event_type event_type,
 	schedule_work(cxl_cper_work);
 }
 
-int cxl_cper_register_event_work(struct work_struct *work)
+static int cxl_cper_register_work(struct work_struct **work_ptr,
+				  spinlock_t *lock,
+				  struct work_struct *work)
 {
-	if (cxl_cper_work)
+	if (*work_ptr)
 		return -EINVAL;
 
-	guard(spinlock)(&cxl_cper_work_lock);
-	cxl_cper_work = work;
+	guard(spinlock)(lock);
+	*work_ptr = work;
 	return 0;
 }
-EXPORT_SYMBOL_NS_GPL(cxl_cper_register_event_work, CXL);
 
-int cxl_cper_unregister_event_work(struct work_struct *work)
+static int cxl_cper_unregister_work(struct work_struct **work_ptr,
+				    spinlock_t *lock,
+				    struct work_struct *work)
 {
-	if (cxl_cper_work != work)
+	if (*work_ptr != work)
 		return -EINVAL;
 
-	guard(spinlock)(&cxl_cper_work_lock);
-	cxl_cper_work = NULL;
+	guard(spinlock)(lock);
+	*work_ptr = NULL;
 	return 0;
 }
+
+int cxl_cper_register_event_work(struct work_struct *work)
+{
+	return cxl_cper_register_work(&cxl_cper_work, &cxl_cper_work_lock,
+				      work);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_cper_register_event_work, CXL);
+
+int cxl_cper_unregister_event_work(struct work_struct *work)
+{
+	return cxl_cper_unregister_work(&cxl_cper_work, &cxl_cper_work_lock,
+					work);
+}
 EXPORT_SYMBOL_NS_GPL(cxl_cper_unregister_event_work, CXL);
 
 int cxl_cper_kfifo_get(struct cxl_cper_work_data *wd)
-- 
2.17.1

Re: [PATCH v3 5/7] acpi/ghes, cxl: Refactor work registration functions to support multiple workqueues

Posted by Jonathan Cameron 1 year, 2 months ago

On Tue, 19 Nov 2024 00:39:13 +0000
Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> wrote:

> Refactor the work registration and unregistration functions in GHES to
> enable reuse across different workqueues. This update lays the foundation
> for integrating additional workqueues in the CXL subsystem for better
> modularity and code reuse.
> 
> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
> ---
>  drivers/acpi/apei/ghes.c | 34 +++++++++++++++++++++++++---------
>  1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 082c409707ba..62ffe6eb5503 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -717,26 +717,42 @@ static void cxl_cper_post_event(enum cxl_event_type event_type,
>  	schedule_work(cxl_cper_work);
>  }
>  
> -int cxl_cper_register_event_work(struct work_struct *work)
> +static int cxl_cper_register_work(struct work_struct **work_ptr,
> +				  spinlock_t *lock,
> +				  struct work_struct *work)

This is a somewhat strange interface.  It doesn't
really do anything particularly useful. I'd be tempted to
just open code this at each call site.


>  {
> -	if (cxl_cper_work)
> +	if (*work_ptr)
>  		return -EINVAL;
>  
> -	guard(spinlock)(&cxl_cper_work_lock);
> -	cxl_cper_work = work;
> +	guard(spinlock)(lock);
> +	*work_ptr = work;
>  	return 0;
>  }
> -EXPORT_SYMBOL_NS_GPL(cxl_cper_register_event_work, CXL);
>  
> -int cxl_cper_unregister_event_work(struct work_struct *work)
> +static int cxl_cper_unregister_work(struct work_struct **work_ptr,
> +				    spinlock_t *lock,
> +				    struct work_struct *work)
>  {
> -	if (cxl_cper_work != work)
> +	if (*work_ptr != work)
As above.

>  		return -EINVAL;
>  
> -	guard(spinlock)(&cxl_cper_work_lock);
> -	cxl_cper_work = NULL;
> +	guard(spinlock)(lock);
> +	*work_ptr = NULL;
>  	return 0;
>  }
> +
> +int cxl_cper_register_event_work(struct work_struct *work)
> +{
> +	return cxl_cper_register_work(&cxl_cper_work, &cxl_cper_work_lock,
> +				      work);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_cper_register_event_work, CXL);
> +
> +int cxl_cper_unregister_event_work(struct work_struct *work)
> +{
> +	return cxl_cper_unregister_work(&cxl_cper_work, &cxl_cper_work_lock,
> +					work);
> +}
>  EXPORT_SYMBOL_NS_GPL(cxl_cper_unregister_event_work, CXL);
>  
>  int cxl_cper_kfifo_get(struct cxl_cper_work_data *wd)

Re: [PATCH v3 5/7] acpi/ghes, cxl: Refactor work registration functions to support multiple workqueues

Posted by Smita Koralahalli 1 year, 2 months ago

On 11/26/2024 7:57 AM, Jonathan Cameron wrote:
> On Tue, 19 Nov 2024 00:39:13 +0000
> Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> wrote:
> 
>> Refactor the work registration and unregistration functions in GHES to
>> enable reuse across different workqueues. This update lays the foundation
>> for integrating additional workqueues in the CXL subsystem for better
>> modularity and code reuse.
>>
>> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
>> ---
>>   drivers/acpi/apei/ghes.c | 34 +++++++++++++++++++++++++---------
>>   1 file changed, 25 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 082c409707ba..62ffe6eb5503 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -717,26 +717,42 @@ static void cxl_cper_post_event(enum cxl_event_type event_type,
>>   	schedule_work(cxl_cper_work);
>>   }
>>   
>> -int cxl_cper_register_event_work(struct work_struct *work)
>> +static int cxl_cper_register_work(struct work_struct **work_ptr,
>> +				  spinlock_t *lock,
>> +				  struct work_struct *work)
> 
> This is a somewhat strange interface.  It doesn't
> really do anything particularly useful. I'd be tempted to
> just open code this at each call site.

Okay I will change.
> 
> 
>>   {
>> -	if (cxl_cper_work)
>> +	if (*work_ptr)
>>   		return -EINVAL;
>>   
>> -	guard(spinlock)(&cxl_cper_work_lock);
>> -	cxl_cper_work = work;
>> +	guard(spinlock)(lock);
>> +	*work_ptr = work;
>>   	return 0;
>>   }
>> -EXPORT_SYMBOL_NS_GPL(cxl_cper_register_event_work, CXL);
>>   
>> -int cxl_cper_unregister_event_work(struct work_struct *work)
>> +static int cxl_cper_unregister_work(struct work_struct **work_ptr,
>> +				    spinlock_t *lock,
>> +				    struct work_struct *work)
>>   {
>> -	if (cxl_cper_work != work)
>> +	if (*work_ptr != work)
> As above.
okay.

Thanks
Smita
> 
>>   		return -EINVAL;
>>   
>> -	guard(spinlock)(&cxl_cper_work_lock);
>> -	cxl_cper_work = NULL;
>> +	guard(spinlock)(lock);
>> +	*work_ptr = NULL;
>>   	return 0;
>>   }
>> +
>> +int cxl_cper_register_event_work(struct work_struct *work)
>> +{
>> +	return cxl_cper_register_work(&cxl_cper_work, &cxl_cper_work_lock,
>> +				      work);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_cper_register_event_work, CXL);
>> +
>> +int cxl_cper_unregister_event_work(struct work_struct *work)
>> +{
>> +	return cxl_cper_unregister_work(&cxl_cper_work, &cxl_cper_work_lock,
>> +					work);
>> +}
>>   EXPORT_SYMBOL_NS_GPL(cxl_cper_unregister_event_work, CXL);
>>   
>>   int cxl_cper_kfifo_get(struct cxl_cper_work_data *wd)
>

[PATCH v3 1/7] efi/cper, cxl: Prefix protocol error struct and function names with cxl_
[PATCH v3 2/7] efi/cper, cxl: Make definitions and structures global
[PATCH v3 3/7] efi/cper, cxl: Remove cper_cxl.h
[PATCH v3 4/7] acpi/ghes, cxl: Rename cxl_cper_register_work to cxl_cper_register_event_work
[PATCH v3 5/7] acpi/ghes, cxl: Refactor work registration functions to support multiple workqueues
[PATCH v3 6/7] acpi/ghes, cper: Recognize and cache CXL Protocol errors
[PATCH v3 7/7] acpi/ghes, cxl/pci: Process CXL CPER Protocol Errors