Re: [PATCH 4/4] ras/events: Trace CXL CPER events even without the CXL stack loaded

[Ira Weiny]

Dan Williams wrote:
> Ira Weiny wrote:
> > 
> > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> > index f433f4eae888..9ac323cbf195 100644
> > --- a/drivers/acpi/apei/ghes.c
> > +++ b/drivers/acpi/apei/ghes.c
> > @@ -729,7 +729,10 @@ static void cxl_cper_local_fn(struct work_struct *work)
> >  
> >  	while (kfifo_out_spinlocked(&cxl_cper_fifo, &wd, 1,
> >  			&cxl_cper_read_lock)) {
> > -		/* drop msg */
> > +		struct cxl_cper_event_rec *rec = &wd.rec;
> > +		union cxl_event *evt = &rec->event;
> > +
> > +		trace_cper_cxl_gen_event(rec, &evt->generic);
> 
> So it was confusing to read the empty stub function 2 patches back when this
> change was coming, and basic reporting of CXL event does not need the
> workqueue indirection. Note that EDAC triggers trace events directly in
> the atomic notifier chain, so CXL could do the same.
> 
> >  static DECLARE_WORK(cxl_local_work, cxl_cper_local_fn);
> > diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
> > index cbd3ddd7c33d..319faf552b65 100644
> > --- a/include/ras/ras_event.h
> > +++ b/include/ras/ras_event.h
> 
> This is more heavywieght than I was expecting and defeats the purpose of
> centralizing advanced decode in the CXL driver itself.

Fair enough but it is not that heavy IMO.  It was mostly lifted from the
CXL traces.

> 
> I would expect this to be just the tracing equivalent of the
> ignore_section logic in cper_estatus_print_section().

I think it is fair to give the user some information.  I'll redo this
patch to be first and drop the extra work item.

Ira