Re: [PATCH v4 03/12] xen/spinlock: introduce new type for recursive spinlocks

[Juergen Gross <jgross@xxxxxxxx>]

On 12.12.23 13:57, Julien Grall wrote:
> Hi Juergen,
>
> On 12/12/2023 09:47, Juergen Gross wrote:
> > diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
> > index 1cd9120eac..20d15f34dd 100644
> > --- a/xen/include/xen/spinlock.h
> > +++ b/xen/include/xen/spinlock.h
> > @@ -45,7 +45,7 @@ union lock_debug { };
> >       lock profiling on:
> >       Global locks which should be subject to profiling must be declared via
> > -    DEFINE_SPINLOCK.
> > +    DEFINE_[R]SPINLOCK.
> >       For locks in structures further measures are necessary:
> >       - the structure definition must include a profile_head with exactly this
> > @@ -56,7 +56,7 @@ union lock_debug { };
> >       - the single locks which are subject to profiling have to be initialized
> >         via
> > -      spin_lock_init_prof(ptr, lock);
> > +      [r]spin_lock_init_prof(ptr, lock);
> >         with ptr being the main structure pointer and lock the spinlock field
> > @@ -109,12 +109,16 @@ struct lock_profile_qhead {
> >       spinlock_t l = _SPIN_LOCK_UNLOCKED(NULL);                                 
> > \
> >       static struct lock_profile __lock_profile_data_##l = _LOCK_PROFILE(l);    
> > \
> >       _LOCK_PROFILE_PTR(l)
> > +#define DEFINE_RSPINLOCK(l)                                                   \
> > +    rspinlock_t l = _SPIN_LOCK_UNLOCKED(NULL);                                \
> > +    static struct lock_profile __lock_profile_data_##l = _LOCK_PROFILE(l);    \
> > +    _LOCK_PROFILE_PTR(l)
> > -#define spin_lock_init_prof(s, l)                                             \
> > +#define __spin_lock_init_prof(s, l, locktype)                                 \
> If I am not mistaken the double-underscore prefix is a violation in MISRA. So 
> can this be renamed to:
> spin_lock_init_prof__()?
Fine with me.

Note that __lock_profile_data_##l and __lock_profile_##name seem to violate
MISRA, too. Probably a good idea to change them as well? This should probably
be done as a prereq patch then?

> The rest of the code looks fine. I have checked the lock you are modifying in 
> common/drivers and they all are meant to be recursive lock:
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
Thanks,


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature