Re: [PATCH v2] acpi/ghes: Prevent sleeping with spinlock held

Ira Weiny
2024-02-14T17:11:38Z

Ira Weiny wrote:
> Jonathan Cameron wrote:
> > On Tue, 06 Feb 2024 14:15:32 -0800
> > Ira Weiny <ira.weiny@intel.com> wrote:
> >
> >

[snip]

> >
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 9ff8a439d674..7ee45f22f56f 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -2957,7 +2957,7 @@ static void output_printk(struct trace_event_buffer *fbuffer)
> > iter->ent = fbuffer->entry;
> > event_call->event.funcs->trace(iter, 0, event);
> > trace_seq_putc(&iter->seq, 0);
> > - printk("%s", iter->seq.buffer);
> > + printk_deferred("%s", iter->seq.buffer);
> >
> > raw_spin_unlock_irqrestore(&tracepoint_iter_lock, flags);
> > }
> >
> > My assumption is similar views will apply here to Peter Zijlstra's comment on
> > not using printk_deferred.
> >
> > https://lore.kernel.org/all/20231010141244.GM377@noisy.programming.kicks-ass.net/
> >
> > Note I also tried the hacks Peter links to from there. They trip issues in the initial
> > CPER print - so I assume not appropriate here.
> >
> > So I'm thinking this is a won't fix - wait for the printk rework to land and
> > assume this will be resolved as well?
> >
>
> Or could we avoid the situation entirely by using a static call?
>
> The work queue still needs to be created because of the atomicness of
> ghes_do_proc() but it avoids cxl_cper_rw_sem.
>
> I think the hardest thing may be writing the commit message to explain all
> this. :-(
>

Never mind, I already went down that path. I was surprised I did not
mention it in this commit message. I did in V1. :-(

"A static call was considered but ARM does not select HAVE_STATIC_CALL
and in that case setting the function pointer uses a RW semaphore."
-- https://lore.kernel.org/all/20240202-cxl-cper-smatch-v1-1-7a4103c7f5a0@intel.com/

Should have carried that through.

So should we revert ...

Fixes: 671a794c33c6 ("acpi/ghes: Process CXL Component Events")

... and wait for the printk fix or just move forward with this patch?

Sorry for the noise,
Ira