| Message ID | 87c0f41e43a1c95ef7d8923c77a2072eb9baee96.1690276551.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [XEN,v2] xen/spinlock: mechanically rename parameter name 'debug' | expand |
On 25.07.2023 11:17, Nicola Vetrini wrote: > Rule 5.3 has the following headline: > "An identifier declared in an inner scope shall not hide an > identifier declared in an outer scope" > > To avoid any confusion resulting from the parameter 'debug' > hiding the homonymous function declared at > 'xen/arch/x86/include/asm/processor.h:428' > the rename of parameters s/debug/lkdbg/ is performed. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > Changes in v2: > - s/dbg/lkdbg/ But only in some of the cases. E.g. ... > -static void check_barrier(union lock_debug *debug) > +static void check_barrier(union lock_debug *dbg) ... not here (there are a few more). Jan
On Tue, 25 Jul 2023, Jan Beulich wrote: > On 25.07.2023 11:17, Nicola Vetrini wrote: > > Rule 5.3 has the following headline: > > "An identifier declared in an inner scope shall not hide an > > identifier declared in an outer scope" > > > > To avoid any confusion resulting from the parameter 'debug' > > hiding the homonymous function declared at > > 'xen/arch/x86/include/asm/processor.h:428' > > the rename of parameters s/debug/lkdbg/ is performed. > > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > > --- > > Changes in v2: > > - s/dbg/lkdbg/ > > But only in some of the cases. E.g. ... > > > -static void check_barrier(union lock_debug *debug) > > +static void check_barrier(union lock_debug *dbg) > > ... not here (there are a few more). I agree with Jan: these are all union lock_debug parameters, so it would make sense to me to use lkdbg everywhere in this patch.
On 25/07/23 21:37, Stefano Stabellini wrote: > On Tue, 25 Jul 2023, Jan Beulich wrote: >> On 25.07.2023 11:17, Nicola Vetrini wrote: >>> Rule 5.3 has the following headline: >>> "An identifier declared in an inner scope shall not hide an >>> identifier declared in an outer scope" >>> >>> To avoid any confusion resulting from the parameter 'debug' >>> hiding the homonymous function declared at >>> 'xen/arch/x86/include/asm/processor.h:428' >>> the rename of parameters s/debug/lkdbg/ is performed. >>> >>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>> --- >>> Changes in v2: >>> - s/dbg/lkdbg/ >> >> But only in some of the cases. E.g. ... >> >>> -static void check_barrier(union lock_debug *debug) >>> +static void check_barrier(union lock_debug *dbg) >> >> ... not here (there are a few more). > > I agree with Jan: these are all union lock_debug parameters, so it would > make sense to me to use lkdbg everywhere in this patch. Yes, indeed, that's unintentional. Can this be done on commit or should I send a v3? Regards,
On Tue, 25 Jul 2023, Nicola Vetrini wrote: > On 25/07/23 21:37, Stefano Stabellini wrote: > > On Tue, 25 Jul 2023, Jan Beulich wrote: > > > On 25.07.2023 11:17, Nicola Vetrini wrote: > > > > Rule 5.3 has the following headline: > > > > "An identifier declared in an inner scope shall not hide an > > > > identifier declared in an outer scope" > > > > > > > > To avoid any confusion resulting from the parameter 'debug' > > > > hiding the homonymous function declared at > > > > 'xen/arch/x86/include/asm/processor.h:428' > > > > the rename of parameters s/debug/lkdbg/ is performed. > > > > > > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > > > > --- > > > > Changes in v2: > > > > - s/dbg/lkdbg/ > > > > > > But only in some of the cases. E.g. ... > > > > > > > -static void check_barrier(union lock_debug *debug) > > > > +static void check_barrier(union lock_debug *dbg) > > > > > > ... not here (there are a few more). > > > > I agree with Jan: these are all union lock_debug parameters, so it would > > make sense to me to use lkdbg everywhere in this patch. > > Yes, indeed, that's unintentional. Can this be done on commit or should I send > a v3? Please send an update if possible
On 25.07.2023 22:28, Nicola Vetrini wrote: > > > On 25/07/23 21:37, Stefano Stabellini wrote: >> On Tue, 25 Jul 2023, Jan Beulich wrote: >>> On 25.07.2023 11:17, Nicola Vetrini wrote: >>>> Rule 5.3 has the following headline: >>>> "An identifier declared in an inner scope shall not hide an >>>> identifier declared in an outer scope" >>>> >>>> To avoid any confusion resulting from the parameter 'debug' >>>> hiding the homonymous function declared at >>>> 'xen/arch/x86/include/asm/processor.h:428' >>>> the rename of parameters s/debug/lkdbg/ is performed. >>>> >>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>> --- >>>> Changes in v2: >>>> - s/dbg/lkdbg/ >>> >>> But only in some of the cases. E.g. ... >>> >>>> -static void check_barrier(union lock_debug *debug) >>>> +static void check_barrier(union lock_debug *dbg) >>> >>> ... not here (there are a few more). >> >> I agree with Jan: these are all union lock_debug parameters, so it would >> make sense to me to use lkdbg everywhere in this patch. > > Yes, indeed, that's unintentional. Can this be done on commit or should > I send a v3? This wants to be a v3, but I'd suggest to wait a little with this until we've decided whether to go the alternative route and rename the entry point symbol that's colliding here. I would prefer this in general, but even more so if sooner or later we'd rename most of them anyway. Jan
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c index 7f453234a9..56813d4594 100644 --- a/xen/common/spinlock.c +++ b/xen/common/spinlock.c @@ -78,7 +78,7 @@ static int __init cf_check lockdebug_init(void) } presmp_initcall(lockdebug_init); -void check_lock(union lock_debug *debug, bool try) +void check_lock(union lock_debug *lkdbg, bool try) { bool irq_safe = !local_irq_is_enabled(); unsigned int cpu = smp_processor_id(); @@ -118,12 +118,12 @@ void check_lock(union lock_debug *debug, bool try) if ( try && irq_safe ) return; - if ( unlikely(debug->irq_safe != irq_safe) ) + if ( unlikely(lkdbg->irq_safe != irq_safe) ) { union lock_debug seen, new = { 0 }; new.irq_safe = irq_safe; - seen.val = cmpxchg(&debug->val, LOCK_DEBUG_INITVAL, new.val); + seen.val = cmpxchg(&lkdbg->val, LOCK_DEBUG_INITVAL, new.val); if ( !seen.unseen && seen.irq_safe == !irq_safe ) { @@ -137,14 +137,14 @@ void check_lock(union lock_debug *debug, bool try) return; for ( i = 0; i < nr_taken; i++ ) - if ( taken[i] == debug ) + if ( taken[i] == lkdbg ) { - printk("CHECKLOCK FAILURE: lock at %p taken recursively\n", debug); + printk("CHECKLOCK FAILURE: lock at %p taken recursively\n", lkdbg); BUG(); } } -static void check_barrier(union lock_debug *debug) +static void check_barrier(union lock_debug *dbg) { if ( unlikely(atomic_read(&spin_debug) <= 0) ) return; @@ -160,10 +160,10 @@ static void check_barrier(union lock_debug *debug) * However, if we spin on an IRQ-unsafe lock with IRQs disabled then that * is clearly wrong, for the same reason outlined in check_lock() above. */ - BUG_ON(!local_irq_is_enabled() && !debug->irq_safe); + BUG_ON(!local_irq_is_enabled() && !dbg->irq_safe); } -void lock_enter(const union lock_debug *debug) +void lock_enter(const union lock_debug *lkdbg) { unsigned int cpu = smp_processor_id(); const union lock_debug **taken = per_cpu(locks_taken, cpu); @@ -176,7 +176,7 @@ void lock_enter(const union lock_debug *debug) local_irq_save(flags); if ( *nr_taken < lock_depth_size ) - taken[(*nr_taken)++] = debug; + taken[(*nr_taken)++] = lkdbg; else if ( !max_depth_reached ) { max_depth_reached = true; @@ -187,7 +187,7 @@ void lock_enter(const union lock_debug *debug) local_irq_restore(flags); } -void lock_exit(const union lock_debug *debug) +void lock_exit(const union lock_debug *lkdbg) { unsigned int cpu = smp_processor_id(); const union lock_debug **taken = per_cpu(locks_taken, cpu); @@ -202,7 +202,7 @@ void lock_exit(const union lock_debug *debug) for ( i = *nr_taken; i > 0; i-- ) { - if ( taken[i - 1] == debug ) + if ( taken[i - 1] == lkdbg ) { memmove(taken + i - 1, taken + i, (*nr_taken - i) * sizeof(*taken)); @@ -217,28 +217,28 @@ void lock_exit(const union lock_debug *debug) if ( !max_depth_reached ) { - printk("CHECKLOCK released lock at %p not recorded!\n", debug); + printk("CHECKLOCK released lock at %p not recorded!\n", lkdbg); WARN(); } local_irq_restore(flags); } -static void got_lock(union lock_debug *debug) +static void got_lock(union lock_debug *dbg) { - debug->cpu = smp_processor_id(); + dbg->cpu = smp_processor_id(); - lock_enter(debug); + lock_enter(dbg); } -static void rel_lock(union lock_debug *debug) +static void rel_lock(union lock_debug *dbg) { if ( atomic_read(&spin_debug) > 0 ) - BUG_ON(debug->cpu != smp_processor_id()); + BUG_ON(dbg->cpu != smp_processor_id()); - lock_exit(debug); + lock_exit(dbg); - debug->cpu = SPINLOCK_NO_CPU; + dbg->cpu = SPINLOCK_NO_CPU; } void spin_debug_enable(void) diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h index 0a02a527dc..464af705eb 100644 --- a/xen/include/xen/spinlock.h +++ b/xen/include/xen/spinlock.h @@ -22,9 +22,9 @@ union lock_debug { }; }; #define _LOCK_DEBUG { LOCK_DEBUG_INITVAL } -void check_lock(union lock_debug *debug, bool try); -void lock_enter(const union lock_debug *debug); -void lock_exit(const union lock_debug *debug); +void check_lock(union lock_debug *lkdbg, bool try); +void lock_enter(const union lock_debug *lkdbg); +void lock_exit(const union lock_debug *lkdbg); void spin_debug_enable(void); void spin_debug_disable(void); #else
Rule 5.3 has the following headline: "An identifier declared in an inner scope shall not hide an identifier declared in an outer scope" To avoid any confusion resulting from the parameter 'debug' hiding the homonymous function declared at 'xen/arch/x86/include/asm/processor.h:428' the rename of parameters s/debug/lkdbg/ is performed. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- Changes in v2: - s/dbg/lkdbg/ --- xen/common/spinlock.c | 38 +++++++++++++++++++------------------- xen/include/xen/spinlock.h | 6 +++--- 2 files changed, 22 insertions(+), 22 deletions(-)