Re: [PATCH next v2 1/2] dump_stack: move cpu lock to printk.c

From: Petr Mladek <pmladek@suse.com>
To: John Ogness <john.ogness@linutronix.de>
Cc: kernel test robot <lkp@intel.com>,
	kbuild-all@lists.01.org,
	Sergey Senozhatsky <senozhatsky@chromium.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	linux-kernel@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Linux Memory Management List <linux-mm@kvack.org>,
	Dmitry Safonov <0x7f454c46@gmail.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Marco Elver <elver@google.com>
Subject: Re: [PATCH next v2 1/2] dump_stack: move cpu lock to printk.c
Date: Fri, 11 Jun 2021 09:00:34 +0200	[thread overview]
Message-ID: <YMMKErwuH7ps8fNe@alley> (raw)
In-Reply-To: <877dj1u9zc.fsf@jogness.linutronix.de>

On Thu 2021-06-10 15:26:15, John Ogness wrote:
> On 2021-06-08, Petr Mladek <pmladek@suse.com> wrote:
> >>    lib/dump_stack.c: In function 'dump_stack_lvl':
> >> >> lib/dump_stack.c:107:2: warning: 'lock_flag' is used uninitialized in this function [-Wuninitialized]
> >>      107 |  printk_cpu_unlock_irqrestore(lock_flag, irq_flags);
> >>          |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Interesting. I am curious that it does not complain also about
> > irq_flags. But it is possible the it reports only the first problem.
> 
> Strangely enough, if I set a value for @lock_flag, it is happy and does
> not complain about @irq_flags. Probably a compiler oversight.

Yeah, it is strange.

> > Anyway, we will likely need to do some trickery via #define to tell
> > the compiler that the value is set.
> 
> This is on ARCH=mips and !CONFIG_SMP. So the value is _not_ getting
> set. (The static inline function does nothing.)
> 
> By changing printk_cpu_unlock_irqrestore() to use pointers:
> 
>     static inline void printk_cpu_unlock_irqrestore(bool *lock_flag, unsigned long *irq_flags)
> 
> then the warning disappears. Indeed, by not using pointers on unlock,
> technically data is copied that was never initialized. I thought maybe
> the compiler would optimize all that out, but it seems that it does not.
> 
> I have no problems using pointers for unlock(). It was strange using
> pointers for lock(), but not for unlock() anyway.
> 
> Or would you prefer something else?

I would actually prefer to introduce the macros and pass the flags
without referencing.

I was about to write that I did not mind. But then it came to me that
it might be worth being compatible with the other
irqsafe()/irqrestore() APIs. It seems that people are pretty used
to pass flags directly:

$> git grep irqsave.*flags | wc -l
17084
$> git grep irqsave.*\&flags | wc -l
15

That said, I do not resist on it. It will not block the patchset
if you decided to used the pointers. The lock should not be used
widely...

Best Regards,
Petr