Re: [PATCH printk-rework 08/14] printk: add syslog_lock

[Petr Mladek]

On Sun 2021-02-21 22:39:42, Helge Deller wrote:
> On 2/19/21 5:33 PM, John Ogness wrote:
> > Added CC: linux-parisc@xxxxxxxxxxxxxxx
> > 
> > On 2021-02-19, John Ogness <john.ogness@xxxxxxxxxxxxx> wrote:
> > > > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > > > > index 20c21a25143d..401df370832b 100644
> > > > > --- a/kernel/printk/printk.c
> > > > > +++ b/kernel/printk/printk.c
> > > > > +/* Return a consistent copy of @syslog_seq. */
> > > > > +static u64 read_syslog_seq_irq(void)
> > > > > +{
> > > > > +	u64 seq;
> > > > > +
> > > > > +	raw_spin_lock_irq(&syslog_lock);
> > > > > +	seq = syslog_seq;
> > > > > +	raw_spin_unlock_irq(&syslog_lock);
> > > > 
> > > > Is there any particular reason to disable interrupts here?
> > > > 
> > > > It would make sense only when the lock could be taken in IRQ
> > > > context. Then we would need to always disable interrupts when
> > > > the lock is taken. And if it is taken in IRQ context, we would
> > > > need to safe flags.
> > > 
> > > All other instances of locking @syslog_lock are done with interrupts
> > > disabled. And we have:
> > > 
> > > register_console()
> > >    logbuf_lock_irqsave()
> > >      raw_spin_lock(&syslog_lock)
> > > 
> > > I suppose I need to go through all the console drivers to see if any
> > > register in interrupt context. If not, that logbuf_lock_irqsave()
> > > should be replaced with logbuf_lock_irq(). And then locking
> > > @syslog_lock will not need to disable interrupts.
> > 
> > I found a possible call chain in interrupt context. From arch/parisc
> > there is the interrupt handler:
> > 
> > handle_interruption(code=1) /* High-priority machine check (HPMC) */
> >    pdc_console_restart()
> >      pdc_console_init_force()
> >        register_console()
> > 
> > All other register_console() calls in the kernel are either during init
> > (within __init sections and probe functions) or are clearly not in
> > interrupt context (using mutex, kzalloc, spin_lock_irq, etc).
> > 
> > I am not familiar with parisc, but I am assuming handle_interruption()
> > is always called with interrupts disabled (unless the HPMC interrupt is
> > somehow an exception).
> 
> Yes, handle_interruption() is the irq handler, running with irqs off.
> HPMC is the crash handler - it's called when the kernel will stop
> anyway. pdc_console is a very basic firmware console which prints
> the last messages before the machine halts on fatal errors.
> So, this code it's not the typical use case....

Thanks for information.

Is this code supposed to work only during early boot or anytime,
please?

Note that it is not safe because register_console() takes
console_lock() which is a sleeping lock.

That said, we are going to rework the console handling a lot. We are
trying to remove as many locks from the printk path as possible.
I guess that the list of consoles will be synchronized using
rcu at the end. But it is still a long way to go.

Best Regards,
Petr