Messages in this thread | | | Date | Tue, 8 Jun 2021 16:54:58 +0200 | From | Petr Mladek <> | Subject | Re: [PATCH next v2 1/2] dump_stack: move cpu lock to printk.c |
| |
On Tue 2021-06-08 15:55:35, John Ogness wrote: > On 2021-06-08, Petr Mladek <pmladek@suse.com> wrote: > > Reviewed-by: Petr Mladek <pmladek@suse.com> > > > >> --- a/kernel/printk/printk.c > >> +++ b/kernel/printk/printk.c > >> @@ -3532,3 +3532,78 @@ void kmsg_dump_rewind(struct kmsg_dump_iter *iter) > >> +void printk_cpu_lock_irqsave(bool *lock_flag, unsigned long *irq_flags) > >> +{ > >> + int old; > >> + int cpu; > >> + > >> +retry: > >> + local_irq_save(*irq_flags); > >> + > >> + cpu = smp_processor_id(); > >> + > >> + old = atomic_cmpxchg(&printk_cpulock_owner, -1, cpu); > >> + if (old == -1) { > >> + /* This CPU is now the owner. */ > >> + > > > > Superfluous space? > > I was concerned that people may associate the comment with the following > line of code. Especially in the next patch when many more lines are > added. The comment is for the whole conditional block. > > >> + *lock_flag = true; > > > > The original name name "was_locked" was more descriptive. I agree that > > it was not good for an API. What about keeping the inverted logic and > > calling it "lock_nested" ? > > > > I do not resist on any change. The logic is trivial so... > > I wanted it to be an opaque variable, which is why it is named so. But I > can rename it for v3. There is no need to debate naming here.
Yup. I didn't want to block the patch because of this. I mentioned it just for case v3 was needed and you agreed. Feel free to keep your preferred names and spacing. I am not going to fight over it.
Best Regards, Petr
| |