Re: netconsole: HARDIRQ-safe -> HARDIRQ-unsafe lock order warning

[Mike Galbraith <efault@gmx.de>]

On Thu, 2025-08-21 at 10:35 -0700, Breno Leitao wrote:
> > On Thu, Aug 21, 2025 at 05:51:59AM +0200, Mike Galbraith wrote:
 
> > > > --- a/drivers/net/netconsole.c
> > > > +++ b/drivers/net/netconsole.c
> > > > @@ -1952,12 +1952,12 @@ static void netcon_write_thread(struct c
> > > >  static void netconsole_device_lock(struct console *con, unsigned long *flags)
> > > >  {
> > > >  	/* protects all the targets at the same time */
> > > > -	spin_lock_irqsave(&target_list_lock, *flags);
> > > > +	spin_lock(&target_list_lock);
> > 
> > I personally think this target_list_lock can be moved to an RCU lock.
> > 
> > If that is doable, then we probably make netconsole_device_lock()
> > to a simple `rcu_read_lock()`, which would solve this problem as well.

The bigger issue for the nbcon patch would seem to be the seemingly
required .write_atomic leading to landing here with disabled IRQs.

WRT my patch, seeing a hard RT crash on wired box cleanly logged with
your nbcon patch applied (plus my twiddle mentioned earlier) tells me
my patch has lost its original reason to exist.  It's relevant to this
thread only in that those once thought to be RT specific IRQ disable
spots turned out to actually be RT agnostic wireless sore spots.

> > > > --- a/net/core/netpoll.c
> > > > +++ b/net/core/netpoll.c
> > > > @@ -58,6 +58,29 @@ static void zap_completion_queue(void);
> > > >  static unsigned int carrier_timeout = 4;
> > > >  module_param(carrier_timeout, uint, 0644);
> > > >  
> > > > +DEFINE_PER_CPU(int, _netpoll_tx_running);
> > > > +EXPORT_PER_CPU_SYMBOL(_netpoll_tx_running);
> > > > +
> > > > +#define
> > > > netpoll_tx_begin(flags)					\
> > > > +	do
> > > > {							\
> > > > +		if (IS_ENABLED(CONFIG_PREEMPT_RT)
> > > > ||		\
> > > > +		   
> > > > IS_ENABLED(CONFIG_NETCONSOLE_NBCON))	\
> > > > +			local_bh_disable();		
> > > > 	\
> > > > +		else					
> > > > 	\
> > > > +			local_irq_save(flags);		
> > > > 	\
> > > > +		this_cpu_write(_netpoll_tx_running,
> > > > 1);		\
> > > > +	} while (0)
> > 
> > Why can't we just use local_bh_disable() in both cases?

Yeah, believe so.

> > > 
> > > > @@ -246,7 +269,7 @@ static void refill_skbs(struct netpoll *
> > > >  static void zap_completion_queue(void)
> > > >  {
> > > >  	unsigned long flags;
> > > > -	struct softnet_data *sd = &get_cpu_var(softnet_data);
> > > > +	struct softnet_data *sd = this_cpu_ptr(&softnet_data);
> > 
> > How do I check if this is safe to do ?

Too much water under the bridge, I don't recall my path to conclusion
reached, and it seems to no longer matter.

	-Mike