Re: [PATCH v17 05/11] cxl: Limit CXL-CPER kfifo registration functions scope

[Dave Jiang]

On 5/5/26 10:30 AM, Terry Bowman wrote:
> From: Dan Williams <djbw@xxxxxxxxxx>
> 
> Some CPER functions used by CXL drivers are exported using the
> EXPORT_SYMBOL_NS_GPL(fn, ns) macro. This doesn't provide compile time
> enforcement or visibility of the consumers.
> 
> This can be improved by using EXPORT_SYMBOL_FOR_MODULES() instead.
> EXPORT_SYMBOL_FOR_MODULES() explicitly names the modules that can access
> the function. This provides more precise control and visibility of symbol
> exposure than the namespace macro. It also provides compile time checking.
> 
> To improve control and clarity, update cxl_cper_register_prot_err_work(),
> cxl_cper_unregister_prot_err_work(), and cxl_cper_prot_err_kfifo_get()
> to use EXPORT_SYMBOL_FOR_MODULES(). Also, update the register and unregister
> functions to return void type.
> 
> Update the CPER kfifo unregister to cancel work while using
> synchronization.
> 
> Co-developed-by: Terry Bowman <terry.bowman@xxxxxxx>
> Signed-off-by: Terry Bowman <terry.bowman@xxxxxxx>
> Signed-off-by: Dan Williams <djbw@xxxxxxxxxx>

Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>


> 
> ---
> 
> Changes in v16->v17:
> - Split from v16 02/10 ("Update unregistration for AER-CXL and
>   CPER-CXL kfifos"); AER-CXL half folded into v17 01/10.
> - Convert exports to EXPORT_SYMBOL_FOR_MODULES("cxl_core").
> - Change register/unregister return type from int to void.
> - Drop work_struct argument from cxl_cper_unregister_prot_err_work();
>   it now cancels its own work.
> - Remove now-redundant cancel_work_sync() from cxl_ras_exit().
> - Add WARN_ONCE() in cxl_cper_register_prot_err_work() for
>   double-registration.
> ---
>  drivers/acpi/apei/ghes.c | 27 ++++++++++++++-------------
>  drivers/cxl/core/ras.c   |  6 +++---
>  include/cxl/event.h      | 10 ++++------
>  3 files changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 3236a3ce79d6..dd0a073af93c 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -778,33 +778,34 @@ static void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
>  #endif
>  }
>  
> -int cxl_cper_register_prot_err_work(struct work_struct *work)
> +void cxl_cper_register_prot_err_work(struct work_struct *work)
>  {
> -	if (cxl_cper_prot_err_work)
> -		return -EINVAL;
> -
>  	guard(spinlock)(&cxl_cper_prot_err_work_lock);
> +	WARN_ONCE(cxl_cper_prot_err_work,
> +		  "CPER-CXL kfifo consumer already registered\n");
>  	cxl_cper_prot_err_work = work;
> -	return 0;
>  }
> -EXPORT_SYMBOL_NS_GPL(cxl_cper_register_prot_err_work, "CXL");
> +EXPORT_SYMBOL_FOR_MODULES(cxl_cper_register_prot_err_work, "cxl_core");
>  
> -int cxl_cper_unregister_prot_err_work(struct work_struct *work)
> +void cxl_cper_unregister_prot_err_work(void)
>  {
> -	if (cxl_cper_prot_err_work != work)
> -		return -EINVAL;
> +	struct work_struct *work;
>  
> -	guard(spinlock)(&cxl_cper_prot_err_work_lock);
> +	spin_lock(&cxl_cper_prot_err_work_lock);
> +	work = cxl_cper_prot_err_work;
>  	cxl_cper_prot_err_work = NULL;
> -	return 0;
> +	spin_unlock(&cxl_cper_prot_err_work_lock);
> +
> +	if (work)
> +		cancel_work_sync(work);
>  }
> -EXPORT_SYMBOL_NS_GPL(cxl_cper_unregister_prot_err_work, "CXL");
> +EXPORT_SYMBOL_FOR_MODULES(cxl_cper_unregister_prot_err_work, "cxl_core");
>  
>  int cxl_cper_prot_err_kfifo_get(struct cxl_cper_prot_err_work_data *wd)
>  {
>  	return kfifo_get(&cxl_cper_prot_err_fifo, wd);
>  }
> -EXPORT_SYMBOL_NS_GPL(cxl_cper_prot_err_kfifo_get, "CXL");
> +EXPORT_SYMBOL_FOR_MODULES(cxl_cper_prot_err_kfifo_get, "cxl_core");
>  
>  /* Room for 8 entries for each of the 4 event log queues */
>  #define CXL_CPER_FIFO_DEPTH 32
> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index 56611da8357a..9193dac4e507 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c
> @@ -68,13 +68,13 @@ static DECLARE_WORK(cxl_cper_prot_err_work, cxl_cper_prot_err_work_fn);
>  
>  int cxl_ras_init(void)
>  {
> -	return cxl_cper_register_prot_err_work(&cxl_cper_prot_err_work);
> +	cxl_cper_register_prot_err_work(&cxl_cper_prot_err_work);
> +	return 0;
>  }
>  
>  void cxl_ras_exit(void)
>  {
> -	cxl_cper_unregister_prot_err_work(&cxl_cper_prot_err_work);
> -	cancel_work_sync(&cxl_cper_prot_err_work);
> +	cxl_cper_unregister_prot_err_work();
>  }
>  
>  static void cxl_dport_map_ras(struct cxl_dport *dport)
> diff --git a/include/cxl/event.h b/include/cxl/event.h
> index ff97fea718d2..51acedb0d683 100644
> --- a/include/cxl/event.h
> +++ b/include/cxl/event.h
> @@ -289,8 +289,8 @@ struct cxl_cper_prot_err_work_data {
>  int cxl_cper_register_work(struct work_struct *work);
>  int cxl_cper_unregister_work(struct work_struct *work);
>  int cxl_cper_kfifo_get(struct cxl_cper_work_data *wd);
> -int cxl_cper_register_prot_err_work(struct work_struct *work);
> -int cxl_cper_unregister_prot_err_work(struct work_struct *work);
> +void cxl_cper_register_prot_err_work(struct work_struct *work);
> +void cxl_cper_unregister_prot_err_work(void);
>  int cxl_cper_prot_err_kfifo_get(struct cxl_cper_prot_err_work_data *wd);
>  #else
>  static inline int cxl_cper_register_work(struct work_struct *work)
> @@ -306,13 +306,11 @@ static inline int cxl_cper_kfifo_get(struct cxl_cper_work_data *wd)
>  {
>  	return 0;
>  }
> -static inline int cxl_cper_register_prot_err_work(struct work_struct *work)
> +static inline void cxl_cper_register_prot_err_work(struct work_struct *work)
>  {
> -	return 0;
>  }
> -static inline int cxl_cper_unregister_prot_err_work(struct work_struct *work)
> +static inline void cxl_cper_unregister_prot_err_work(void)
>  {
> -	return 0;
>  }
>  static inline int cxl_cper_prot_err_kfifo_get(struct cxl_cper_prot_err_work_data *wd)
>  {